Skip to content

Conversation

@shotamatsuda
Copy link
Contributor

@shotamatsuda shotamatsuda commented Nov 15, 2025

Description

This PR adds velocityNodeSource field to TRAANode to specify the node that needs the original projection matrix.

As context, I have a HighpVelocityNode that multiplies the model and view matrices on the CPU, because in large coordinates such as meter-scale ECEF, the builtin VelocityNode cannot be used as a reliable velocity source. Following the discussion in #30955, I'm fine with dealing with such use cases in user code, but without changes like in this PR it's not possible to use TRAANode in that setup.

I'm not fully sure about the name velocityNodeSource, and also there could be a better way to tell the source of velocity textures in other places. If that's the case, I'm perfectly fine with using that approach instead.

@shotamatsuda shotamatsuda marked this pull request as ready for review November 15, 2025 05:05
@sunag
Copy link
Collaborator

sunag commented Nov 15, 2025

I'm not fully sure about the name velocityNodeSource, and also there could be a better way to tell the source of velocity textures in other places. If that's the case, I'm perfectly fine with using that approach instead.

I would prefer to contextualize the velocity, so we can group and also isolate the operation for any node that uses it. But the existing code probably needs refactoring in the setProjectionMatrix() function.

@shotamatsuda
Copy link
Contributor Author

shotamatsuda commented Nov 15, 2025

I agree. Would the post-processing context (context.postProcessing) be a good place for it, if you already have an idea?

@sunag
Copy link
Collaborator

sunag commented Nov 15, 2025

Would the post-processing context (context.postProcessing) be a good place for it, if you already have an idea?

I'll need to do some tests to be sure.

Coincidentally, I was working in a global context; This should also help. It's part of this PR, but I'll separate it.

@shotamatsuda
Copy link
Contributor Author

That looks great. Does it replace the builder's shared context? It doesn't seem to do anything right now.
Anyway, storing shared nodes in some form of context seems more future proof. I will keep your PR in mind.

@shotamatsuda shotamatsuda marked this pull request as draft November 15, 2025 08:17
@sunag
Copy link
Collaborator

sunag commented Nov 15, 2025

I think we can try do it this way

  • Make velocityNodeSource a _velocitySourceNode. By making it a node instead of an interface and keeping it private, users will be able to extend the VelocityNode class to make specific changes.
  • Assign this._velocitySourceNode = builder.context.velocity at setup time.

Developers could currently use this:

// There is ambiguity in the term "velocity" here, but this practice should be replaced by the global context.
traaPass = traa( beautyNode, depthNode, velocityNode, camera ).context( { velocity: customVelocity } );

After the global context, it will be able to be defined at the root, so that any other node can auto use the velocity node.

// alt 1
renderer.globalContext = globalContext( {
	velocity: customVelocity 
} );

// possibly: alt 2
renderer.globalContext.setValue( 'velocity', customVelocity ); 

The default velocity node should reflect the node defined in the context when assembling the code in the setup.

@shotamatsuda
Copy link
Contributor Author

Thank you for your response. Wouldn't it just be an indirect way of assigning it to tell the velocity source via context()? If the global context hopefully won't take too long, it might be better to wait for it.

Also, I would suggest storing the unjittered projection matrix in the global context (this isn't possible in the current per-material context) instead of the velocity node in that case, because some post-processing nodes will also need that matrix to unjitter the applied offset.

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