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

Respect viewport position in coordinate conversion functions #17633

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

Conversation

Novakasa
Copy link
Contributor

@Novakasa Novakasa commented Feb 1, 2025

Objective

Solution

  • This PR now takes the viewport position into account in the functions mentioned above.
  • Extended 2d_viewport_to_world example to test the functions with a dynamic viewport position and size, camera positions and zoom levels. It is probably worth discussing whether to change the example, add a new one or just completely skip touching the examples.

Testing

Used the modified example to test the functions with dynamic camera transform as well as dynamic viewport size and position.

@Novakasa Novakasa force-pushed the viewport-to-word-fix branch from b7e8a03 to 47772ea Compare February 1, 2025 14:24
@Novakasa Novakasa changed the title Respect viewport position in camera coordinate conversion functions Respect viewport position in coordinate conversion functions Feb 1, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Math Fundamental domain-agnostic mathematical operations D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 2, 2025
@alice-i-cecile
Copy link
Member

Migration Guide

Not sure if this does this need a migration guide entry, since this can probably be considered a bugfix.

This PR changes the result of the functions Camera::viewport_to_world_2d, Camera::viewport_to_world, Camera::world_to_viewport and Camera::world_to_viewport_with_depth in the case when the viewport position is not UVec2::ZERO. With a viewport position of UVec2::ZERO, the behaviour is not changed.

Good migration guide, but yes, bug fix :)

@Novakasa Novakasa force-pushed the viewport-to-word-fix branch from 47772ea to 6e5febb Compare February 2, 2025 19:32
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to update the example to make sure this doesn't regress. The example looks a little funny on macOS, likely due to window scale factor, although seems to be functional. I'd suggest testing with scale factor = 2.0 to emulate macOS.

image

@Novakasa Novakasa force-pushed the viewport-to-word-fix branch from 6e5febb to 9ba38ac Compare February 3, 2025 13:23
@Novakasa Novakasa force-pushed the viewport-to-word-fix branch from 9ba38ac to 9bd7b58 Compare February 3, 2025 13:36
@Novakasa
Copy link
Contributor Author

Novakasa commented Feb 3, 2025

Polished the example quite a bit

  • Initialize viewport relative to physical window size (should hopefully fix MacOS weirdness)
  • Controls now in FixedUpdate and respect fixed delta time
  • Simplified viewport bounds visualization with a background mesh
  • Draw cursor after Transform propagation to get rid of camera control lag
  • Add comments

@Novakasa
Copy link
Contributor Author

Novakasa commented Feb 3, 2025

@tychedelia would appreciate another test of the example, since I think the scale_override behaviour on Windows might be bit different than MacOS defaults.

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Looks great and even more improved! Thanks.

@tychedelia tychedelia requested a review from mockersf February 3, 2025 18:18
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Camera::viewport_to_world_2d doesn't take into account the viewport position
4 participants