-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
Fix updating some material properties in initMaterial #21020
Conversation
Environment or fog can be changed to a different object so that the shader code used to render them doesn't change. In this case we want to update the values on materialProperties so further redundant initMaterial calls are prevented.
Merging early so we have enough time to test it. |
Thanks! |
// always update environment and fog - changing these trigger an initMaterial call, but it's possible that the program doesn't change | ||
|
||
materialProperties.environment = material.isMeshStandardMaterial ? scene.environment : null; | ||
materialProperties.fog = scene.fog; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't a different fog object detected by getParameters()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what this PR addresses is that even if it's a different object, the object type is the same?
But, honestly, I have not done a deep study on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the fog type is the same and only the object differs, a shader recompilation is not necessary. Uniform updates should handle different fog values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oletus could you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A live example that demonstrates the current issue would also help to clarify the change 😇 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing now that I misunderstood the new comment in WebGLRenderer
, sorry.
The PR is intended to prevent additional calls of initMaterial()
which are not necessary. These might happen since we compare objects in setProgram()
.
A live example would still help to fill the gaps in my mental model 😊 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, your last comment is right. This patch makes sure materialProperties get updated if environment or fog is replaced with a different object of the same type. It could be possible to change webgl_materials_standard example to support changing to a different environment map, would you want me to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oletus That'd be great 👍
Environment or fog can be changed to a different object so that the shader code used to render them doesn't change. In this case we want to update the values on materialProperties so further redundant initMaterial calls are prevented.
This contribution is funded by https://higharc.com/