Skip to content
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

Refactor VarNode #26821

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Refactor VarNode #26821

merged 7 commits into from
Oct 2, 2023

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Sep 22, 2023

Fixes #26820

Also helps #26795

Description
Refactor VarNode (also move some defaults from VarNode.generate() to NodeBuilder.getVarFromNode()). Also remove replaceNode argument from Node.traverse() callback signature.

Currently this (more specifically, lighting) does not work -- I think it is so because the nodes from lighting context are referencing nodes from the builder's stack which have not been built to that time yet. I tried to fix this by explicitly building the stack in ContextNode, but it still didn't work, and I don't really understand stacks and contexts theme deep enough to debug it... @sunag Can you please help with this?

@LeviPesin LeviPesin marked this pull request as draft September 22, 2023 11:15
@sunag
Copy link
Collaborator

sunag commented Sep 22, 2023

We need a ShaderCallNode like happen in FunctionCallNode, it will need to store the call arguments and fire only in the construct process, in this way we can pass the StackNode.

@LeviPesin
Copy link
Contributor Author

Can you please add it to this PR?

@sunag
Copy link
Collaborator

sunag commented Sep 22, 2023

Can you please add it to this PR?

I've already started developing it, as soon as it's ready I'll publish it, I'm doing it in a new PR to make it easier.

@LeviPesin
Copy link
Contributor Author

@sunag Can you please tell how to fix this PR with the new change?

@sunag
Copy link
Collaborator

sunag commented Sep 26, 2023

There are more changes to be made here, as soon as possible I will publish a new PR.

@@ -42,7 +42,7 @@ class HemisphereLightNode extends AnalyticLightNode {

const irradiance = mix( groundColorNode, colorNode, hemiDiffuseWeight );

builder.context.irradiance.addAssign( irradiance );
builder.stack.addAssign( builder.context.irradiance, irradiance );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunag I think you missed this in your PR?

@LeviPesin LeviPesin marked this pull request as ready for review October 2, 2023 07:20
@LeviPesin
Copy link
Contributor Author

@sunag Rebased this PR for the latest changes (most changes from this PR were incorporated into yours, but not all), it should work now.

@sunag sunag added this to the r158 milestone Oct 2, 2023
@sunag sunag merged commit 4451660 into mrdoob:dev Oct 2, 2023
17 checks passed
@sunag
Copy link
Collaborator

sunag commented Oct 2, 2023

Thanks!

@LeviPesin LeviPesin deleted the refactor-varnode-assign branch October 2, 2023 16:58
@Methuselah96 Methuselah96 mentioned this pull request Jan 18, 2024
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebGPU Node system extremely slow startup
2 participants