Conversation
…res() return count
# Conflicts: # vclib/render/include/vclib/bgfx/drawable/drawable_mesh_bgfx.h
# Conflicts: # vclib/render/include/vclib/bgfx/drawable/drawable_mesh_bgfx.h # vclib/render/include/vclib/bgfx/drawable/mesh/mesh_render_buffers.h # vclib/render/include/vclib/bgfx/drawable/uniforms/drawable_mesh_uniforms.h # vclib/render/include/vclib/bgfx/drawable/uniforms/material_uniforms.h
There was a problem hiding this comment.
Pull request overview
This PR refactors the texture binding system for mesh rendering to use dynamic texture stages instead of fixed stage assignments. Previously, textures were bound to predefined stages (VCL_MRB_TEXTURE0-5), but now stages are allocated dynamically as needed and the stage information is encoded in shader uniforms using a packed 32-bit integer (4 bits per texture type).
Changes:
- Introduced dynamic texture stage allocation where texture stages are assigned at runtime and encoded in DrawableMeshUniforms
- Removed texture availability tracking from MaterialUniforms in favor of stage-based availability checks
- Consolidated PBR-related macros from pbr_macros.h into mesh_render_buffers_macros.h
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vclib/render/shaders/vclib/bgfx/pbr_common.sh | Updated include path and formatting for macro definitions |
| vclib/render/shaders/vclib/bgfx/drawable/uniforms/material_uniforms.sh | Removed u_pbr_texture_settings uniform definition |
| vclib/render/shaders/vclib/bgfx/drawable/uniforms/drawable_mesh_texture_uniforms.sh | New file implementing dynamic texture stage functions with bit-field encoding |
| vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/tex_wedge.sh | Updated to use new baseColorTex() function instead of direct texture2D |
| vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/tex_vertex.sh | Updated to use new baseColorTex() function instead of direct texture2D |
| vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/surface_uber_in.sh | Removed SAMPLER2D declarations, now using texture functions from drawable_mesh_texture_uniforms.sh |
| vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/fs_surface_uber_pbr.sc | Updated all texture calls to use new availability checks and texture functions |
| vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/fs_surface_uber.sc | Updated texture calls to use new baseColorTex() function |
| vclib/render/include/vclib/bgfx/drawable/uniforms/material_uniforms.h | Removed texture availability parameter from set() function and related logic |
| vclib/render/include/vclib/bgfx/drawable/uniforms/drawable_mesh_uniforms.h | Added texture stage encoding/decoding functions and TextureType enum |
| vclib/render/include/vclib/bgfx/drawable/mesh/pbr_macros.h | File deleted, macros moved to mesh_render_buffers_macros.h |
| vclib/render/include/vclib/bgfx/drawable/mesh/mesh_render_buffers_macros.h | Consolidated PBR macros, removed fixed texture stage definitions |
| vclib/render/include/vclib/bgfx/drawable/mesh/mesh_render_buffers.h | Implemented dynamic texture binding with stage tracking, changed uniform names |
| vclib/render/include/vclib/bgfx/drawable/drawable_mesh_bgfx.h | Updated to call resetTextureStages() and handle dynamic stage allocation |
| vclib/render/include/vclib/bgfx/drawable/drawable_environment.h | Changed BRDF LUT uniform name from "s_brdf_lut" to "s_tex5" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/tex_wedge.sh
Outdated
Show resolved
Hide resolved
vclib/render/shaders/vclib/bgfx/drawable/drawable_mesh/surface/tex_vertex.sh
Outdated
Show resolved
Hide resolved
vclib/render/shaders/vclib/bgfx/drawable/uniforms/drawable_mesh_texture_uniforms.sh
Outdated
Show resolved
Hide resolved
vclib/render/shaders/vclib/bgfx/drawable/uniforms/drawable_mesh_texture_uniforms.sh
Outdated
Show resolved
Hide resolved
| { | ||
| assert(toUnderlying(type) < toUnderlying(TextureType::COUNT)); | ||
|
|
||
| if (toUnderlying(type) < 8) { // use y |
There was a problem hiding this comment.
Misleading comment: the comment says "use y" but the code uses sMeshData[2], which is the z component, not y. The comment should say "use z" or be removed entirely since it's redundant with the function name setYTextureStage.
| if (toUnderlying(type) < 8) { // use y | |
| if (toUnderlying(type) < 8) { // use z component (sMeshData[2]) |
| for (uint i = 0; i < mMRB.triangleChunksNumber(); ++i) { | ||
| // Bind textures before vertex buffers!! | ||
|
|
||
| /* TEXTURES */ |
There was a problem hiding this comment.
Bug: texture stages need to be reset before binding textures for each chunk. Currently, resetTextureStages is only called once before the loop (line 217), but when different chunks have different materials with different numbers of textures, stale texture stage data from previous chunks will persist. For example, if chunk 0 has 3 textures and chunk 1 has 2 textures, chunk 1 will still have the third texture stage set from chunk 0. Add a call to DrawableMeshUniforms::resetTextureStages() at the beginning of the loop, before line 233 where bindTextures is called.
| /* TEXTURES */ | |
| /* TEXTURES */ | |
| // Reset texture stages for each chunk to avoid stale bindings | |
| DrawableMeshUniforms::resetTextureStages(); |
Textures are not bound to fixed stages anymore when drawing mesh surface.
Stages are used dynamically only when a texture is actually needed.
The stage for each texture type is then encoded on uniforms (packed in DrawableMeshUniforms).