raysan5 / raylib

A simple and easy-to-use library to enjoy videogames programming
http://www.raylib.com
zlib License
21.95k stars 2.22k forks source link

[models] Add GenMeshDefault to improve Mesh generation #1556

Closed chrisdill closed 3 years ago

chrisdill commented 3 years ago

Issue description

Currently to generate a Mesh with your own data(vertices, texcoords etc), you need to allocate it manually and make sure the vertexCount is setup properly. This works although I think it can be made easier for most use cases.

My suggestion is to add a GenMeshDefault function to help with Mesh creation. You pass in the vertexCount, triangleCount and a enum to flag the arrays you want and it allocates them for you. You could also store the flags in the mesh for querying later.

typedef enum {
    MESH_VERTEX,
    MESH_TEXCOORD,
    MESH_TEXCOORD2,
    MESH_NORMAL,
    MESH_TANGENT,
    MESH_COLOR,
    MESH_INDEX
} MeshFlag;

RLAPI GenMeshDefault(int vertexCount, int triangleCount, MeshFlag flags);

The naming can be changed as needed. This was the first thing that I thought of.

Any feedback or discussion about this issue is appreciated! :)

Code Example

// Generate a mesh with 60 vertices
int vertexCount = 60;
int flags = MESH_VERTEX | MESH_NORMAL | MESH_TEXCOORD;
Mesh mesh = GenMeshDefault(vertexCount, vertexCount/3, flags);

// vertices, normals and texcoords are now allocated for 60 vertices
mesh.vertices[0] = 1.0f;
// ...

// Unload mesh same as usual
UnloadMesh(mesh);
JeffM2501 commented 3 years ago

AllocateMeshData from

https://github.com/JeffM2501/raylibExtras/blob/1c56e6e2fe617642f0529d3fc73d2a33373ec9e5/RLGeoTools_C/RLGeoTools.c#L40 effectively does this already, having it included in the base system would be nice.

I would not call It GenMeshDefault because that makes it seem like the generated mesh is valid (like the default font is valid, and the default texture is valid). In this case the mesh is not valid, just allocated, so it may be better to simply name the function AllocateMesh, or just GenMesh, or GenMeshCustom or something like that.

raysan5 commented 3 years ago

GenMeshCustom(), I like it!

chrisdill commented 3 years ago

@JeffM2501 Makes sense. I agree that GenMeshCustom() is a clearer name. I have also looked at existing GenMesh functions and I think they can use this too since the data sizes are calculated the same.

chrisdill commented 3 years ago

Here is my initial implementation for GenMeshCustom. Been testing it with existing GenMesh functions/examples and it is working as expected.

Unsure on the enum name. We could use it with rlUpdateMesh and rlUpdateMeshAt in rlgl so maybe MeshAttributeType?

// Generate a mesh using flags to specify attributes
Mesh GenMeshCustom(int vertexCount, int triangleCount, int flags)
{
    Mesh mesh = { 0 };

    mesh.vertexCount = vertexCount;
    mesh.triangleCount = triangleCount;

    TraceLog(LOG_INFO, "GenMeshCustom: vertexCount=%d triangleCount=%d", mesh.vertexCount, mesh.triangleCount);

    if (flags & MESH_VERTEX)
    {
        mesh.vertices = (float *)RL_MALLOC(mesh.vertexCount*3*sizeof(float));
    }
    if (flags & MESH_TEXCOORD)
    {
        mesh.texcoords = (float *)RL_MALLOC(mesh.vertexCount*2*sizeof(float));
    }
    if (flags & MESH_TEXCOORD2)
    {
        mesh.texcoords2 = (float *)RL_MALLOC(mesh.vertexCount*2*sizeof(float));
    }
    if (flags & MESH_NORMAL)
    {
        mesh.normals = (float *)RL_MALLOC(mesh.vertexCount*3*sizeof(float));
    }
    if (flags & MESH_TANGENT)
    {
        mesh.tangents = (float *)RL_MALLOC(mesh.vertexCount*4*sizeof(float));
    }
    if (flags & MESH_COLOR)
    {
        mesh.colors = (unsigned char *)RL_MALLOC(mesh.vertexCount*4*sizeof(float));
    }
    if (flags & MESH_INDEX)
    {
        mesh.indices = (unsigned short *)RL_MALLOC(mesh.triangleCount*3*sizeof(float));
    }

    mesh.vboId = (unsigned int *)RL_CALLOC(DEFAULT_MESH_VERTEX_BUFFERS, sizeof(unsigned int));

    return mesh;
}
JeffM2501 commented 3 years ago

I don't think it's valid to generate a mesh without vertices, so having that as a flag is a bit redundant. The same may be true for texture coords as well. I see rlLoadMesh always assumes the vertecies and texcoords buffers exists. The others are checked against null.

I'd put the word "GEN" in the enums to show they go with generation, and so they don't conflict with other future terms. I'd call it MeshGenOptions or something like that, and have the values be GEN_MESH_NORMALS, GEN_MESH_COLORS, etc... That will make it more clear what is going on.

chrisdill commented 3 years ago

I can remove checks for required data if that is intended. Though I would keep the flags for updating the mesh. For example you can pass a vertex flag to rlUpdateMeshAt for the buffer without needing to specify the exact buffer id.

The GEN prefix makes sense if the flags are only used for generation. Maybe MeshBufferType for the name? Should make it clear what is going on for both use cases. The prefix could be MESH_BUFFER or BUFFER_MESH etc.

Crydsch commented 3 years ago

I'd like to add that keeping the flags and checks would allow one to create a mesh consisting only of an IndexBuffer. Then with a corresponding custom VertexShader one can generate the vertices. Of course we then need checks for the VertexBuffer and TexcoordBuffer in rlLoadMesh as well. This would make this custom function far more versatile.

raysan5 commented 3 years ago

@ChrisDill @JeffM2501 I'm doing a huge redesign of rlgl module to move all Mesh/Materials management to models module and now I'm considering this function... my question is, what are the benefits of having the function instead of just letting the user create the Mesh manually? This function implies an additional enumerator and dealing with flags... just thinking about it...

I'm also considering the usefulness of mesh.triangleCount variable, is it really required? what do you think?

Finally, about UpdateMesh(), I'm considering removing that function, I just implemented:

void rlUpdateVertexBuffer(int bufferId, void *data, int dataSize, int offset);

I think most users requiring updating vertex data on GPU could directly deal with rlUpdateVertexBuffer().

Well, just some thoughts, as commented, I'm doing a big internal redesign (that actually requires exposing a lot of OpenGL functionality in rlgl).

In case you wonder the reason for that big redesign, it would be useful for a future rlvk module, just in case...

chrisdill commented 3 years ago

The goal of GenMeshCustom was to make buffer creation easier since buffer sizes are set the same way. For example vertices is expected to be vertexCount * 3 * sizeof(float), texcoords is set to vertexCount * 2 * sizeof(float) etc.

Creating a Mesh manually works though you need to make sure it is set up properly. Maybe that is the tradeoff in flexibility.

I think mesh.triangleCount is useful for knowing how many triangles are being drawn. It can't be expected to be equal to vertexCount / 3 so it makes sense to store it separately.

chriscamacho commented 3 years ago

I think (off the top of my head) I might be relying on triCount in models.c to split OBJ's into material meshes, this needs careful review ! I still need to investigate further why its not currently working....

raysan5 commented 3 years ago

@ChrisDill @chriscamacho Actually, just discovered that for some Meshes (mesh.triangleCount*3 != mesh.vertexCount), that's because some meshes could contain indexed data... Mmmh... that should be investigated...

Crydsch commented 3 years ago

This is generally the case when using IndexedRendering. Its very idea is to reuse vertecies, thus (mesh.triangleCount*3 != mesh.vertexCount). I actually use this without an vertex buffer entirely, by generating the vertex data in the vertex shader. In this special case it actually possible to have (mesh.vertexCount == 0).

chriscamacho commented 3 years ago

@Crydsch so what kicks the vertex shader, if you have no vertex data...

@raysan5 afaict by the time stuff comes out of tinyobj_loader its all indexed, but then I've just looked up the indexes and probably on some models duplicating some verts (deliberately) I can't see the "wasted" memory being significant and it's potentially less lookups for the GPU, but who knows what's faster on what hardware with which drivers - impossible to second guess... so long story short with OBJ loading the *3 rule should hold...

Crydsch commented 3 years ago

@chriscamacho I am using the normal rlDrawMesh(..), then the vertex shader is invoked (once) for each index. You just don't have any vertex data input in your shader. You then generate your output data (including the vertex position) based on the given gl_VertexID.

Also, "un-indexing" meshes sounds very wrong to me. Indexed meshes should generally be preferred. Not only is your memory footprint smaller - which may or may not make a difference, but is never wrong - , further it is more performant. OpenGL (/drivers) can cache and re-use an once calculated vertex if the same index is used multiple times. This can significantly reduce your vertex shader invocations!

Source: https://www.khronos.org/opengl/wiki/Vertex_Shader#Invocation_frequency

chriscamacho commented 3 years ago

@Crydsch if unindexing is so wrong feel free to implement something better !

What you get in an OBJ is just a soup of vertexes, and a bunch of indexes, the easiest way to pick them apart is to "un-index" as you put it, in practice in almost all the models I tested there were very few duplicated vertices - that's just how artists roll...

regarding your zero vertex technique its not zero vertices its a whole bunch of vertices you're just ignoring the data (unless I misunderstand your explanation?), I struggle to see the advantage of this technique, if there is data there you may as well use it - even if you are going to add deltas onto it later...

Crydsch commented 3 years ago

@chriscamacho Don't get me wrong, if there are not many duplicate indexes in a mesh then indexed rendering may not be beneficial.

Actually there is vertex data, just not in a buffer i need to load onto the gpu. I use the vertex shader to generate the vertex data. This is probably a rather special case - true. If you're interested I'll gladly go into more detail on discord ;)

raysan5 commented 3 years ago

After lot of thinking about this functionality, I decided to add a simple GenMeshDefault() that loads by default and empty mesh with the most common vertex attributes: position, texcoord, normal, color. It also uploads the 0-initialized data to GPU (like the other GenMesh*() functions).

Advance users requiring some specific vertex attributes can create the Mesh manually.