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

Fix Node::physical_rect and add a physical_size method #8551

Merged
merged 7 commits into from
May 11, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented May 5, 2023

Objective

  • Node::physical_rect divides the logical size of the node by the scale factor, when it should multiply.
  • Add a physical_size method to Node that calculates the physical size of a node.

Changelog

  • Added a method physical_size to Node that calculates the physical size of the Node based on the given scale factor.
  • Fixed the Node::physical_rect method, the logical size should be multiplied by the scale factor to get the physical size.
  • Removed the scale_value function from the text widget module and replaced its usage with Node::physical_size.
  • Derived Copy for Node (since it's only a wrapped Vec2).
  • Made Node::size const.

* Added a method `physical_size` to `Node` that calculates the physical size of the `Node` based on the given scale factor.
* Fixed the `Node::physical_rect` method, the logical size should be multiplied by the scale factor to get the physical size.
* Removed the `scale_value` function from the `text` widget module and replaced its usage with `Node::physical_size`.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels May 5, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone May 5, 2023
@alice-i-cecile alice-i-cecile added 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 May 6, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch May 8, 2023
@@ -30,6 +30,15 @@ impl Node {
self.calculated_size
}

/// Returns the size of the node in physical pixels based on the given scale factor.
#[inline]
pub fn physical_size(&self, scale_factor: f64) -> Vec2 {
Copy link
Member

Choose a reason for hiding this comment

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

Curious why we need the double precision here when almost everything we're working with is single precision.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC this is just a winit convention that we've stuck with.

Copy link
Contributor Author

@ickshonpe ickshonpe May 10, 2023

Choose a reason for hiding this comment

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

This hasn't seemed like a priority so I've left it alone but I can't see anywhere that it makes any difference. We're not dealing with large numbers, Taffy and bevy_text both do rounding of coordinates and almost everything is recalculated with each update. There is nowhere for errors to accumulate.

If there are problems any with using f32s everywhere, I think they would be very situational and we can identify and use double precision in those specific cases. It makes no sense to use double precision when calculating mouse over, for instance, and it's an extra little burden for contributors and users when the scale factor implementation is already fragile and confusing.

Changed `physical_size` and `physical_rect` to use single precision values.
* `Node` is just a wrapper for a `Vec2` so made it derive `Copy`.
* Changed `scale_factor` method params back `f64`. If it's not part of an engine wide change of `scale_factor`, single precision params are just going to be annoying.
@alice-i-cecile
Copy link
Member

@ickshonpe can you resolve the merge conflicts and then I'll merge :)

@ickshonpe
Copy link
Contributor Author

@ickshonpe can you resolve the merge conflicts and then I'll merge :)

done

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 11, 2023
Merged via the queue into bevyengine:main with commit a35ed55 May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants