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

Nodes: Allow JS-like assigns, implement AssignNode #26795

Merged
merged 14 commits into from
Oct 8, 2023

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Sep 18, 2023

Alternative to #26785

Description

Implements SplitNode.assign() and ArrayElementNode.assign(), also adds a separate class for AssignNode.

Same cases of use are supported:

let vec = ...;

vec = vec.w.assign( 1 );
vec = vec.xy.assign( 1 );
vec = vec.xy.assign( vec2( 1, 2 ) );
vec = vec.z.assign( vec.w );

Following use cases are also supported:

let nodeArr = ...;

nodeArr = nodeArr[ 0 ].assign( 1 );

The following use cases for VarNodes are supported as a bonus:

const variable = temp( ... );

variable.w = 1;
variable.xy = 1;
variable.xy = vec2( 1, 2 );
variable.z = variable.w;

Caching issue

Something is quite wrong with its caching in the case of Background:

nodeVar0 = ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) );
nodeVar0.z = ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) ).w;

But it also was before:

nodeVar0 = vec4<f32>( ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) ).x, ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) ).y, ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) ).w, ( ( NodeUniforms.projectionMatrix * NodeUniforms.modelViewMatrix ) * vec4<f32>( nodeVarying1, 1.0 ) ).w );

So I just fixed it in Background manually by adding .temp() -- but if using the "bonus" use case the caching issue still appears.

Adds support for JS-like assigns:

const variable = someNode;

variable.w = 1;
variable.xy = 1;
variable.xy = vec2( 1, 2 );
variable.z = variable.w;

Signed-off-by: Levi Pesin <[email protected]>
@sunag
Copy link
Collaborator

sunag commented Sep 18, 2023

The code below cannot return the same vector, see #26493 (comment), #26493 (comment)

vec = vec.xy.assign( 1 );

--

The code below is good!

variable.xy = vec2( 1, 2 );

Is it possible to you simplify this PR to support just this?

@sunag
Copy link
Collaborator

sunag commented Sep 18, 2023

Would this be compatible with stack.if?

@sunag
Copy link
Collaborator

sunag commented Sep 18, 2023

It can be confusing for the user to try something like this and it doesn't work properly:

const variable = temp( ... );

stack.if( bool, ( stack ) => {

	variable.x = 123;

} );

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Sep 18, 2023

The code below cannot return the same vector

Yeah, I understand now that it looks quite confusing... Maybe we could say that the "main" approach is to do vec.abc = something and the other one is also supported, but don't use it in examples? Like in the same way we have two approach for nodes, node elements (preferred) and "raw" node classes (not really preferred, but still supported).

Would this be compatible with stack.if?

I think the problem here is in VarNode.assign()... It currently works in "compile-time" (JS code), maybe we should change its behavior to work in "run-time" (shader code) so that it would be more in line with other methods.

@sunag
Copy link
Collaborator

sunag commented Sep 18, 2023

I don't think we should support this syntax vec = vec.xy.assign( 1 ), if we could shorten the code it would be better.

@LeviPesin
Copy link
Contributor Author

I don't really see how to support the "bonus" syntax without supporting this one...

@LeviPesin
Copy link
Contributor Author

I'm afraid with the latest changes to VarNode.assign() the "bonus" syntax can no longer work... The var = var.z.assign( var.w ) still works, though.
@sunag Is there any way we could enable the bonus syntax to work again? I don't really see such...

@sunag
Copy link
Collaborator

sunag commented Oct 5, 2023

There is an approach that could work, although we would have to find a good way to do it. This would be having a global stack reference, like currentStack, if we make this work we can have nodes like assign/if/else/loop without needing to use stack.assign or stack.if for example, so we can do everything without having to use stack property. I've had this in mind for some time but haven't tested it yet.

@@ -59,8 +59,11 @@ class Background extends DataMap {
getSamplerLevelNode: () => backgroundBlurriness
} ).mul( backgroundIntensity );

let viewProj = modelViewProjection();
viewProj = viewProj.setZ( viewProj.w );
const viewProj = tslFn( () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wouldn't work normally, because currentStack is null when this is ran -- so I've wrapped it in a some kind of "Node IIFE".

@LeviPesin LeviPesin changed the title Implement SplitNode.assign() and ArrayElementNode.assign() Nodes: Allow JS-like assigns, implement AssignNode Oct 8, 2023
@LeviPesin
Copy link
Contributor Author

LeviPesin commented Oct 8, 2023

@sunag This PR should work now. It has also became much simpler with the latest changes and also now non-VarNodes should be also supported. (There is a one minor issue with possibility of currentStack being null, but this can be overcame by using "Node IIFEs")

(AssignNode can be moved to its own PR if needed)

Also, not sure whether we need SetNode now? We can now write variable.xy = anything or anything like that and it will work.

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

sunag commented Oct 8, 2023

Thanks!

@sunag
Copy link
Collaborator

sunag commented Oct 8, 2023

I think SetNode will still be useful, for inline code and visual editors.

@sunag
Copy link
Collaborator

sunag commented Oct 8, 2023

If Proxy would support operators like +, *, - it would be complete :)

@sunag
Copy link
Collaborator

sunag commented Oct 8, 2023

It looks like this PR broke webgpu_compute_particles

@sunag
Copy link
Collaborator

sunag commented Oct 8, 2023

It looks like this PR broke webgpu_compute_particles

Don't worry about it, I'll publish a PR to review, Great work!

@LeviPesin LeviPesin deleted the split-node-assign branch October 8, 2023 19:55
@LeviPesin
Copy link
Contributor Author

If Proxy would support operators like +, *, - it would be complete :)

I think JS doesn't allow overriding such operators yet ☹️

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.

2 participants