refactor(particles): drop redundant material reset on camera change#8928
Merged
Conversation
The particle emitter re-ran resetMaterial() (via onChangeCamera()) every time the scene's active camera changed. That made sense when onChangeCamera() also regenerated the shader and the shader was built eagerly from camera state (wrong variant compiled before a camera was known). Since #6804 moved particle shader creation to on-demand getShaderVariant() — keyed on the rendering camera's output gamma/tonemapping at draw time — regenShader() was removed, but the now-pointless resetMaterial() call was left behind. resetMaterial() only re-uploads camera-independent uniforms, so re-running it on camera change is wasted work (and fires every frame under multiple/alternating cameras). The active camera is still cached for the CPU distance sorter, which is its only consumer. Remove the onChangeCamera() indirection.
Build size reportThis PR changes the size of the minified bundles.
|
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
1 participant
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.
Removes a vestigial per-frame material reset in the particle emitter that no longer does anything useful.
The emitter cached the scene's active camera in
addTime()and, on any change, calledonChangeCamera()→resetMaterial(). This was meaningful whenonChangeCamera()also ranregenShader()and the particle shader was built eagerly from camera state (the wrong variant could compile before a camera was assigned — see issue #6700). Since #6804 moved particle shader creation to on-demandgetShaderVariant(), keyed on the rendering camera's output gamma/tonemapping at draw time,regenShader()was removed — but the now-pointlessresetMaterial()call was left behind.resetMaterial()only re-uploads camera-independent uniforms, so re-running it whenever the active camera changes is wasted work, and it fires every frame when multiple/alternating cameras render the same emitter.Changes:
onChangeCamera()→resetMaterial()call from the per-frame camera-tracking inaddTime(); the active camera is still cached for the CPU distance sorter (its only consumer).onChangeCamera()method.resetMaterial()remains as the one-time setup inrebuild().No behavior change: the only consumer of the cached camera is the CPU sorter (assignment preserved); screen-facing billboarding uses the rendering camera in-shader; and output gamma/tonemapping is still handled by
getShaderVariant().Performance: