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

[rlgl] Implement vertex normals for RLGL immediate drawing mode #3866

Merged
merged 5 commits into from
Apr 23, 2024

Conversation

bohonghuang
Copy link
Contributor

Before this PR, rlNormal3f was not functional because rlVertexBuffer had no buffer for normals. This PR implements the addition of this buffer and binds it to the default render batch, allowing it to be used in custom shaders. A simplified version of the basic lighting example is provided as a demonstration, which uses DrawCube and DrawPlane functions instead of DrawModel.

basic_light_with_draw_cube_plane

@bohonghuang bohonghuang force-pushed the immediate-normal branch 3 times, most recently from e71ae3b to 3acbe2d Compare March 12, 2024 08:44
@raysan5
Copy link
Owner

raysan5 commented Mar 26, 2024

@bohonghuang Thanks for the addition. I'm still considering it. It adds performance cost and I don't know if many raylib users could benefit of vertex normals, considering that RenderBatch is mostly used for 2D (shapes/textures) that do not define normals.

@bohonghuang
Copy link
Contributor Author

@bohonghuang Thanks for the addition. I'm still considering it. It adds performance cost and I don't know if many raylib users could benefit of vertex normals, considering that RenderBatch is mostly used for 2D (shapes/textures) that do not define normals.

I'm not sure how many people have the same needs as me, which is to dynamically generate and modify vertices in real-time without managing context, which is really convenient and one of the main reasons I love Raylib.
However, from the perspective of the completeness of RLGL's features and the consistency of Raylib's API, I think it is necessary. For example, if a user wants to experiment with their own lighting shader, they must load a mesh instead of using the built-in shape drawing functions in rmodels. I think there might be an API disconnect here.
As you mentioned, this does indeed add some overheads. I think this is mainly reflected in:

  1. The memory and GPU memory occupied by the buffer allocated for normals in rlLoadRenderBatch.

    batch.vertexBuffer[i].normals = (float *)RL_MALLOC(bufferElements*3*4*sizeof(float));

    I believe this can be negligible because most Raylib applications only use the default render batch, so the normal buffer will only be allocated once.

  2. The I/O waiting caused by uploading normal data to the GPU in rlDrawRenderBatch.

    glBindBuffer(GL_ARRAY_BUFFER, batch->vertexBuffer[batch->currentBuffer].vboId[2]);
    glBufferSubData(GL_ARRAY_BUFFER, 0, RLGL.State.vertexCounter*3*sizeof(float), batch->vertexBuffer[batch->currentBuffer].normals);

    This should be the main source of overhead, but since light shading is handled by the user-provided shader, it can be optimized with a simple check (forgive me for not doing this earlier). This means that scenarios that do not use normals (like 2D rendering as you mentioned) will not introduce this overhead, because the default shader has no vertexNormal attribute:

    if (RLGL.State.currentShaderLocs[RL_SHADER_LOC_VERTEX_NORMAL] != -1)
    {
        glBindBuffer(GL_ARRAY_BUFFER, batch->vertexBuffer[batch->currentBuffer].vboId[2]);
        glBufferSubData(GL_ARRAY_BUFFER, 0, RLGL.State.vertexCounter*3*sizeof(float), batch->vertexBuffer[batch->currentBuffer].normals);
    }
  3. The increase in normal buffer data in rlVertex3f.

    // Add current normal
    RLGL.currentBatch->vertexBuffer[RLGL.currentBatch->currentBuffer].normals[3*RLGL.State.vertexCounter] = RLGL.State.normalx;
    RLGL.currentBatch->vertexBuffer[RLGL.currentBatch->currentBuffer].normals[3*RLGL.State.vertexCounter + 1] = RLGL.State.normaly;
    RLGL.currentBatch->vertexBuffer[RLGL.currentBatch->currentBuffer].normals[3*RLGL.State.vertexCounter + 2] = RLGL.State.normalz;

    It is worth noting that the transformation of normals by the transformation matrix occurs in rlNormal3f, and the overhead added by rlVertex3f is actually very small.

In my tests, these overheads are really small in applications where I heavily use the RLGL immediate mode, unless there is a very large number of vertices being drawn, which could potentially show I/O bottlenecks. However, I don't think this kind of extreme case belongs to the use case of RLGL immediate mode, as the overhead caused by other immediate APIs could also be significant.

screenrec002

@raysan5 raysan5 changed the title Implement vertex normals for RLGL immediate drawing mode [rlgl] Implement vertex normals for RLGL immediate drawing mode Mar 31, 2024
@raysan5
Copy link
Owner

raysan5 commented Apr 5, 2024

@bohonghuang Thank you very much for the detailed explanation. I agree with your points.

@raysan5
Copy link
Owner

raysan5 commented Apr 5, 2024

@bohonghuang Could it be added a custom #define RLGL_SUPPORT_BATCH_NORMALS 1 in config.h to allow users enable/disable this feature if not desired? it will also be useful for performance testing.

@bohonghuang
Copy link
Contributor Author

@bohonghuang Could it be added a custom #define RLGL_SUPPORT_BATCH_NORMALS 1 in config.h to allow users enable/disable this feature if not desired?

This is certainly feasible, but I have a few questions:

  1. Is it necessary? The introduced overhead is neglectable, so the impact of this change won't be felt if the feature is not used.
  2. If this configuration option is introduced, should we apply conditional compilation to every modified location or only in the locations where the overhead may be introduced?
  3. Should this compilation option be enabled by default or disabled?

@raysan5
Copy link
Owner

raysan5 commented Apr 5, 2024

@bohonghuang Your questions are legit. I would enable it by default and just let disable it for special cases where it could impact performance... but let me think about it a bit more before adding that flag...

@bohonghuang
Copy link
Contributor Author

@raysan5 Is there anything I can do to help the progress of this PR?

@raysan5
Copy link
Owner

raysan5 commented Apr 17, 2024

@raysan5 Is there anything I can do to help the progress of this PR?

Thanks but I'm afraid I need some more time to think about it before merging, I'm working on other things at the moment.

@raysan5 raysan5 merged commit d80febd into raysan5:master Apr 23, 2024
14 checks passed
@raysan5
Copy link
Owner

raysan5 commented Apr 23, 2024

@bohonghuang I'm merging this change, thanks for the implementation!

@bohonghuang
Copy link
Contributor Author

bohonghuang commented Apr 23, 2024

@bohonghuang I'm merging this change, thanks for the implementation!

@raysan5 With pleasure. Thanks for your time and effort in reviewing and merging this PR as well, which saves me the effort of maintaining a separate branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants