Skip to content

Fix mesh_picking not working due to mixing vertex and triangle indices.#18533

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
IQuick143:mesh_picking_fix
Mar 25, 2025
Merged

Fix mesh_picking not working due to mixing vertex and triangle indices.#18533
alice-i-cecile merged 1 commit intobevyengine:mainfrom
IQuick143:mesh_picking_fix

Conversation

@IQuick143
Copy link
Copy Markdown
Contributor

Objective

Solution

Testing

  • Run cargo run --example mesh_picking

@IQuick143 IQuick143 added C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Picking Pointing at and selecting objects of all sorts labels Mar 25, 2025
@IQuick143 IQuick143 added this to the 0.16 milestone Mar 25, 2025
Copy link
Copy Markdown
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the example and this fixes the problem.

It would be nice to have a test that triggers it, this code already has a bunch of tests but apparently none of them caught this issue.

@IQuick143
Copy link
Copy Markdown
Contributor Author

Yeah I agree, but personally idk what the test should be.
Maybe something that tests the normals on a sphere? (Those are easy to calculate and were wrong before I fixed them.)

Copy link
Copy Markdown
Contributor

@tbillington tbillington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I did add more tests, obviously not enough though 😅

@IQuick143 IQuick143 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 25, 2025
@ElDiddi
Copy link
Copy Markdown

ElDiddi commented Mar 25, 2025

This fix also worked for me on Linux. (openSUSE Tumbleweed, wayland)

@kristoff3r
Copy link
Copy Markdown
Contributor

Yeah I agree, but personally idk what the test should be. Maybe something that tests the normals on a sphere? (Those are easy to calculate and were wrong before I fixed them.)

Yeah something like that, just the simplest possible thing that will error on main but not with this. Bonus points if it can be manually validated, but for a regression test even a hardcoded, known good example is fine.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2025
Merged via the queue into bevyengine:main with commit db923d6 Mar 25, 2025
38 checks passed
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Picking Pointing at and selecting objects of all sorts C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants