-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Mesh: Prevent infinite loop in raycast(). #22068
Conversation
I'm afraid that's not. When defining groups, you normally define a discrete range (which is not open up). Do you mind explaining in more detail how your group data are defined? You can modify the following fiddle (multimaterial setup with raycasting) for demonstration: https://jsfiddle.net/d7gjxqsm/ |
Other patterns are supported, and used: #12135 (comment) |
I'm aware of that but it is not the common case^^. |
The example from @WestLangley is exactly what I was doing. I added multiple groups from 0 - Infinity so that I could apply multiple materials to a single mesh. If you raycast at that mesh, your app will hang. I defer to @Mugen87 regarding how common it is. :) I can modify the fiddle later today if it is still useful. |
Here is a fiddle to show the problem. It is basically the pattern from @WestLangley applied to the fiddle from @Mugen87 . In order to enable the bug, uncomment the raycaster.intersectObjects() line near the bottom. For me, this puts my browser in a really unhappy state. |
After all I'm not super happy we the entire groups/multi material implementation. Hopefully we can left it out in |
@mrdoob This PR is good to go. |
Thanks! |
Description
If you raycast at a Mesh with multiple materials, you end up in an infinite loop if both group.count and drawRange.count are set to Infinity. (Which seems to me to be a common case.) The single material cases check against the minimum of index.count or position.count (depending on if the Geometry is indexed or not). It seems to me that this same check should be included in the array of materials case as well.