Invalidate the lighting bind memo in the instanced draw path - #277
Open
Groseuros wants to merge 3 commits into
Open
Invalidate the lighting bind memo in the instanced draw path#277Groseuros wants to merge 3 commits into
Groseuros wants to merge 3 commits into
Conversation
SceneEncoder._encodeInstanced clears the render pass bindings without the paired EngineLightingUniforms.invalidateBindMemo() that _encode performs at the equivalent site. EngineLightingUniforms memoises its bind set on (pass, shader, lighting, environment) and documents that the encoder invalidates that memo whenever it clears the pass; the instanced path did not, so the memo kept reporting bindings that had just been wiped. In a run of same-pipeline opaque draws only the first instanced draw is therefore fully bound. Every later one — and any non-instanced mesh encoded after it — loses prefiltered_radiance, brdf_lut, shadow_map, sh_coefficients, punctual_lights, ssao_texture and FogInfo, and shades black. The geometry is still submitted and still writes depth, so it reads as missing rather than unlit. A single InstancedMesh escapes this: its clear hits an empty pass and the memo misses anyway on a fresh RenderPass, so everything binds, and a following mesh sharing the pipeline skips its own clear and inherits the live bindings. Two or more instanced meshes is where it starts. Reproduced with cuboid instances split across K PhysicallyBasedMaterial InstancedMeshes plus a ground plane: correct at K=1, and at K>=2 only one type and no ground on Impeller's Android GLES backend. Vulkan and WebGL2 tolerate the missing binds and render correctly, so the defect is latent there rather than absent. Setting a non-zero emissiveFactor — which travels in FragInfo and is re-bound on every draw — makes the missing geometry reappear, confirming the draws execute and only the memoised engine bindings are lost.
Pairs `clearBindings` with the engine-lighting memo invalidation in a single `_clearBindings`, so the instanced path cannot drop the invalidation again. Adds an instanced_lighting smoke scene (two instanced meshes plus a plain mesh sharing a lit pipeline), which reproduced the loss on Impeller GLES and now matches Metal and WebGL2. Also corrects the web shim's clearBindings comment, which claimed to drop uniform and texture bindings it actually leaves bound.
The shim only dropped vertex and index bindings. Uniform blocks and texture units are GL context state, so they stayed attached and a draw that should have rebound a slot silently read the previous draw's resource. That is why the instanced lighting loss rendered correctly on WebGL2 while breaking on GLES. The pass now records the binding points it hands out and releases them on clear, so a missing rebind fails on web the way it does natively.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EngineLightingUniformsmemoises its bind set and documents that "the scene encoder invalidatesthe memo whenever it clears the pass's bindings".
_encodedoes that (scene_encoder.dart:379-380)._encodeInstancedclears at:441without invalidating, so from the second instanced draw onwardthe engine lighting samplers are wiped and never rebound — those draws, and any non-instanced mesh
encoded after them, shade black while still writing depth.
Needs two or more
InstancedMeshes sharing a pipeline. With one, the clear hits an empty pass andthe memo misses anyway, so everything binds — probably why it hasn't come up.
Repro: cuboid instances split across K
InstancedMeshes withPhysicallyBasedMaterial, plus aground-plane
Nodeand oneDirectionalLight. Correct at K=1; at K≥2 only one type renders and theground disappears.
Only visible on the GLES backend. Windows/Vulkan, an Android flagship on Vulkan, and web/WebGL2
all render correctly without the patch, so they are tolerating the missing binds rather than
unaffected — which is likely why this survived, since Android only falls back to GLES where Vulkan
is unavailable. On GLES it also under-reports frame cost, because the unshaded geometry still writes
depth: the same scene measured 3.8 ms raster before the fix and 4.9 ms after.
No output or timing change on Vulkan or WebGL2 after the patch. Cost is ~12 binds per instanced mesh
per frame, scaling with mesh count rather than instance count.
Happy to instead mirror
_encodeexactly and only clear on an actual pipeline change, if you'dprefer that over the minimal fix.
flutter_scene 0.20.0 and current master (
70125a88); Flutter masterbc2162e270.