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

Nlines #17160

Closed
wants to merge 13 commits into from
Closed

Nlines #17160

wants to merge 13 commits into from

Conversation

nathanieltagg
Copy link

Hi, this is another attempt to install two associated changes to the examples/lines code.

First, I have put in a simple raycast call on LineSegments2 to allow it to be used. I've added code to the example webgl_lines_fat to show how the raycasting works, by drawing a dot at the intersection.

Second, I've added a new material to draw the lines with, called PerspectiveLineMaterial. This material is like the existing one, but sets up pseudo-perspective and allows the line to narrow as it gets further from the camera. (My application defines a lot of 1d lines like this rather than joined cylinders; this is more efficient for both memory and raycasting).

Please consider incorporating these changes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 2, 2019

Please remove the build files and package-lock.json from your PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 2, 2019

@WestLangley
Copy link
Collaborator

Thanks for this. Raycasting support would be nice.

  1. Please allow world width to be as large as 100 in your demo.

  2. Check your formatting with https://zz85.github.io/mrdoobapproves/.

  3. Also see how pixel-perfect raycasting has been implemented for sprites. Techniques there may be applicable.

Related: #15356. Also #16912.

/ping @gkjohnson

@gkjohnson
Copy link
Collaborator

I'm interested in this, as well. I would think there would need to be two raycast paths that account for screen-space lines and perspective attenuated lines.

For screen-space lines I would think the raycaster._camera and material.resolution fields should be used and the lines can just be treated as tubes in the the perspective attenuated version. I'm not sure that raycaster.linePrecision should be used for in this case considering the lines should have thickness. Or maybe it should be used to "expand" the thickness of the line during raycast?

@WestLangley what point should be returned when raycasting against the lines? In the case of raycasting near the edge of a 100-pixel line should the closest point on the original line be returned (which would not be along the ray itself) or should the point near the edge of the line be returned? I'm thinking the latter behavior is what I would expect.

@WestLangley
Copy link
Collaborator

Right. Ideally, raycaster.linePrecision should not be used.

what point should be returned when raycasting against the lines?

The most accurate one, if possible.

Also, I'd leave the existing examples as-is, and add a separate example for fat-line raycasting. It is easier for users to have single-feature examples.

@nathanieltagg
Copy link
Author

nathanieltagg commented Aug 3, 2019 via email

@nathanieltagg
Copy link
Author

@gkjohnson I'm not sure what you mean by 100 world width? Oh, wait, I think you mean "max line width". You're right, fixed.

@gkjohnson That sounds like an interesting idea, to use the 'world line' width instead of the linePrecision variable. However, I tried implementing, and it doesn't work at all well, for reasons I'm unsure of: it grossly overestimates the collision hitbox. I'm including that code, commented out, in the the commit I've just pushed.

Even if we make it work, it won't always do the right thing, because the min/max line width clamps the visible value. I think we shouldn't let the perfect be the enemy of the good here.

what point should be returned when raycasting against the lines?
The most accurate one, if possible.

I think the question is: should you return the point that is along the raycast line that has the closest approach to the line-object, or should you return a point on the line-object that is closest to the raycast-line? For my money, it should be the latter, since you can always use it to get the former (but not vice-versa).

Personally, I don't care about any of these issues. In my application, I use raycasting for picking the line object in general, and don't care about the intersection point; I just want to highlight the selected line object.

I've added examples/webgl_raycast_line.html as the new example, and removed the raycasting code from the fat lines example.

@gkjohnson
Copy link
Collaborator

The latest example for testing:
https://raw.githack.com/nathanieltagg/three.js/nlines/examples/webgl_raycast_line.html

Even if we make it work, it won't always do the right thing, because the min/max line width clamps the visible value. I think we shouldn't let the perfect be the enemy of the good here.

I think it would be best to simplify the PR to only focus on adding raycasting to the screen-space lines and make sure that is correct instead of adding perspective attenuated lines here, as well. As it is the example doesn't do quite what I expect because when I hover my mouse exactly over the lines the red intersection dot will display in a different spot on a different line segment. It also jumps between line segments instead of smoothly moving across the curve.

This definitely isn't a simple addition and I do feel that the raycasting code should be taking advantage of the Raycaster._camera field (as the sprites raycast does) and the resolution field of the material to ensure it aligns with with visuals produced by the shader.

@nathanieltagg
Copy link
Author

Maybe. But I reiterate that this feature is useful -now-, as I am already using by it. I’m fine with gradual improvement, but the priority should be something functional.

I’m not sure why it’s jumping.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2019

Side note: I've noticed that many devs still use THREE.MeshLine since it provides raycasting support. It's a bit hard to tell users why three.js is unable to provide this feature for its wide line implementation. Especially since this is not the first PR that tries to add it, see #15356

So, to get things forward I suggest to split the PR into two. One PR that implements raycasting and an other one that introduces PerspectiveLineMaterial.

@nathanieltagg Are you willing to do this change? Otherwise I'll put the raycasting stuff on my agenda since it was requested by the community multiple times.

@nathanieltagg
Copy link
Author

I can.. but what's the reason for splitting (i.e. leaving out the perspective material)? I haven't seen a complaint about it, and I'm not sure why it shouldn't be put in...?

After a little break from this, I think I realized what the problem was with using the perspective line width: it was the addition of the scale factor, which is not strictly obeyed. I've put that change in, and so now perspective line materials no longer need the linePrecision set. It's not perfect, but it seems to work pretty well!

I also figured out what was up with the 'jumping' behavior: the algorithm was fine, but the first intersect returned by the raycaster is the nearest intersection, not the best intersection. So, sometimes it would jump to a part of the line closer to the camera but further away from the mouse. I've put in a ad-hoc fix: the intersection points for lines now report a transverseDistance. In the example code, I now have a sort that finds the best transverse match instead of the match closest to the camera.

Also merged with dev to stay up-to-date.

@nathanieltagg
Copy link
Author

By the way, I'm having trouble with the new layout. Am I supposed to be editing the jsm folder, the js folder, or am I supposed to be keeping both in sync?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2019

I thought because of some open issue we might have faster results if we split up the PR. Big PRs are in general hard to review.

Am I supposed to be editing the jsm folder, the js folder, or am I supposed to be keeping both in sync?

You edit the files in the js folder and then run node utils/modularize.js. Sorry for this workflow but we are in a transition period right now.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2019

BTW: Please remove the package-lock.json from your PR.

@nathanieltagg
Copy link
Author

I'm not sure how to split it up into two things, since they're sort of interdependent. The raycast code looks for perspective materials, and shows this in the example. The perspective material could go in first, but not second.

@WestLangley
Copy link
Collaborator

I support adding raycasting for fat lines. It should be pixel-perfect -- just like sprite raycasting. Please include a separate example to demonstrate the feature. I suggest doing that first.

I do not support adding perspective size attenuation to fat lines.

But it is @mrdoob's project, so he will have to decide if he wants fat lines with perspective or prefers to support something like MeshLine. In the perspective case, the line width will have to be in world units and shadows should be supported.

@nathanieltagg
Copy link
Author

@WestLangley:

I support adding raycasting for fat lines. It should be pixel-perfect

I don't know how to get it pixel-perfect. Furthermore, I use raycasting in my project and would find it an impediment: asking users to put their mouse over the exact pixel in a 'fat' line that is 2 pixels wide would not be good UI design for me.

I do not support adding perspective size attenuation to fat lines.

Because...? Again, I am using this right now and it's useful. I have very complex 3d line objects I need to parse and represent in 3D on the client side, and creating tube structures in javascript is computationally intensive. (E.g. several dozen 'track' like objects, each of which consists of about 2000 3-D points defining the track.) On the other hand, the fat-line structure, which is a brilliant use of iterated objects BTW, does it in the shader and therefore is much cheaper. I'm not suggesting it get used by everyone, just saying I want it as an optional material, and I suspect others might have similar needs.

I understand that my perspectives are not universal, but I'm actually using these in a real project, which means that n=1 people find it useful, and I think n=0 people would find it makes things worse.

I found the fatline solution to be a really good hack, and I really appreciate your work bringing it,
but let's not let perfect be the enemy of the good.

@WestLangley
Copy link
Collaborator

@nathanieltagg

I just expressed my opinion. If others find MeshLine unsatisfactory and think fat lines should have perspective support, and if @mrdoob wants it, then it will happen.

@nathanieltagg
Copy link
Author

I never said it was unsatisfactory - it's great! I love it! Thanks for your work on it! I'm just saying that I find this stuff useful, and I think others will too.

@WestLangley
Copy link
Collaborator

@nathanieltagg I think you misunderstood my statement. THREE.MeshLine is an alternative approach and was not by me.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 18, 2019

Raycasting support was implemented via #17872.

LineMaterial still does not support perspective size attenuation though. Closing this PR in favor of #16912.

@Mugen87 Mugen87 closed this Dec 18, 2019
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.

4 participants