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

Line2NodeMaterial: Add opacityNode support #29964

Closed

Conversation

Makio64
Copy link
Contributor

@Makio64 Makio64 commented Nov 25, 2024

Related issue: #29962

Add opacityNode support to line2NodeMaterial

wip : worked almost straightforward but at the intersection of line there is a round corner added
Screenshot 2024-11-25 at 23 13 24

Copy link

github-actions bot commented Nov 25, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.14
78.99
339.14
78.99
+0 B
+0 B
WebGPU 483.51
134.22
483.55
134.23
+40 B
+9 B
WebGPU Nodes 482.98
134.13
483.01
134.13
+40 B
+8 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.62
111.98
464.62
111.98
+0 B
+0 B
WebGPU 552.64
149.58
552.64
149.58
+0 B
+0 B
WebGPU Nodes 508.52
139.29
508.52
139.29
+0 B
+0 B

@Makio64
Copy link
Contributor Author

Makio64 commented Nov 25, 2024

Screenshot 2024-11-25 at 23 23 47

It looks better but we can still see a point between segmentation

@Makio64
Copy link
Contributor Author

Makio64 commented Nov 25, 2024

I had to modify LineGeometry to fix the fact the segment were over each others..

also updated the demo
Screenshot 2024-11-26 at 01 53 51

@Mugen87 let me know what you think of it, i didn't see bad side effects

@sunag sunag changed the title add opacityNode support to line2NodeMaterial Line2NodeMaterial: Add opacityNode support Nov 25, 2024
@sunag sunag added this to the r171 milestone Nov 25, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 25, 2024

Wide line materials do not support alpha because of the resulting artifacts, see #23680.

I'm not familiar enough with the wide line implementation so I hope @WestLangley or @gkjohnson can have a look at this PR.

@WestLangley
Copy link
Collaborator

Please test with wider lines and fewer points so you can see the artifacts. The segmented colors look like a previous issue.

Screenshot 2024-11-25 at 3 12 11 PM

Setting transparent to true, it looks pretty much the same for me. ???
Screenshot 2024-11-25 at 3 18 52 PM

@gkjohnson
Copy link
Collaborator

Setting transparent to true, it looks pretty much the same for me. ???

Without something like a stencil test (see #23680 (comment)) it won't be possible to avoid these artifacts. Even with the pre-depth / stencil test, though, overlapping lines won't display correctly which would be even more noticeable with per-line opacity.

I don't have strong opinions on this other than it won't look right or work "out of the box" in the connected line segments case - this can be useful for users who know how to work around it, though. Separated line segments should look okay, as well. With more control over render order or ability to render an object with two passes in three.js it would be more possible to make a variant of this that "just works" (overlapping aside).

I wonder if something like a compute shader rasterizer or some custom per-segment depth adjustment could help with the artifacts.

@Makio64
Copy link
Contributor Author

Makio64 commented Nov 26, 2024

@WestLangley Edit : My bad it still not working, i didnt see than i remove transparent: true from the material.

More complex approach like suggest by @gkjohnson might be necessary for it ?
Or we can add to the vertex the info about the previous and next point position allowing to create the nice angle in the mesh , similar approach used here : https://github.com/pmndrs/meshline/blob/master/src/MeshLineGeometry.ts

@Makio64
Copy link
Contributor Author

Makio64 commented Nov 26, 2024

I ported https://github.com/pmndrs/meshline/ to nodeMaterial today and it's working great with alpha / uv / map / dash / gradient / etc.., i can clean it and will open a new PR.

Is there a reason why threejs used another approach than this one ?
If i name it MeshLineNodeMaterial / MeshLineGeometry will it be confusing ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 26, 2024

Is there a reason why threejs used another approach than this one ?

This is years ago but originally we wanted to port this implementation, see #11040. We've decided for #11349 though because it was more robust (MeshLine had issues with small point sets) and it was also more optimized because it allocates less memory on the GPU (thanks to instancing). I don't know if the current MeshLine implementation has improved in that regard.

@sunag sunag removed this from the r171 milestone Nov 27, 2024
@sunag sunag closed this Nov 27, 2024
@Mugen87 Mugen87 added this to the r171 milestone Nov 27, 2024
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.

5 participants