-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
WebGLNodes: Ensure onBuild()
uses the correct material reference.
#27097
Conversation
@@ -8,21 +8,23 @@ export const nodeFrame = new NodeFrame(); | |||
|
|||
Material.prototype.onBuild = function ( object, parameters, renderer ) { | |||
|
|||
if ( Array.isArray( object.material ) ) { | |||
const material = this; |
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.
@sunag I don't think this fix is compatible with #26841 though. When onBuild()
is called in WebGLRenderer
, the material can't be an array of materials so using const material = this;
and then Array.isArray( object.material )
will always evaluate to false
. Any ideas how to avoid breakage of multi materials?
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.
If we can't fix this in WebGLNodes
, I'll revert and add this to the renderer:
if ( scene.overrideMaterial === null ) material.onBuild( object, parameters, _this );
That means however Scene.overrideMaterial
can't be of type node material. But at least #27095 will be fixed.
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 it would be good to fix this in WebGLNodes
somehow since the assumption materials can be derived from the object
reference isn't right. I've realized this does not only break overrideMaterial
but shadow map rendering as well (since objects with node materials are rendered with depth/distance materials).
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 added a remaining reference here, it looks ok to me now using overrideMaterial
.
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 have simplified the method a bit. The fiddle from #26811 (comment) still works and #27095 is also fixed. Hope, it looks good!
overrideMaterial
.onBuild()
uses the correct material reference.
* WebGLNodes: Fix usage with `overrideMaterial`. * follow the material choice by the renderer * WebGLNodes: Simpify `onBuild()`. --------- Co-authored-by: sunag <[email protected]>
Fixed #27095.
Description
This PR ensures
WebGLNodes
can process renderings with an activeoverrideMaterial
.The fix is actually simple.
Material.onBuild()
does not extract the material from the 3D object anymore but usesthis
instead.