-
-
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
TSL: Share context between RTT #28811
Conversation
@Mugen87 It should fix the use of |
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
@@ -155,7 +155,8 @@ class GaussianBlurNode extends TempNode { | |||
// | |||
|
|||
const material = this._material || ( this._material = builder.createNodeMaterial() ); | |||
material.fragmentNode = blur(); | |||
material.fragmentNode = blur().context( builder.getSharedContext() ); |
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.
Do you mind explaining why .context( builder.getSharedContext() )
is now used in GaussianBlurNode
(and not just RTTNode
)?
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.
As it is a different render-pass, this shader is compiled in a different NodeBuilder
and will probably not share the same context or the same settings defined in the renderer, any Node that uses the RenderToTexture approach can inherit the context defined in its essence if it is transferred with this approach.
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.
So if this change would be missing in GaussianBlurNode
, renderer settings could get out of sync and produce similar color errors like reported in #28781 (comment)?
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.
So if this change would be missing in GaussianBlurNode, renderer settings could get out of sync and produce similar color errors like reported in #28781 (comment)?
Yes, that's it, but only when a node uses context to obtain parameters like the renderOutput()
case, in other situations it should be ok.
Indeed, colors are now correct! 🙌 |
Related issue: #28781 (comment)
Description
Share context between RTT. There are possibly more nodes to add the
.context( builder.getSharedContext() )