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

Examples: Fix raycasting in webgl_lines_fat_raycasting example. #23690

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Mar 9, 2022

Related issue: #23358 (comment)

Description

Adjust where the line material resolution is set so the raycast result is correct. Previously the material resolution was being set after the raycast occurred meaning the line width was incorrect during the raycast.

BEFORE
https://raw.githack.com/mrdoob/three.js/f35120c9083ce31f1994575cef075357bc98f53d/examples/webgl_lines_fat_raycasting.html

AFTER
https://raw.githack.com/mrdoob/three.js/fix-fat-lines-raycast/examples/webgl_lines_fat_raycasting.html

cc @bergden-resonai

@mrdoob mrdoob added this to the r139 milestone Mar 9, 2022
@bergden-resonai
Copy link
Contributor

I'm not sure I understand this fix - the resolution is set on the frame rate, so assuming the window size didn't change why won't this be identical to the previous code up to the 1st frame?

Either way awesome! seems like it's working well now, crossing this off my todo list :)

Note:
I think it's better for the URL to point to a fixed version instead of the dev version otherwise the PR link will be wrong after the merge
https://raw.githack.com/mrdoob/three.js/f35120c9083ce31f1994575cef075357bc98f53d/examples/webgl_lines_fat_raycasting.html

@gkjohnson
Copy link
Collaborator Author

I'm not sure I understand this fix - the resolution is set on the frame rate

The render resolution on the material is set twice -- once for the main render and once for the subframe render. It needs to be set for the main render size again before checking intersections otherwise it's left at the previous subframe state.

@mrdoob
Copy link
Owner

mrdoob commented Mar 10, 2022

Do we need the subframe in this example? 🤔

@WestLangley
Copy link
Collaborator

Do we need the subframe in this example?

I had the same concern: #23358 (comment)

@gkjohnson
Copy link
Collaborator Author

I don't have strong opinions about whether or not it should be in the example. I can remove it if you want.

@bergden-resonai
Copy link
Contributor

Sorry to bring this up again, but I still suspect a bug with the raycasting+worldwidth lines+camera near combo

Screen.Recording.2022-03-10.at.18.51.50.mov

I should've opened a ticket after the merge to dev - without this example it will be harder to debug and reproduce
camera = new THREE.PerspectiveCamera( 40, window.innerWidth / window.innerHeight, 10, 1000 );

(or even just the rendering)
image

Partially discussed here:
#23358 (comment)

Can't really formulate my issue at the moment - I'll need more time to isolate what I think is buggy here
Having 2 cameras is somewhat helpful

Also:
Now that I think about it there might be a bug in the example where the visualization of the threshold is incorrect for the mini canvas, the line width should be adjusted there according to the main canvas

@mrdoob mrdoob modified the milestones: r139, r140 Mar 24, 2022
@gkjohnson
Copy link
Collaborator Author

@mrdoob aside from whether the subframe should be in the example I think this should be merged. Right now the example looks broken. If we'd like to update it to remove the picture-in-picture we can do that in another PR.

@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob modified the milestones: r141, r142 May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@Mugen87 Mugen87 merged commit f07ec20 into dev Oct 31, 2022
@Mugen87 Mugen87 deleted the fix-fat-lines-raycast branch October 31, 2022 13:15
@Mugen87 Mugen87 changed the title Fat Line Raycast Example: Set the resolution in the correct spot Examples: Fix raycasting in webgl_lines_fat_raycasting example. Oct 31, 2022
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