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

Change Camera.viewport_to_world_2d to rescale according to the rendering target's scaling factor #17338

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Suibeom
Copy link

@Suibeom Suibeom commented Jan 13, 2025

Camera.viewport_to_world_2d doesn't behave as expected with the target window is rescaled, or didn't have a 1:1 scale factor in the first place. If the rendering target has enough information for us to get its scaling factor, we should use it to rescale the viewport coordinates before computing normalized device coordinates and then converting to world coordinates. This change should preserve the existing behavior if rendering target information is not available.

Fixes #17312

Solution

Get the target_scaling_factor option from our camera and unwrap_or, with default 1.0, so we will use the scaling factor if it exists. It might be that we want to return an error if this option is None, but it didn't care before, so perhaps the safer change is to default to the previous behavior if we can't rescale in the first place.

Testing

Issue reported included an MRE here:

Issue MRE

This change successfully addresses the undesirable behavior in the MRE

Screenshot 2025-01-12 at 9 05 39 PM

Migration Guide

If you were using Camera.viewport_to_world_2d previously and manually correcting for scaling, it will probably break!

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 ✨

@rparrett
Copy link
Contributor

rparrett commented Jan 13, 2025

This breaks even our own 2d_viewport_to_world example, which is passing logical coordinates from window.cursor_position() to viewport_to_world_2d. I think this method probably should take logical coordinates.

If there's anything to be done about #17312, maybe it's modifying the UI camera projection or whatever so that world coordinates match up with logical coordinates instead of physical coordinates?

@Suibeom Suibeom marked this pull request as draft January 13, 2025 16:50
@IQuick143 IQuick143 added A-Rendering Drawing game state to the screen X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Need to take scale factor into account when transforming Node to world position
3 participants