Problem rendering VBO
- by Onno
I'm developing a game engine using OpenTK. I'm trying to get to grips with the use of VBO's. I've run into some trouble because somehow it doesn't render correctly.
Thus far I've used immediate mode to render a test object, a test cube with a texture.
namespace SharpEngine.Utility.Mesh
{
using System;
using System.Collections.Generic;
using OpenTK;
using OpenTK.Graphics;
using OpenTK.Graphics.OpenGL;
using SharpEngine.Utility;
using System.Drawing;
public class ImmediateFaceBasedCube : IMesh
{
private IList<Face> faces = new List<Face>();
public ImmediateFaceBasedCube()
{
IList<Vector3> allVertices = new List<Vector3>();
//rechtsbovenvoor
allVertices.Add(new Vector3(1.0f, 1.0f, 1.0f)); //0
//rechtsbovenachter
allVertices.Add(new Vector3(1.0f, 1.0f, -1.0f)); //1
//linksbovenachter
allVertices.Add(new Vector3(-1.0f, 1.0f, -1.0f)); //2
//linksbovenvoor
allVertices.Add(new Vector3(-1.0f, 1.0f, 1.0f)); //3
//rechtsondervoor
allVertices.Add(new Vector3(1.0f, -1.0f, 1.0f)); //4
//rechtsonderachter
allVertices.Add(new Vector3(1.0f, -1.0f, -1.0f)); //5
//linksonderachter
allVertices.Add(new Vector3(-1.0f, -1.0f, -1.0f)); //6
//linksondervoor
allVertices.Add(new Vector3(-1.0f, -1.0f, 1.0f)); //7
IList<Vector2> textureCoordinates = new List<Vector2>();
textureCoordinates.Add(new Vector2(0, 0)); //AA - 0
textureCoordinates.Add(new Vector2(0, 0.3333333f)); //AB - 1
textureCoordinates.Add(new Vector2(0, 0.6666666f)); //AC - 2
textureCoordinates.Add(new Vector2(0, 1)); //AD - 3
textureCoordinates.Add(new Vector2(0.3333333f, 0)); //BA - 4
textureCoordinates.Add(new Vector2(0.3333333f, 0.3333333f)); //BB - 5
textureCoordinates.Add(new Vector2(0.3333333f, 0.6666666f)); //BC - 6
textureCoordinates.Add(new Vector2(0.3333333f, 1)); //BD - 7
textureCoordinates.Add(new Vector2(0.6666666f, 0)); //CA - 8
textureCoordinates.Add(new Vector2(0.6666666f, 0.3333333f)); //CB - 9
textureCoordinates.Add(new Vector2(0.6666666f, 0.6666666f)); //CC -10
textureCoordinates.Add(new Vector2(0.6666666f, 1)); //CD -11
textureCoordinates.Add(new Vector2(1, 0)); //DA -12
textureCoordinates.Add(new Vector2(1, 0.3333333f)); //DB -13
textureCoordinates.Add(new Vector2(1, 0.6666666f)); //DC -14
textureCoordinates.Add(new Vector2(1, 1)); //DD -15
Vector3 copy1 = new Vector3(-2.0f, -2.5f, -3.5f);
IList<Vector3> normals = new List<Vector3>();
normals.Add(new Vector3(0, 1.0f, 0)); //0
normals.Add(new Vector3(0, 0, 1.0f)); //1
normals.Add(new Vector3(1.0f, 0, 0)); //2
normals.Add(new Vector3(0, 0, -1.0f)); //3
normals.Add(new Vector3(-1.0f, 0, 0)); //4
normals.Add(new Vector3(0, -1.0f, 0)); //5
//todo: move vertex normal and texture data to datastructure
//todo: VBO based rendering
//top face
//1
IList<VertexData> verticesT1 = new List<VertexData>();
VertexData T1a = new VertexData();
T1a.Normal = normals[0];
T1a.TexCoord = textureCoordinates[5];
T1a.Position = allVertices[3];
verticesT1.Add(T1a);
VertexData T1b = new VertexData();
T1b.Normal = normals[0];
T1b.TexCoord = textureCoordinates[9];
T1b.Position = allVertices[0];
verticesT1.Add(T1b);
VertexData T1c = new VertexData();
T1c.Normal = normals[0];
T1c.TexCoord = textureCoordinates[10];
T1c.Position = allVertices[1];
verticesT1.Add(T1c);
Face F1 = new Face(verticesT1);
faces.Add(F1);
//2
IList<VertexData> verticesT2 = new List<VertexData>();
VertexData T2a = new VertexData();
T2a.Normal = normals[0];
T2a.TexCoord = textureCoordinates[10];
T2a.Position = allVertices[1];
verticesT2.Add(T2a);
VertexData T2b = new VertexData();
T2b.Normal = normals[0];
T2b.TexCoord = textureCoordinates[6];
T2b.Position = allVertices[2];
verticesT2.Add(T2b);
VertexData T2c = new VertexData();
T2c.Normal = normals[0];
T2c.TexCoord = textureCoordinates[5];
T2c.Position = allVertices[3];
verticesT2.Add(T2c);
Face F2 = new Face(verticesT2);
faces.Add(F2);
//front face
//3
IList<VertexData> verticesT3 = new List<VertexData>();
VertexData T3a = new VertexData();
T3a.Normal = normals[1];
T3a.TexCoord = textureCoordinates[1];
T3a.Position = allVertices[3];
verticesT3.Add(T3a);
VertexData T3b = new VertexData();
T3b.Normal = normals[1];
T3b.TexCoord = textureCoordinates[0];
T3b.Position = allVertices[7];
verticesT3.Add(T3b);
VertexData T3c = new VertexData();
T3c.Normal = normals[1];
T3c.TexCoord = textureCoordinates[5];
T3c.Position = allVertices[0];
verticesT3.Add(T3c);
Face F3 = new Face(verticesT3);
faces.Add(F3);
//4
IList<VertexData> verticesT4 = new List<VertexData>();
VertexData T4a = new VertexData();
T4a.Normal = normals[1];
T4a.TexCoord = textureCoordinates[5];
T4a.Position = allVertices[0];
verticesT4.Add(T4a);
VertexData T4b = new VertexData();
T4b.Normal = normals[1];
T4b.TexCoord = textureCoordinates[0];
T4b.Position = allVertices[7];
verticesT4.Add(T4b);
VertexData T4c = new VertexData();
T4c.Normal = normals[1];
T4c.TexCoord = textureCoordinates[4];
T4c.Position = allVertices[4];
verticesT4.Add(T4c);
Face F4 = new Face(verticesT4);
faces.Add(F4);
//right face
//5
IList<VertexData> verticesT5 = new List<VertexData>();
VertexData T5a = new VertexData();
T5a.Normal = normals[2];
T5a.TexCoord = textureCoordinates[2];
T5a.Position = allVertices[0];
verticesT5.Add(T5a);
VertexData T5b = new VertexData();
T5b.Normal = normals[2];
T5b.TexCoord = textureCoordinates[1];
T5b.Position = allVertices[4];
verticesT5.Add(T5b);
VertexData T5c = new VertexData();
T5c.Normal = normals[2];
T5c.TexCoord = textureCoordinates[6];
T5c.Position = allVertices[1];
verticesT5.Add(T5c);
Face F5 = new Face(verticesT5);
faces.Add(F5);
//6
IList<VertexData> verticesT6 = new List<VertexData>();
VertexData T6a = new VertexData();
T6a.Normal = normals[2];
T6a.TexCoord = textureCoordinates[1];
T6a.Position = allVertices[4];
verticesT6.Add(T6a);
VertexData T6b = new VertexData();
T6b.Normal = normals[2];
T6b.TexCoord = textureCoordinates[5];
T6b.Position = allVertices[5];
verticesT6.Add(T6b);
VertexData T6c = new VertexData();
T6c.Normal = normals[2];
T6c.TexCoord = textureCoordinates[6];
T6c.Position = allVertices[1];
verticesT6.Add(T6c);
Face F6 = new Face(verticesT6);
faces.Add(F6);
//back face
//7
IList<VertexData> verticesT7 = new List<VertexData>();
VertexData T7a = new VertexData();
T7a.Normal = normals[3];
T7a.TexCoord = textureCoordinates[4];
T7a.Position = allVertices[5];
verticesT7.Add(T7a);
VertexData T7b = new VertexData();
T7b.Normal = normals[3];
T7b.TexCoord = textureCoordinates[9];
T7b.Position = allVertices[2];
verticesT7.Add(T7b);
VertexData T7c = new VertexData();
T7c.Normal = normals[3];
T7c.TexCoord = textureCoordinates[5];
T7c.Position = allVertices[1];
verticesT7.Add(T7c);
Face F7 = new Face(verticesT7);
faces.Add(F7);
//8
IList<VertexData> verticesT8 = new List<VertexData>();
VertexData T8a = new VertexData();
T8a.Normal = normals[3];
T8a.TexCoord = textureCoordinates[9];
T8a.Position = allVertices[2];
verticesT8.Add(T8a);
VertexData T8b = new VertexData();
T8b.Normal = normals[3];
T8b.TexCoord = textureCoordinates[4];
T8b.Position = allVertices[5];
verticesT8.Add(T8b);
VertexData T8c = new VertexData();
T8c.Normal = normals[3];
T8c.TexCoord = textureCoordinates[8];
T8c.Position = allVertices[6];
verticesT8.Add(T8c);
Face F8 = new Face(verticesT8);
faces.Add(F8);
//left face
//9
IList<VertexData> verticesT9 = new List<VertexData>();
VertexData T9a = new VertexData();
T9a.Normal = normals[4];
T9a.TexCoord = textureCoordinates[8];
T9a.Position = allVertices[6];
verticesT9.Add(T9a);
VertexData T9b = new VertexData();
T9b.Normal = normals[4];
T9b.TexCoord = textureCoordinates[13];
T9b.Position = allVertices[3];
verticesT9.Add(T9b);
VertexData T9c = new VertexData();
T9c.Normal = normals[4];
T9c.TexCoord = textureCoordinates[9];
T9c.Position = allVertices[2];
verticesT9.Add(T9c);
Face F9 = new Face(verticesT9);
faces.Add(F9);
//10
IList<VertexData> verticesT10 = new List<VertexData>();
VertexData T10a = new VertexData();
T10a.Normal = normals[4];
T10a.TexCoord = textureCoordinates[8];
T10a.Position = allVertices[6];
verticesT10.Add(T10a);
VertexData T10b = new VertexData();
T10b.Normal = normals[4];
T10b.TexCoord = textureCoordinates[12];
T10b.Position = allVertices[7];
verticesT10.Add(T10b);
VertexData T10c = new VertexData();
T10c.Normal = normals[4];
T10c.TexCoord = textureCoordinates[13];
T10c.Position = allVertices[3];
verticesT10.Add(T10c);
Face F10 = new Face(verticesT10);
faces.Add(F10);
//bottom face
//11
IList<VertexData> verticesT11 = new List<VertexData>();
VertexData T11a = new VertexData();
T11a.Normal = normals[5];
T11a.TexCoord = textureCoordinates[10];
T11a.Position = allVertices[7];
verticesT11.Add(T11a);
VertexData T11b = new VertexData();
T11b.Normal = normals[5];
T11b.TexCoord = textureCoordinates[9];
T11b.Position = allVertices[6];
verticesT11.Add(T11b);
VertexData T11c = new VertexData();
T11c.Normal = normals[5];
T11c.TexCoord = textureCoordinates[14];
T11c.Position = allVertices[4];
verticesT11.Add(T11c);
Face F11 = new Face(verticesT11);
faces.Add(F11);
//12
IList<VertexData> verticesT12 = new List<VertexData>();
VertexData T12a = new VertexData();
T12a.Normal = normals[5];
T12a.TexCoord = textureCoordinates[13];
T12a.Position = allVertices[5];
verticesT12.Add(T12a);
VertexData T12b = new VertexData();
T12b.Normal = normals[5];
T12b.TexCoord = textureCoordinates[14];
T12b.Position = allVertices[4];
verticesT12.Add(T12b);
VertexData T12c = new VertexData();
T12c.Normal = normals[5];
T12c.TexCoord = textureCoordinates[9];
T12c.Position = allVertices[6];
verticesT12.Add(T12c);
Face F12 = new Face(verticesT12);
faces.Add(F12);
}
public void draw()
{
GL.Begin(BeginMode.Triangles);
foreach (Face face in faces)
{
foreach (VertexData datapoint in face.verticesWithTexCoords)
{
GL.Normal3(datapoint.Normal);
GL.TexCoord2(datapoint.TexCoord);
GL.Vertex3(datapoint.Position);
}
}
GL.End();
}
}
}
Gets me this very nice picture:
The immediate mode cube renders nicely and taught me a bit on how to use OpenGL, but VBO's are the way to go. Since I read on the OpenTK forums that OpenTK has problems doing VA's or DL's, I decided to skip using those.
Now, I've tried to change this cube to a VBO by using the same vertex, normal and tc collections, and making float arrays from them by using the coordinates in combination with uint arrays which contain the index numbers from the immediate cube. (see the private functions at end of the code sample)
Somehow this only renders two triangles
namespace SharpEngine.Utility.Mesh
{
using System;
using System.Collections.Generic;
using OpenTK;
using OpenTK.Graphics;
using OpenTK.Graphics.OpenGL;
using SharpEngine.Utility;
using System.Drawing;
public class VBOFaceBasedCube : IMesh
{
private int VerticesVBOID;
private int VerticesVBOStride;
private int VertexCount;
private int ELementBufferObjectID;
private int textureCoordinateVBOID;
private int textureCoordinateVBOStride;
//private int textureCoordinateArraySize;
private int normalVBOID;
private int normalVBOStride;
public VBOFaceBasedCube()
{
IList<Vector3> allVertices = new List<Vector3>();
//rechtsbovenvoor
allVertices.Add(new Vector3(1.0f, 1.0f, 1.0f)); //0
//rechtsbovenachter
allVertices.Add(new Vector3(1.0f, 1.0f, -1.0f)); //1
//linksbovenachter
allVertices.Add(new Vector3(-1.0f, 1.0f, -1.0f)); //2
//linksbovenvoor
allVertices.Add(new Vector3(-1.0f, 1.0f, 1.0f)); //3
//rechtsondervoor
allVertices.Add(new Vector3(1.0f, -1.0f, 1.0f)); //4
//rechtsonderachter
allVertices.Add(new Vector3(1.0f, -1.0f, -1.0f)); //5
//linksonderachter
allVertices.Add(new Vector3(-1.0f, -1.0f, -1.0f)); //6
//linksondervoor
allVertices.Add(new Vector3(-1.0f, -1.0f, 1.0f)); //7
IList<Vector2> textureCoordinates = new List<Vector2>();
textureCoordinates.Add(new Vector2(0, 0)); //AA - 0
textureCoordinates.Add(new Vector2(0, 0.3333333f)); //AB - 1
textureCoordinates.Add(new Vector2(0, 0.6666666f)); //AC - 2
textureCoordinates.Add(new Vector2(0, 1)); //AD - 3
textureCoordinates.Add(new Vector2(0.3333333f, 0)); //BA - 4
textureCoordinates.Add(new Vector2(0.3333333f, 0.3333333f)); //BB - 5
textureCoordinates.Add(new Vector2(0.3333333f, 0.6666666f)); //BC - 6
textureCoordinates.Add(new Vector2(0.3333333f, 1)); //BD - 7
textureCoordinates.Add(new Vector2(0.6666666f, 0)); //CA - 8
textureCoordinates.Add(new Vector2(0.6666666f, 0.3333333f)); //CB - 9
textureCoordinates.Add(new Vector2(0.6666666f, 0.6666666f)); //CC -10
textureCoordinates.Add(new Vector2(0.6666666f, 1)); //CD -11
textureCoordinates.Add(new Vector2(1, 0)); //DA -12
textureCoordinates.Add(new Vector2(1, 0.3333333f)); //DB -13
textureCoordinates.Add(new Vector2(1, 0.6666666f)); //DC -14
textureCoordinates.Add(new Vector2(1, 1)); //DD -15
Vector3 copy1 = new Vector3(-2.0f, -2.5f, -3.5f);
IList<Vector3> normals = new List<Vector3>();
normals.Add(new Vector3(0, 1.0f, 0)); //0
normals.Add(new Vector3(0, 0, 1.0f)); //1
normals.Add(new Vector3(1.0f, 0, 0)); //2
normals.Add(new Vector3(0, 0, -1.0f)); //3
normals.Add(new Vector3(-1.0f, 0, 0)); //4
normals.Add(new Vector3(0, -1.0f, 0)); //5
//todo: VBO based rendering
uint[] vertexElements = {
3,0,1, //01
1,2,3, //02
3,7,0, //03
0,7,4, //04
0,4,1, //05
4,5,1, //06
5,2,1, //07
2,5,6, //08
6,3,2, //09
6,7,5, //10
7,6,4, //11
5,4,6 //12
};
VertexCount = vertexElements.Length;
IList<uint> vertexElementList = new List<uint>(vertexElements);
uint[] normalElements = {
0,0,0,
0,0,0,
1,1,1,
1,1,1,
2,2,2,
2,2,2,
3,3,3,
3,3,3,
4,4,4,
4,4,4,
5,5,5,
5,5,5
};
IList<uint> normalElementList = new List<uint>(normalElements);
uint[] textureIndexArray = {
5,9,10,
10,6,5,
1,0,5,
5,0,4,
2,1,6,
1,5,6,
4,9,5,
9,4,8,
8,13,9,
8,12,13,
10,9,14,
13,14,9
};
//textureCoordinateArraySize = textureIndexArray.Length;
IList<uint> textureIndexList = new List<uint>(textureIndexArray);
LoadVBO(allVertices, normals, textureCoordinates, vertexElements, normalElementList, textureIndexList);
}
public void draw()
{
//bind vertices
//bind elements
//bind normals
//bind texture coordinates
GL.EnableClientState(ArrayCap.VertexArray);
GL.EnableClientState(ArrayCap.NormalArray);
GL.EnableClientState(ArrayCap.TextureCoordArray);
GL.BindBuffer(BufferTarget.ArrayBuffer, VerticesVBOID);
GL.VertexPointer(3, VertexPointerType.Float, VerticesVBOStride, 0);
GL.BindBuffer(BufferTarget.ArrayBuffer, normalVBOID);
GL.NormalPointer(NormalPointerType.Float, normalVBOStride, 0);
GL.BindBuffer(BufferTarget.ArrayBuffer, textureCoordinateVBOID);
GL.TexCoordPointer(2, TexCoordPointerType.Float, textureCoordinateVBOStride, 0);
GL.BindBuffer(BufferTarget.ElementArrayBuffer, ELementBufferObjectID);
GL.DrawElements(BeginMode.Polygon, VertexCount, DrawElementsType.UnsignedShort, 0);
}
//loads a static VBO
void LoadVBO(IList<Vector3> vertices, IList<Vector3> normals, IList<Vector2> texcoords, uint[] elements, IList<uint> normalIndices, IList<uint> texCoordIndices)
{
int size;
//todo
// To create a VBO:
// 1) Generate the buffer handles for the vertex and element buffers.
// 2) Bind the vertex buffer handle and upload your vertex data. Check that the buffer was uploaded correctly.
// 3) Bind the element buffer handle and upload your element data. Check that the buffer was uploaded correctly.
float[] verticesArray = convertVector3fListToFloatArray(vertices);
float[] normalsArray = createFloatArrayFromListOfVector3ElementsAndIndices(normals, normalIndices);
float[] textureCoordinateArray = createFloatArrayFromListOfVector2ElementsAndIndices(texcoords, texCoordIndices);
GL.GenBuffers(1, out VerticesVBOID);
GL.BindBuffer(BufferTarget.ArrayBuffer, VerticesVBOID);
Console.WriteLine("load 1 - vertices");
VerticesVBOStride = BlittableValueType.StrideOf(verticesArray);
GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(verticesArray.Length * sizeof(float)), verticesArray,
BufferUsageHint.StaticDraw);
GL.GetBufferParameter(BufferTarget.ArrayBuffer, BufferParameterName.BufferSize, out size);
if (verticesArray.Length * BlittableValueType.StrideOf(verticesArray) != size)
{
throw new ApplicationException("Vertex data not uploaded correctly");
}
else
{
Console.WriteLine("load 1 finished ok");
size = 0;
}
Console.WriteLine("load 2 - elements");
GL.GenBuffers(1, out ELementBufferObjectID);
GL.BindBuffer(BufferTarget.ElementArrayBuffer, ELementBufferObjectID);
GL.BufferData(BufferTarget.ElementArrayBuffer, (IntPtr)(elements.Length * sizeof(uint)), elements,
BufferUsageHint.StaticDraw);
GL.GetBufferParameter(BufferTarget.ElementArrayBuffer, BufferParameterName.BufferSize, out size);
if (elements.Length * sizeof(uint) != size)
{
throw new ApplicationException("Element data not uploaded correctly");
}
else
{
size = 0;
Console.WriteLine("load 2 finished ok");
}
GL.GenBuffers(1, out normalVBOID);
GL.BindBuffer(BufferTarget.ArrayBuffer, normalVBOID);
Console.WriteLine("load 3 - normals");
normalVBOStride = BlittableValueType.StrideOf(normalsArray);
GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(normalsArray.Length * sizeof(float)), normalsArray,
BufferUsageHint.StaticDraw);
GL.GetBufferParameter(BufferTarget.ArrayBuffer, BufferParameterName.BufferSize, out size);
Console.WriteLine("load 3 - pre check");
if (normalsArray.Length * BlittableValueType.StrideOf(normalsArray) != size)
{
throw new ApplicationException("Normal data not uploaded correctly");
}
else
{
Console.WriteLine("load 3 finished ok");
size = 0;
}
GL.GenBuffers(1, out textureCoordinateVBOID);
GL.BindBuffer(BufferTarget.ArrayBuffer, textureCoordinateVBOID);
Console.WriteLine("load 4- texture coordinates");
textureCoordinateVBOStride = BlittableValueType.StrideOf(textureCoordinateArray);
GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(textureCoordinateArray.Length * textureCoordinateVBOStride), textureCoordinateArray,
BufferUsageHint.StaticDraw);
GL.GetBufferParameter(BufferTarget.ArrayBuffer, BufferParameterName.BufferSize, out size);
if (textureCoordinateArray.Length * BlittableValueType.StrideOf(textureCoordinateArray) != size)
{
throw new ApplicationException("texture coordinate data not uploaded correctly");
}
else
{
Console.WriteLine("load 3 finished ok");
size = 0;
}
}
//used to convert vertex arrayss for use with VBO's
private float[] convertVector3fListToFloatArray(IList<Vector3> input)
{
int arrayElementCount = input.Count * 3;
float[] output = new float[arrayElementCount];
int fillCount = 0;
foreach (Vector3 v in input)
{
output[fillCount] = v.X;
output[fillCount + 1] = v.Y;
output[fillCount + 2] = v.Z;
fillCount += 3;
}
return output;
}
//used for converting texture coordinate arrays for use with VBO's
private float[] convertVector2List_to_floatArray(IList<Vector2> input)
{
int arrayElementCount = input.Count * 2;
float[] output = new float[arrayElementCount];
int fillCount = 0;
foreach (Vector2 v in input)
{
output[fillCount] = v.X;
output[fillCount + 1] = v.Y;
fillCount += 2;
}
return output;
}
//used to create an array of floats from
private float[] createFloatArrayFromListOfVector3ElementsAndIndices(IList<Vector3> inputVectors, IList<uint> indices)
{
int arrayElementCount = inputVectors.Count * indices.Count * 3;
float[] output = new float[arrayElementCount];
int fillCount = 0;
foreach (int i in indices)
{
output[fillCount] = inputVectors[i].X;
output[fillCount + 1] = inputVectors[i].Y;
output[fillCount + 2] = inputVectors[i].Z;
fillCount += 3;
}
return output;
}
private float[] createFloatArrayFromListOfVector2ElementsAndIndices(IList<Vector2> inputVectors, IList<uint> indices)
{
int arrayElementCount = inputVectors.Count * indices.Count * 2;
float[] output = new float[arrayElementCount];
int fillCount = 0;
foreach (int i in indices)
{
output[fillCount] = inputVectors[i].X;
output[fillCount + 1] = inputVectors[i].Y;
fillCount += 2;
}
return output;
}
}
}
This code will only render two triangles and they're nothing like I had in mind:
I've done some searching. In some other questions I read that, if I did something wrong, I'd get no rendering at all. Clearly, something gets sent to the GFX card, but it might be that I'm not sending the right data. I've tried altering the sequence in which the triangles are rendered by swapping some of the index numbers in the vert, tc and normal index arrays, but this doesn't seem to be of any effect. I'm slightly lost here. What am I doing wrong here?
