Skip to content

Removed conversion from pointer physical coordinates to viewport local coordinates in bevy_picking make_ray function #18870

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 27, 2025
Merged

Removed conversion from pointer physical coordinates to viewport local coordinates in bevy_picking make_ray function #18870

merged 2 commits into from
Apr 27, 2025

Conversation

KrzysztofZywiecki
Copy link
Contributor

Objective

Solution

After PR #17633, Camera::viewport_to_world method corrects viewport_position passed in that input so that it's offset by camera's viewport. Camera::viewport_to_world is used by make_ray function which in turn also offsets pointer position by viewport position, which causes picking objects to be shifted by viewport position, and it wasn't removed in the aforementioned PR. This second offsetting in make_ray was removed.

Testing

  • I tested simple_picking example by applying some horizontal offset to camera's viewport.
  • I tested my application that displayed a single rectangle with picking on two cameras arranged in a row. When using local bevy with this fix, both cameras can be used for picking correctly.
  • I modified split_screen example: I added observer to ground plane that changes color on hover, and removed UI as it interfered with picking both on master and my branch. On master, only top left camera was triggering the observer, and on my branch all cameras could change plane's color on hover.
  • I added viewport offset to mesh_picking, with my changes it works correctly, while on master picking ray is shifted.
  • Sprite picking with viewport offset doesn't work both on master and on this branch.

These are the only scenarios I tested. I think other picking functions that use this function should be tested but I couldn't track more uses of it.

Removed conversion of pointer position from physical to logical posit…
7911ae9 
…ion. It's handled in camera.viewport_to_world
@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added this to the 0.16.1 milestone Apr 17, 2025
@Henauxg Henauxg added C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Picking Pointing at and selecting objects of all sorts labels Apr 23, 2025
@mockersf
Copy link
Member

this should also be tested on a screen that doesn't have a scale of 1

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 26, 2025
@KrzysztofZywiecki
Copy link
Contributor Author

I tried changing my display scale to 50%, 100%, 150%, 200%, and testing again with simple_picking, split_screen, and mesh_picking both on master and after my changes. In all those cases after my fix picking worked predictably while master didn't work with splitscreen/viewport offset as described.

Is setting display scale a good way to test this? I was printing out window.scale_factor, and for ex. 200% scale I got scale factor of 2 so I assumed it will be enough.

@mockersf
Copy link
Member

sounds good, thanks!

@mockersf mockersf added this pull request to the merge queue Apr 27, 2025
Merged via the queue into bevyengine:main with commit 7f04906 Apr 27, 2025
32 checks passed
@NthTensor NthTensor modified the milestones: 0.16.1, 0.17 Apr 27, 2025
@atlv24 atlv24 mentioned this pull request May 11, 2025
Closed
jf908 pushed a commit to jf908/bevy that referenced this pull request May 12, 2025
@KrzysztofZywiecki @jf908
Removed conversion from pointer physical coordinates to viewport loca…
73508d0 
…l coordinates in bevy_picking make_ray function (bevyengine#18870)

# Objective

- Fixes bevyengine#18856.

## Solution

After PR bevyengine#17633, `Camera::viewport_to_world` method corrects
`viewport_position` passed in that input so that it's offset by camera's
viewport. `Camera::viewport_to_world` is used by `make_ray` function
which in turn also offsets pointer position by viewport position, which
causes picking objects to be shifted by viewport position, and it wasn't
removed in the aforementioned PR. This second offsetting in `make_ray`
was removed.

## Testing

- I tested simple_picking example by applying some horizontal offset to
camera's viewport.
- I tested my application that displayed a single rectangle with picking
on two cameras arranged in a row. When using local bevy with this fix,
both cameras can be used for picking correctly.
- I modified split_screen example: I added observer to ground plane that
changes color on hover, and removed UI as it interfered with picking
both on master and my branch. On master, only top left camera was
triggering the observer, and on my branch all cameras could change
plane's color on hover.
- I added viewport offset to mesh_picking, with my changes it works
correctly, while on master picking ray is shifted.
- Sprite picking with viewport offset doesn't work both on master and on
this branch.

These are the only scenarios I tested. I think other picking functions
that use this function should be tested but I couldn't track more uses
of it.

Co-authored-by: Krzysztof Zywiecki <krzysiu@pop-os.Dlink>
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
@KrzysztofZywiecki @andrewzhurov
Removed conversion from pointer physical coordinates to viewport loca…
f979a5a 
…l coordinates in bevy_picking make_ray function (bevyengine#18870)

# Objective

- Fixes bevyengine#18856.

## Solution

After PR bevyengine#17633, `Camera::viewport_to_world` method corrects
`viewport_position` passed in that input so that it's offset by camera's
viewport. `Camera::viewport_to_world` is used by `make_ray` function
which in turn also offsets pointer position by viewport position, which
causes picking objects to be shifted by viewport position, and it wasn't
removed in the aforementioned PR. This second offsetting in `make_ray`
was removed.

## Testing

- I tested simple_picking example by applying some horizontal offset to
camera's viewport.
- I tested my application that displayed a single rectangle with picking
on two cameras arranged in a row. When using local bevy with this fix,
both cameras can be used for picking correctly.
- I modified split_screen example: I added observer to ground plane that
changes color on hover, and removed UI as it interfered with picking
both on master and my branch. On master, only top left camera was
triggering the observer, and on my branch all cameras could change
plane's color on hover.
- I added viewport offset to mesh_picking, with my changes it works
correctly, while on master picking ray is shifted.
- Sprite picking with viewport offset doesn't work both on master and on
this branch.

These are the only scenarios I tested. I think other picking functions
that use this function should be tested but I couldn't track more uses
of it.

Co-authored-by: Krzysztof Zywiecki <krzysiu@pop-os.Dlink>
StrikeForceZero pushed a commit to StrikeForceZero/bevy that referenced this pull request May 29, 2025
@KrzysztofZywiecki @StrikeForceZero
Removed conversion from pointer physical coordinates to viewport loca…
bbe0bdf 
…l coordinates in bevy_picking make_ray function (bevyengine#18870)

# Objective

- Fixes bevyengine#18856.

## Solution

After PR bevyengine#17633, `Camera::viewport_to_world` method corrects
`viewport_position` passed in that input so that it's offset by camera's
viewport. `Camera::viewport_to_world` is used by `make_ray` function
which in turn also offsets pointer position by viewport position, which
causes picking objects to be shifted by viewport position, and it wasn't
removed in the aforementioned PR. This second offsetting in `make_ray`
was removed.

## Testing

- I tested simple_picking example by applying some horizontal offset to
camera's viewport.
- I tested my application that displayed a single rectangle with picking
on two cameras arranged in a row. When using local bevy with this fix,
both cameras can be used for picking correctly.
- I modified split_screen example: I added observer to ground plane that
changes color on hover, and removed UI as it interfered with picking
both on master and my branch. On master, only top left camera was
triggering the observer, and on my branch all cameras could change
plane's color on hover.
- I added viewport offset to mesh_picking, with my changes it works
correctly, while on master picking ray is shifted.
- Sprite picking with viewport offset doesn't work both on master and on
this branch.

These are the only scenarios I tested. I think other picking functions
that use this function should be tested but I couldn't track more uses
of it.

Co-authored-by: Krzysztof Zywiecki <krzysiu@pop-os.Dlink>
mockersf pushed a commit that referenced this pull request May 30, 2025
@KrzysztofZywiecki @mockersf
Removed conversion from pointer physical coordinates to viewport loca…
b4ee898 
…l coordinates in bevy_picking make_ray function (#18870)

# Objective

- Fixes #18856.

## Solution

After PR #17633, `Camera::viewport_to_world` method corrects
`viewport_position` passed in that input so that it's offset by camera's
viewport. `Camera::viewport_to_world` is used by `make_ray` function
which in turn also offsets pointer position by viewport position, which
causes picking objects to be shifted by viewport position, and it wasn't
removed in the aforementioned PR. This second offsetting in `make_ray`
was removed.

## Testing

- I tested simple_picking example by applying some horizontal offset to
camera's viewport.
- I tested my application that displayed a single rectangle with picking
on two cameras arranged in a row. When using local bevy with this fix,
both cameras can be used for picking correctly.
- I modified split_screen example: I added observer to ground plane that
changes color on hover, and removed UI as it interfered with picking
both on master and my branch. On master, only top left camera was
triggering the observer, and on my branch all cameras could change
plane's color on hover.
- I added viewport offset to mesh_picking, with my changes it works
correctly, while on master picking ray is shifted.
- Sprite picking with viewport offset doesn't work both on master and on
this branch.

These are the only scenarios I tested. I think other picking functions
that use this function should be tested but I couldn't track more uses
of it.

Co-authored-by: Krzysztof Zywiecki <krzysiu@pop-os.Dlink>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@Henauxg Henauxg Henauxg approved these changes

Assignees
No one assigned
Labels
A-Picking Pointing at and selecting objects of all sorts C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
0.16.1
Development

Successfully merging this pull request may close these issues.

Mesh picking offset when the camera's viewport physical_position is non-zero
5 participants
@KrzysztofZywiecki @mockersf @Henauxg @alice-i-cecile @NthTensor