-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
Added linewidth support #11349
Added linewidth support #11349
Conversation
Wow, crazy sweet! Really good idea on approaching this problem. |
Cool! This implementation solves some problems of the original meshline approach 👍. And thanks to geometry instancing, it is definitely more performant! The goal of the original PR was to implement the extended |
@WestLangley BTW: Do you found out what problem causes the wrong rendering mentioned in #11040 (comment)? |
That is the goal here, too. The use of instancing means the three.js classes/API may have to be refactored. That is why I created new classes in the demo as a first step toward thinking about the redesign -- if any.
The demo is meant to show that Also, although it may be fun to play with large values of |
I have not. But I think the mitering approach is flawed. Line edges have to be parallel in 2D, and that means that mitered joints intersecting at very acute angles would have outside edges that intersect at near infinity. That is why I opted for the rounded end caps and avoided mitering altogether. |
I see. Anyway, this PR is a huge step forward so it's better to close #11040 in favor of this one 😅 . |
How long will this take to land? Could you make an npm module? |
What about performance? This can't possibly be the same as gl.LINE?
Why are code comments a super big no no and frowned upon in three.js? Just by looking at file names i can't tell what they do. When looking at code, you really need to read everything and understand. I think it would be helpful to have something like
|
vec2 offset = vec2( dir.y, - dir.x ); | ||
|
||
// undo aspect ratio adjustment | ||
dir.x /= aspect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you do aspectInv = 1/aspect
outside the shader? Probably better to do two multiplications than two divisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uAspect: { value: new THREE.Vector2( actualAspect , invAspect )}
to avoid computing resX/resY (division) for every vert if you need both */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will keep these optimizations in mind for later.
* parameters = { | ||
* color: <hex>, | ||
* linewidth: <float>, | ||
* resolution: <Vector2>, // to be set by renderer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be vec4 ? (x,y,1/x,1/y)
Beautiful work! |
We should prioritize this PR so it lands in the core as fast as possible.
Um, i'm not sure anymore if having two solutions and switching if |
@Mugen87 what's the perf of |
^ +1 I wouldn't go too crazy about replacing gl lines with this. I havent had a chance to dig into the code but i assume it's at least twice as many vertices to process in order to draw the mesh, but i assume it's actually more - if you need the dir of a line to be known from the vertex, you need to transform other vertices for each vertex. I took this for a spin, for a limited test with just an array of few verts it worked flawlessly, but integrating it into a more complex project failed, i started crashing webgl. |
@pailhead could you provide an example where this crashes WebGL as you say? |
It was an user error i thought i was creating too many lines, but i didn't pass the right material. I accidentally passed the line geometry instead. Dunno if you can recreate it:
Something rendered, but it was really slow and i kept getting "rats...". These work great, would love to post some screenshots but need to find a suitable model that i can show. Round joints are the way to go, mitering (didnt know it was called like that) is just flawed. I'm just worried that rendering all this for a very thin line is a bit overkill. Why not just make it optional? It might be worth considering how people are using three nowadays, someone may just opt to import these lines from either src or examples and use them as is. |
@hccampos I don't have any performance comparisons so far. But replacing I've tested the implementation on various devices today. The visual results are consistent and really impressive! I hope i have some time for simple performance tests in the next days. |
@Mugen87 sounds like a plan. Another to look into would be dynamically changing lines. When using |
If anyone is interested checking this out, i thought it would possibly squeeze a couple more frames but not a huge improvement, should probably make sure that it using the right shader. Original: Some attempted optimizations: I mostly moved the resolution and aspect stuff outside the shader, and compute/provide inverses through uniforms. I also multiplied p and mv once before multiplying the vecs. I dont think i set the test right but i think it may speed it up a tad. But more importantly, if you hit space you should switch to gl lines, and it's much much faster. As much as i love love LOVE ❤️ them, i'm pretty surprised if you really want to just straight up replace native lines with this approach. Even more so if you want to do it for lines that are 1px wide. I experimented with the resolution setter. I wanted to give and return a vec2 but i wanted to compute inverses for the uniforms as a sideffect. While failing at this, i'm not sure what the shader was getting (i did have a I'm interested if there are better ways to test this, i'm not entirely sure what is going on, i tried to disable the frustum culling in order to have a fixed number of verts rendering all the time, but i see it gets much faster if you zoom in, slower if you zoom out. |
@pailhead it seems likely that your changes caused crazy geometry to be produced, and that the huge slowdowns in rendering were what caused your Windows machine to hit Timeout Detection and Recovery (resetting the graphics card). But if you make a small self-contained test case with that issue, we'll be happy to take a look. Note that there are already a bunch of tests which crash graphics drivers in https://github.com/KhronosGroup/WebGL/tree/master/sdk/tests/extra , so see whether your case is covered by one of them (very expensive shader, very expensive geometry, ...). @WestLangley this is awesome work and I think it should be the default for Three.js's lines. A fallback to using native lines could be offered, but I think they'll have to be emulated on more and more platforms to provide useful features like wide lines (which has always been necessary on Windows, and is now necessary on macOS since most browsers have switched to using the Core Profile to implement WebGL). |
Could you explain more what "crazy" means exactly in this case? The easiest way to reproduce what i think i encountered is to open the console on the demo link and paste:
It's the same amount of stuff, but it's being rendered on top of one another. Frame rate goes from 60 to 2 when zoomed in, 10ish when zoomed out. It's the same amount of stuff being rendered, and actually the edit adding a few more draw calls with this geo and wrong material will crash it:
|
Would this be an opportunity to add morphTargets support to lines (either as part of this PR or after it's merged?). #2875 - it's one of the longest standing open issues on three.js and something I ran into recently. |
@pailhead something unexpected is probably going on with the shader or the geometry. It's causing the GPU to take much longer than expected to render, causing it to reset. This is an expected failure mode, especially during development. If this happens then Chrome shuts down rendering of WebGL content for safety. I'm sorry but I don't have time to debug this particular test case in depth. If you would like to reduce it and add it to the WebGL conformance suite, you can follow the pattern of other tests in e.g. https://github.com/KhronosGroup/WebGL/tree/master/sdk/tests/conformance/misc and put up a pull request. There are already some manually-run tests which cause GPU resets in https://github.com/KhronosGroup/WebGL/tree/master/sdk/tests/extra . |
I think you could reduce overhead further for scenes with multiple LineSegmentsGeometry objects, by sharing the position, uvs and index BufferAttributes between all instances? On second thoughts, you probably need reference counting on buffer attributes for that to be safe. |
I think it'll be good to do some testing to see how this compares to |
Can we close this issue now and refer to the new entities 😇 ? #10357 |
I will still hold it for a bit... We could consider making this the default path for |
@mrdoob After merging, the inset viewport in the flatlines demo is not in the intended place. We need to revisit that. |
@WestLangley Okay, I'll take a look during r92 👌 |
I'm hugely in favor of maintaining support for @WestLangley if you're in favor as well, I'd be happy to submit a PR with updated names and/or respective documentation changes and additions to help move this along 😄 I'm pumped to see this making it's way into the repo! Great work. And just to reiterate support, I'm working on a project that can display hundreds of thousands individual lines, so If the community is in favor of the new types as well, I'm happy to help with the documentation at the very least! |
@XanderLuciano I expect we will wait a few release cycles before moving this into core, but a performance study would be helpful -- or suggestions for improvement. Try to break it. :) |
@WestLangley I'm having having some trouble adding new verts/colors to a LineSegmentsGeometry buffer and having them render properly and was hoping you could give me a pointer. Here is the code I'm using:
` I've had success updating the position or color of thicklines within existing buffers by setting those flags but this is the first time I've attempted to add additional verts to these buffers and have not managed to get it working yet. Any advice you can give would be greatly appreciated! |
@Bsheridan12 You can't resize buffers. Allocate larger buffers initially, and then set line.geometry.maxInstancedCount = value; to limit the number of segments that are rendered. |
@WestLangley Cool thanks for the pointer, I was able to get it working with that. Looks like assigning
After updating the positions and colors with the new set of verts/colors and flagging the buffers for update did the trick. Thanks again! |
// main scene | ||
|
||
// renderer will set this eventually | ||
line.material.resolution.set( window.innerWidth, window.innerHeight ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noob question. How's that spite/THREE.MeshLine doesn't require this manual step? Are these solutions fundamentally different? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ping @spite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also have to specify the resolution on THREE.MeshLine (https://github.com/spite/THREE.MeshLine/blob/master/src/THREE.MeshLine.js#L379), I don't think they're different in that regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolution is required because the line width is in pixels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks guys. I used a line width of 0.003 when using THREE.MeshLine and thought "it just works". It assumes a resolution of 1,1 - now I understand.
@WestLangley I am a newer to Shader. I took a look the implementation, but I didn't understand. Could you please explain a little your algorithm including "position" and "uv" attributes in LineSegmentsGeometry? Thanks! |
I noticed that same line looks wider when you dolly out (zoom out), because everything is smaller, yet the line renders at constant width. You can see an example on the attached images. I realized fat lines don't work with raycaster, so I'll have to go back to THREE.MeshLine ^ zoomed in ^ zoomed far out, same line, looks wider (constant pixel width though) |
@mkarnicki This is intended behavior.
|
@clineyuan This PR is using instancing. The If you need further help, you can try the forum. |
What's the intent for using these with |
@WestLangley I am trying to use fat lines in a project that before would use BufferGeometry and LineSegments, but it needs groups to distribute different materials, like so: let geom = new THREE.LineSegments()
geom.geometry = new THREE.BufferGeometry()
geom.geometry.groups = [{ start: 0, count: 2, materialIndex: 0 }, { start: 2, count: 2, materialIndex: 1 }]
const vertices = new Float32Array([0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 1, 0])
geom.geometry.addAttribute('position', new THREE.BufferAttribute(vertices, 3))
geom.material = [new THREE.LineBasicMaterial({ color: 'red' }), new THREE.LineBasicMaterial({ color: 'blue' })]
view.scene.add(geom) Is there any way to do this with fat lines? Everything we've tried failed so far and no idea where to look atm. |
@mkarnicki It not work when I use THREE.Mesh with raycaster. Could you please give me some simple code about using THREE.MeshLine with raycaster ? Thx ! |
Related #15356 |
This PR adds support for "fat" lines. The lines are rendered using instancing, and line segments are rendered in screen space with rounded ends.
LineSegmentsGeometry
is a new class which extendsInstancedBufferGeometry
.Regardless of the complexity of the line, only 6 faces are pushed to the GPU. The additional data pushed to the GPU is an array of start-end xyz-pairs -- similar to how line segments are specified currently. Instancing makes implementation straight-forward.
LineMaterial
is also added. It renders lines in screen-space. The relevant parameter islineWidth
, which is specified in pixels. A rendered line segment looks like this.The difficult part in implementing
LineMaterial
was getting the screen-space rendering correct. But once this was done, segments chain together quite nicely.I did not implement perspective size attenuation. With that feature, lines close to the camera can be as wide as the screen, and they look terrible. IMHO,
linewidth
should be specified in pixels -- not in world units -- so perspective size attenuation does not make sense.Line-joins are round. I think this is preferable to mitering the line-joins; miters are problematic when line segments are nearly parallel.
I have stubbed out several new classes. We can see if they make sense. Due to name conflicts, I added "2" to some of the names for now. This entire API is up for debate.
New classes:
New geometries:
New material:
THREE.LineMaterial
has not been "wired-in" as a built-in material yet.In the demo, I have added an inset viewport to study the effects of the
resolution
parameter and the effects of antialiasing.edit: live demo removed