(Adoped) Remove panics and optimise mesh picking#18232
Merged
alice-i-cecile merged 15 commits intobevyengine:mainfrom Mar 10, 2025
Merged
(Adoped) Remove panics and optimise mesh picking#18232alice-i-cecile merged 15 commits intobevyengine:mainfrom
alice-i-cecile merged 15 commits intobevyengine:mainfrom
Conversation
This reverts the benchmarks back to how they were in bevyengine#17033.
Member
Author
|
cc @tbillington |
alice-i-cecile
approved these changes
Mar 10, 2025
Member
|
Marking as Ready-For-Final-Review due to reviews in previous PR. I agree that the benchmark changes would be nice to land at some point though! |
Member
Author
|
I ran the benchmarks, comparing Benchmark Resultspicking::ray_mesh_intersection::cull_intersect/100_vertices
time: [733.33 ns 734.48 ns 735.69 ns]
change: [-45.485% -45.198% -44.937%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild
picking::ray_mesh_intersection::cull_intersect/10000_vertices
time: [64.099 µs 64.430 µs 65.109 µs]
change: [-46.283% -45.052% -44.117%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
picking::ray_mesh_intersection::cull_intersect/1000000_vertices
time: [6.5265 ms 6.5318 ms 6.5376 ms]
change: [-45.879% -45.756% -45.640%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
picking::ray_mesh_intersection::no_cull_intersect/100_vertices
time: [743.02 ns 746.39 ns 752.14 ns]
change: [-44.471% -44.030% -43.597%] (p = 0.00 < 0.05)
Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
1 (1.00%) high mild
15 (15.00%) high severe
picking::ray_mesh_intersection::no_cull_intersect/10000_vertices
time: [65.276 µs 65.310 µs 65.355 µs]
change: [-44.325% -44.183% -44.044%] (p = 0.00 < 0.05)
Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
7 (7.00%) high mild
7 (7.00%) high severe
picking::ray_mesh_intersection::no_cull_intersect/1000000_vertices
time: [6.6483 ms 6.6560 ms 6.6645 ms]
change: [-45.131% -45.046% -44.959%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
6 (6.00%) high mild
4 (4.00%) high severe
picking::ray_mesh_intersection::cull_no_intersect/100_vertices
time: [343.71 ns 344.55 ns 345.56 ns]
change: [-53.847% -53.671% -53.496%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
1 (1.00%) high mild
12 (12.00%) high severe
picking::ray_mesh_intersection::cull_no_intersect/10000_vertices
time: [37.845 µs 37.862 µs 37.884 µs]
change: [-56.275% -56.124% -55.971%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
3 (3.00%) high mild
5 (5.00%) high severe
picking::ray_mesh_intersection::cull_no_intersect/1000000_vertices
time: [4.0354 ms 4.0382 ms 4.0413 ms]
change: [-56.681% -56.599% -56.523%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe |
IQuick143
reviewed
Mar 25, 2025
|
|
||
| match ray_triangle_intersection(&ray, &tri_vertices, backface_culling) { | ||
| Some(hit) if hit.distance >= 0. && hit.distance < closest_distance => { | ||
| (hit.distance, Some((a, hit))) |
Contributor
There was a problem hiding this comment.
Why are we returning a here, isn't that the index of the first triangle vertex?
IQuick143
reviewed
Mar 25, 2025
|
|
||
| closest_hit | ||
| } | ||
| closest_hit.and_then(|(tri_idx, hit)| { |
Contributor
There was a problem hiding this comment.
But here it's interpreted as a triangle index?
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 25, 2025
…ces. (#18533) # Objective - #18495 ## Solution - The code in the PR #18232 accidentally used a vertex index as a triangle index, causing the wrong triangle to be used for normal computation and if the triangle went out of bounds, it would skip the ray-hit. - Don't do that. ## Testing - Run `cargo run --example mesh_picking`
mockersf
pushed a commit
that referenced
this pull request
Mar 25, 2025
…ces. (#18533) # Objective - #18495 ## Solution - The code in the PR #18232 accidentally used a vertex index as a triangle index, causing the wrong triangle to be used for normal computation and if the triangle went out of bounds, it would skip the ray-hit. - Don't do that. ## Testing - Run `cargo run --example mesh_picking`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note from BD103: this PR was adopted from #16148. The majority of this PR's description is copied from the original.
Objective
Adds tests to cover various mesh picking cases and removes sources of panics.
It should prevent users being able to trigger panics in
bevy_pickingcode via bad mesh data such as #15891, and is a follow up to my comments in #15800 (review).This is motivated by #15979
Testing
Adds 8 new tests to cover
ray_mesh_intersectioncode.Changes from original PR
I reverted the changes to the benchmarks, since that was the largest factor blocking it merging. I'll open a follow-up issue so that those benchmark changes can be implemented.