WebGPURenderer: Fix background #26493
Conversation
| const nodeMaterial = new MeshBasicNodeMaterial(); | ||
| nodeMaterial.colorNode = this.boxMeshNode; | ||
| let viewProj = modelViewProjection(); | ||
| viewProj = vec4( viewProj.x, viewProj.y, viewProj.w, viewProj.w ); |
There was a problem hiding this comment.
Maybe just viewProj.z.assign( viewProj.w )?
There was a problem hiding this comment.
If I were to go with the flow I would say that this should return z, because the last property accessed would still be z, I think that to return the entire vector we should have a function directly on the vector without using property accessors, this can confuse the flow of reading, for example I think viewProj.setZ( viewProj.w ) sounds better in this case.
There was a problem hiding this comment.
viewProj.setZ( viewProj.w ) reads much better 👌
There was a problem hiding this comment.
If I were to go with the flow I would say that this should return z, because the last property accessed would still be z, I think that to return the entire vector we should have a function directly on the vector without using property accessors,
I think we discussed this issue a few months ago and agreed that just adding SplitNode.assign() will allow to write vec.x.assign( anything ).
There was a problem hiding this comment.
If we split the line, we will see that there is something very strange with viewProj.z.assign( viewProj.w ):
const viewProjZ = viewProj.z;
// I think this should return z property not a vec3, so it cannot be inline as suggested
viewProjZ.assign( viewProj.w ) We don't have the same problem with node.set*() because it has no property accessor.
There was a problem hiding this comment.
nove.set*() also follows the current JS API style.
There was a problem hiding this comment.
But what I'm suggesting is: 1) more powerful (you can do anything in all combinations, like vec.xy.assign( anything )), 2) better reflecting shader code, 3) easier to understand, 4) easier to implement. I can file a PR for it if needed.
There was a problem hiding this comment.
So only good things?
You can't think of problems that it creates?
There was a problem hiding this comment.
It is totally possible made node.setXY() or any combination using Proxy, I always considered that when I use node.set*().
There was a problem hiding this comment.
You can't think of problems that it creates?
I don't really see what problems could it create. It would function similar to VarNode.assign().
Related issue: Fixes #26478
Description
NodeMaterial.vertexNode