Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[models] Add GenMeshDefault to improve Mesh generation #1556

Closed
chrisdill opened this issue Jan 29, 2021 · 17 comments
Closed

[models] Add GenMeshDefault to improve Mesh generation #1556

chrisdill opened this issue Jan 29, 2021 · 17 comments
Labels
enhancement This is an improvement of some feature

Comments

@chrisdill
Copy link
Contributor

chrisdill commented Jan 29, 2021

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
Copy link
Contributor

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
Copy link
Owner

raysan5 commented Jan 31, 2021

GenMeshCustom(), I like it!

@chrisdill
Copy link
Contributor Author

chrisdill commented Jan 31, 2021

@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.

@raysan5 raysan5 added the enhancement This is an improvement of some feature label Feb 2, 2021
@chrisdill
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Crydsch commented Feb 21, 2021

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
Copy link
Owner

raysan5 commented Mar 24, 2021

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Owner

raysan5 commented Mar 25, 2021

@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
Copy link
Contributor

Crydsch commented Mar 25, 2021

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
Copy link
Contributor

@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
Copy link
Contributor

Crydsch commented Mar 26, 2021

@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
Copy link
Contributor

@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
Copy link
Contributor

Crydsch commented Mar 26, 2021

@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 added a commit that referenced this issue Apr 2, 2021
@raysan5
Copy link
Owner

raysan5 commented Apr 2, 2021

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.

@raysan5 raysan5 closed this as completed Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an improvement of some feature
Projects
None yet
Development

No branches or pull requests

5 participants