Change widget local coordinates to logical pixels.#1787
Change widget local coordinates to logical pixels.#1787xStrom wants to merge 2 commits intolinebender:mainfrom
Conversation
xStrom
added
masonry
|
If there's something to discuss about |
| } | ||
|
|
||
| /// Multiplies by the `other` value but the result doesn't go above the maximum value. | ||
| pub const fn saturating_mul(self, other: Self) -> Self { |
There was a problem hiding this comment.
This feels conceptually wrong. If this a Length, then it doesn't make sense to multiply a length by a length (and get back a Length).
Perhaps a mul_factor or scale_by that takes an f64?
There was a problem hiding this comment.
Yeah I agree that it feels wrong. The reason I left it as-is is because we want the input factor to also be finite and non-negative.
Though I guess it's possible to just make it a comment and check for the scenarios in the implementation.
I'll think about this some more ..
There was a problem hiding this comment.
I thought about it some more and I don't really like the other options either. So for now, I just removed this method. It's only used in two places right now and those can be implemented differently.
It might still make sense to add something like this in the future, but then that discussion can be more focused.
|
|
||
| let header_x_padding_length = header_x_padding.dp(scale) * 2.; | ||
| let btn_length = BUTTON_LENGTH.dp(scale); | ||
| let header_x_padding_length = header_x_padding.saturating_mul(Length::const_px(2.)); |
There was a problem hiding this comment.
This is an example of where saturating_mul on Length doesn't make sense, but mul_factor or scale_by would.
There was a problem hiding this comment.
Yeah, in this specific case it can also just be a saturating_add of itself, which is what I did now.
| (child1_cross_space, cross_space - child1_cross_space) | ||
| let cross_space = cross_length.saturating_sub(self.bar_thickness); | ||
| let split_point = self.calc_effective_split_point(cross_space.get()); | ||
| let child1_cross_space = cross_space.saturating_mul(split_point.px()); |
There was a problem hiding this comment.
This all just feels confusing to me. Of course it did before as well.
There was a problem hiding this comment.
It's not the best code, but I keep putting off making significant changes here because of some dream of supporting more than a single split point. 😅
waywardmonkeys
left a comment
waywardmonkeys
left a comment
There was a problem hiding this comment.
It feels wrong that Padding and BorderWidth are still in f64 and have workarounds for getting Length, but that makes the behavior for negative or non-finite values less great. (I didn't look to see if that's checked on ingestion.)
The dpi crate isn't a ton better for this despite having size types since it assumes there's just logical and physical spaces. Euclid is slightly better.
Just thinking if there's something that really needs a different fix at some point in the future.
|
Regarding |
2 participants
Background
#1264 set out a plan to move the whole Masonry Core API to physical pixels. That conversion to physical pixels would happen at the widget level. Some of the benefits mentioned on Zulip were that this would also allow the removal of
ui-eventsconversions as those coordinates could remain in physical pixels, and that pixel snapping would be better.The problem
What has happened is that we have effectively created a third pixel unit, which is defined as logical pixels multiplied by the DPI scale. Critically, this is not the same as physical pixels. Because it does not account for any of the transforms that the widget branch may have. So pointer events still need to be converted from physical pixels to these "masonry pixels" and pixel snapping will be incorrect.
Achieving actual physical pixel usage inside widgets is incompatible with the current layout system, as currently the full tree transform is not known before layout. However, let's assume for a moment that it would be possible to have actual physical pixels inside a widget.
If the widget branch has a non-axis-aligned transform with e.g. some shear or rotation then the widget code suddenly needs to know how to properly deal with that. That's a lot of complexity that doesn't need to exist if it just dealt with logical pixels.
Thus, we don't really want to achieve true physical pixel usage. It makes everything harder to reason about and requires a lot more complex code.
Keeping this third "masonry pixel" variant still around also doesn't make sense. It's an extra layer that will still need to be converted to physical pixels. Part of the original rationale was that we end up doing the conversion at different places and it's kind of awkward. Well, introducing these "masonry pixels" doesn't actually remove the need for that conversion. Because those places still need physical pixels, which means having undergone the full transformation. The DPI scale can be part of that same transformation, it doesn't need to be distributed elsewhere.
The path forward
We should instead move almost everything to logical pixels. This will make things easier to reason about, because there won't be unit conversions and all logic will work the same regardless of what DPI scale or complex transform is active. Conversions to physical pixels can happen deep in the core as part of the full transformation and it will be invisible to most of the code.
For cases like
ui-eventswhere physical pixels arrive to widgets, this once again doesn't change much. These events will need a conversion call anyway, because they aren't in the widget's local coordinate space, regardless of logical or physical pixels. So this conversion can continue to do everything that is needed.Painting should also happen in logical pixels. No surprises, no conversions, easy sharing of common code and data between various widget helper methods. For doing physical pixel aware painting, there should be special helper methods, e.g. for hairlines.
Changes in this PR
The layout system and all widget implementations have been changed to logical pixels.
There is now a lot more usage of the
Lengthtype to encapsulate the contract of using finite non-negative values which was previously comment based. This includes various layout method signatures, layout type internals (e.g.LenDef), and also variousWidget::measureimplementations.Follow-up work
There are things that are broken with the old approach that aren't fixed in this PR here yet, in order to limit PR size.