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

Cleanup consistency of position types in bevy_window #5004

Closed
Weibye opened this issue Jun 13, 2022 · 2 comments
Closed

Cleanup consistency of position types in bevy_window #5004

Weibye opened this issue Jun 13, 2022 · 2 comments
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@Weibye
Copy link
Contributor

Weibye commented Jun 13, 2022

What problem does this solve or what need does it fill?

When interacting with windows in bevy and positions thereof, bevy_window presents the user with quite a variety of units without too much clarity or direction on why:

// From WindowCommands
resolution: (u32, u32), // window resolution
logical_resolution: (f32, f32), // also window resolution
position: IVec2, // window position
position: Vec2, // cursor position
// From Window
physical_cursor_position: Option<DVec2>, // cursor position

Winit behind the scenes works in either Logical or Physical units for sizes and positions.

  • physical meaning "physical pixels", usually in the form of u32
  • logical meaning "logical pixels" which are f32 / f64 values and usually a helper type to define sizes before any scaling has been applied.

What solution would you like?

I think it would improve consistency if the end user of bevy was encountered with a similar interface. Either by reexporting winit's definitions or by building some new types better suited for bevy's userspace.

What alternative(s) have you considered?

Keep using native types such as vectors and ints to not clutter the userspace with additional types. Most new users will quickly become familiar with Vec2 / IVec2 so keep using that might be preferable? If yes, I'd prefer all tuples become vectors so that we don't mix them.

@Weibye Weibye added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change labels Jun 13, 2022
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible labels Jun 13, 2022
@LoipesMas
Copy link
Contributor

Replacing tuples with vector types seems like a no-brainer to me, considering the fact that they are already used in such context somewhere else.
But I'm not 100% convinced on exposing logical/physical types, as it can be quite confusing (I sometimes get confused which one should be used when 😅). Unless there would be a good explanation in the docs, but I'm don't even know how it would look like.

@Weibye
Copy link
Contributor Author

Weibye commented Jun 13, 2022

Related to #4960

@bors bors bot closed this as completed in 3203a85 Jul 11, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
Resolves bevyengine#5004. As suggested in the original issue, change tuple types to their corresponding vector type.

## migration guide

Changed the following fields
- `WindowCommand::SetWindowMode.resolution` from `(u32, u32)` to `UVec2`
- `WindowCommand::SetResolution.logical_resolution` from `(f32, f32)` to `Vec2`

Co-authored-by: Daniel Liu <mr.picklepinosaur@gmail.com>
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
Resolves bevyengine#5004. As suggested in the original issue, change tuple types to their corresponding vector type.

## migration guide

Changed the following fields
- `WindowCommand::SetWindowMode.resolution` from `(u32, u32)` to `UVec2`
- `WindowCommand::SetResolution.logical_resolution` from `(f32, f32)` to `Vec2`

Co-authored-by: Daniel Liu <mr.picklepinosaur@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
Resolves bevyengine#5004. As suggested in the original issue, change tuple types to their corresponding vector type.

## migration guide

Changed the following fields
- `WindowCommand::SetWindowMode.resolution` from `(u32, u32)` to `UVec2`
- `WindowCommand::SetResolution.logical_resolution` from `(f32, f32)` to `Vec2`

Co-authored-by: Daniel Liu <mr.picklepinosaur@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants