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

Simplified webgl_lines_fat_raycasting.html #25191

Merged
merged 2 commits into from
Dec 24, 2022

Conversation

WestLangley
Copy link
Collaborator

The demo seemed unnecessarily complex to me, given its objective.

  • Removed the inset div. It is sufficient for the inset to be included in only webgl_lines_fat.html.

  • Reduced the rotation speed. Spinning demos only serve to obfuscate bugs, so this was a compromise.

  • General clean up.

@WestLangley WestLangley added this to the r149 milestone Dec 24, 2022
@WestLangley
Copy link
Collaborator Author

WestLangley commented Dec 24, 2022

This is the original demo. It appears buggy to me when the threshold is increased.

Dec-23-2022 22-32-51

Maybe the intersection is occurring on the edge of the threshold, rather than on the closest point within the range of the threshold?

/ping @bergden-resonai
/ping @gkjohnson

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 24, 2022

Maybe the intersection is occurring on the edge of the threshold, rather than on the closest point within the range of the threshold?

Would you like to clarify this issue before merging? Or is your PR ready to go?

@WestLangley
Copy link
Collaborator Author

Ready to go, please. :-)

@Mugen87 Mugen87 merged commit 6751104 into mrdoob:dev Dec 24, 2022
@WestLangley WestLangley deleted the dev-fat_lines_raycast branch December 24, 2022 20:23
@gkjohnson
Copy link
Collaborator

Maybe the intersection is occurring on the edge of the threshold, rather than on the closest point within the range of the threshold?

I think this is an artifact of the point the demo is displaying and the behavior of this kind of threshold testing. The intersection is being checked with expanded threshold width but point displayed is the closest point on the center line. So when the model rotates and a new line comes into the foreground - the first-interscted line changes and the displayed point "jumps" to the closest point on the line.

I think that's the issue you're referring to, at least?

@WestLangley
Copy link
Collaborator Author

I am not a fan of spinning demos like this. The demo has been buggy and no-one knew it.

Either the display is incorrect, or the algorithm is incorrect.

I would vote to remove the unnecessary animation loop.

@WestLangley
Copy link
Collaborator Author

cursor location is the green dot

line
Screenshot 2022-12-26 at 1 44 47 PM

segments
Screenshot 2022-12-26 at 1 42 02 PM

@bergden-resonai
Copy link
Contributor

bergden-resonai commented Dec 26, 2022

IIUC, both the display and the animation are correct
We interpret the lines as capsules (both in rendering and intersection testing)
This approach is a very bad approximation for corners or in general lines which are very close (in the depth element this adds to the lines)
This approximation looks and is much worse when the threshold is much greater than the width (or a 1D line)

The animation was added very recently to expose potential bugs related to transformations - and I think it also increased the visibility of this issue, so I'm not sure I understand the downside of this in terms of improving the raycasting code
(The obvious alternative being just applying some other arbitrary transformation one-time)

I can't find a reference to how this is implemented elsewhere, I'm not a big fan of the capsules, but it makes corners very simple to handle which is fun
Maybe we can consider moving to 2D capsules? @gkjohnson

@gkjohnson
Copy link
Collaborator

I am not a fan of spinning demos like this. The demo has been buggy and no-one knew it.

Is the spinning not the quality that made this behavior more apparent?

Either the display is incorrect, or the algorithm is incorrect.

The same behavior occurs with the line object in core...

image

(red is returned point, green is cursor)

If you can specify the behavior that you want that's fine but the again the "right" thing to do for the general case is not obvious. The threshold parameters are designed to make line selection easier. Simply selecting the line that is closest in screenspace is one solution but would lead to odd "popping" and ambiguous behavior when trying to select a close foreground with a distant background line oriented perpendicularly behind it.

Maybe we can consider moving to 2D capsules? @gkjohnson

I'm not sure what you mean by this. 2D capsules are used for the screenspace thick lines - which do not work for lines with perspective attenuation.

@WestLangley
Copy link
Collaborator Author

The obvious alternative being just applying some other arbitrary transformation one-time

Yes, I would have been happier with a single transform applied once, instead of the animation addition. The test line could even be commented out.

// mesh.rotation.y = 1; // test

TBH, rather than modifying an example to demonstrate that a former bug no longer exists, I think adding a new test case to the unit tests is the best option. (But it is true, only the core has unit tests.)

@WestLangley
Copy link
Collaborator Author

Perhaps the "segments" image above is correct in 3D -- it just appears incorrect in 2D...

But how to explain the "line" image above?

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