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: Some refactoring and clean up #27512

Closed
wants to merge 1 commit into from

Conversation

LeviPesin
Copy link
Contributor

Description

Refactoring including removing some no longer needed NodeBuilder methods, some nodes and node builders clean up (e.g. using node chaining more, simplifying some code paths, fixing some methods signatures to be more consistent, cleaning up the flows algorithm), and fixes of a few potential and current bugs (including cases when a TempNode, which was used more than once was generated more than once, or when it was possible to generate something like mix( abc, def, ghi ).xyz = somethingElse -- both these issues could happen in a quite a number of cases -- it's still not perfect now, but hopefully should become so with another PR unifying Node and TempNode). Also this cleans up the resulting shader code in quite a lot of cases (mostly by adding NodeBuilder.formatOperation() method; the refactoring of TextureNode also helps).

This PR mostly only cleans the nodes/ directory, not touching renderers or node examples (they can be cleaned up in a separate PR).

Unfortunately there seems to be a VERY strange bug with normal maps, of which I have no idea what it is caused by. You can see it as a "blockiness" on the fibers ball in the webgpu_clearcoat example.
If you replace the code for the fibers ball with the following it becomes easier to see:

const normalMap5 = textureLoader.load( 'textures/uv_grid_opengl.jpg' );
normalMap5.wrapS = THREE.RepeatWrapping;
normalMap5.wrapT = THREE.RepeatWrapping;
normalMap5.repeat.x = 10;
normalMap5.repeat.y = 10;

material = new THREE.MeshStandardMaterial( {
	normalMap: normalMap
} );
mesh = new THREE.Mesh( geometry, material );
mesh.position.x = 1;
mesh.position.y = 1;
group.add( mesh );

material = new THREE.MeshStandardMaterial( {
	map: normalMap5
} );
mesh = new THREE.Mesh( geometry, material );
mesh.position.x = 1;
mesh.position.y = 3;
group.add( mesh );

material = new THREE.MeshStandardMaterial( {
	map: normalMap
} );
mesh = new THREE.Mesh( geometry, material );
mesh.position.x = 3;
mesh.position.y = 1;
group.add( mesh );

material = new THREE.MeshStandardMaterial( {
	normalMap: normalMap5
} );
mesh = new THREE.Mesh( geometry, material );
mesh.position.x = 3;
mesh.position.y = 3;
group.add( mesh );

You can see that the first ball has a blocky-like map (instead of the correct "meridians"-like) and the last ball has just a flat map, whilst it should be visible as having large cavities.
The most interesting part is that (at least on my machine) if you try to move the camera (by move+rotate or zooming in) so that only one of these two balls is visible, then somewhere along that path both balls magically autofixes (this happens in 100% of time). Even more interestingly, if two other new balls are removed -- this no longer happens, the bug still occurs and no longer fixes.
I have no idea why this happens -- but I'm very sure this is not a bug in this PR but rather maybe some WebGPU bug -- I checked the original and the new vertex and fragment shaders and they are 100% the same in terms of functionality.

The PR is tested on all WGSL examples that were at the moment of its creation and all GLSL examples that worked at the moment of its creation.

(I've spent about 2.5 months working on this PR... Sorry that it has so much changes combined together, I will try to split it into smaller PRs if requested -- or will just rebase it on the latest dev if it's OK to merge as this is)

Signed-off-by: Levi Pesin <35454228+LeviPesin@users.noreply.github.com>
@sunag
Copy link
Collaborator

sunag commented Jan 5, 2024

(I've spent about 2.5 months working on this PR... Sorry that it has so much changes combined together, I will try to split it into smaller PRs if requested -- or will just rebase it on the latest dev if it's OK to merge as this is)

Hi @LeviPesin, your PR looks very interesting! I think the best way would be to separate it into small PRs 👍

@LeviPesin
Copy link
Contributor Author

So I tried to list all of the changes I made in this PR and it seems like it will take more than 50 smaller PRs 😅 I will try to prepare them soon.

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.

None yet

3 participants