[render_gl] Remove light limit (and streamline shaders) - #24953
[render_gl] Remove light limit (and streamline shaders)#24953SeanCurtis-TRI wants to merge 1 commit into
Conversation
The primary benefit is the removal of the hard-coded light limit. Attendant
with that, we streamlined the shaders.
- We no longer encode an "empty light" slot. The shader knows exactly how
many lights it has.
- The transformation of geometry vertices from the "model" frame to the
device clip space used to use two matrices (provided as uniforms). The
decomposition wasn't really helpful as we multipled the matrix for
every vertex in the geometry. Instead, we now pre-compute on the CPU
and pass it once per object.
|
+(release notes: fix) +a:@sherm1 for feature review, please. |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and tests consistently remove the limit; only minor documentation corrections remain.
Pull request overview
Removes RenderEngineGl’s five-light limit and streamlines vertex transformations.
Changes:
- Specializes lighting shaders for the configured light count.
- Precomputes model-to-device matrices on the CPU.
- Updates documentation and tests for more than five lights.
File summaries
| File | Description |
|---|---|
geometry/render_vtk/test/internal_render_engine_vtk_test.cc |
Updates multi-light test commentary. |
geometry/render_gl/test/internal_shader_program_test.cc |
Tests combined model-to-device matrices. |
geometry/render_gl/test/internal_render_engine_gl_test.cc |
Verifies rendering with six lights. |
geometry/render_gl/render_engine_gl_params.h |
Removes the documented five-light limit. |
geometry/render_gl/internal_shader_program.h |
Documents the combined transform API. |
geometry/render_gl/internal_shader_program.cc |
Composes and uploads model-to-device matrices. |
geometry/render_gl/internal_render_engine_gl.h |
Updates clone lighting restoration API. |
geometry/render_gl/internal_render_engine_gl.cc |
Generates count-specialized shaders and removes the limit. |
bindings/generated_docstrings/geometry_render_gl.h |
Updates generated light documentation. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - `T_CglCPhysical`: the fixed transform between Drake's physical camera frame | ||
| OpenGL's camera frame. The two cameras have different conventions. This is |
| /* Stores the `T_DM`, the OpenGL model-to-device matrix. Allows derived | ||
| shader programs to do any other instance- and camera-dependent | ||
| configuration). |
sherm1
left a comment
There was a problem hiding this comment.
Feature pending a few minor things. Copilot also requested a few changes.
@sherm1 reviewed 9 files and all commit messages, and made 6 comments.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers (waiting on SeanCurtis-TRI).
geometry/render_gl/internal_render_engine_gl.cc line 124 at r1 (raw file):
layout(location = 0) in vec3 p_MV; layout(location = 1) in vec3 n_M; uniform mat4 T_DM;
BTW consider a line comment here as for the other transforms
geometry/render_gl/internal_render_engine_gl.cc line 244 at r1 (raw file):
exposure = GetDirectionalExposure(light, nhat_W); } if (exposure <= 0.0) return vec3(0.0, 0.0, 0.0);
BTW this depends on exposure having been initialized <= 0 by the float exposure declaration above. Is that guaranteed by the language? If so, consider a comment to that effect for your amateur readers.
geometry/render_gl/internal_shader_program.h line 72 at r1 (raw file):
- We define `T_DCphysical = T_DCgl * T_CglCPhysical`. This is a constant for the camera's intrinsics and is what is *internally* stored as a result of calling SetProjectionMatrix().
BTW consider reordering these bullets to match the order in the equation above. I foundmyself reading along the equation and having to repeatedly search for the next term.
(BTW I love the notation!)
geometry/render_gl/test/internal_render_engine_gl_test.cc line 2902 at r1 (raw file):
{.type = "spot", .intensity = 0.25 * 0.5, .cone_angle = 45}, {.type = "directional", .intensity = 0.25 * 0.5}, {.type = "directional", .intensity = 0.25 * 0.5}},
BTW consider saying why we deem 6 lights to be enough to demonstrate non-limit.
geometry/render_gl/test/internal_shader_program_test.cc line 441 at r1 (raw file):
(Matrix4f() << 1, 0, 0, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0, 0, 0, 1) .finished(); const Matrix4f expected_T_DM = T_DC * X_CglC * X_CW * T_WM;
nit: the notation fell down here. Looks like "C" is used for two different things in the same expression. T_DC is apparently actually T_DCgl. Then the "C" in X_CglC is something else. This should be fixed by renaming something, or at least a comment saying something like "read T_DC as T_DCgl in the following expression".
The primary benefit is the removal of the hard-coded light limit. Attendant with that, we streamlined the shaders.
This change is