Rework pixel snapping.#1792
Conversation
5225253 to
66635af
Compare
Wait, that doesn't sound right at all. Shouldn't the size passed down from the parent Badged to the child Button be a pixel-snapped 71 pixels? I guess the reason we don't do that is because we want pixel snapping to account for matrix transforms? If so, seems like we should just drop snapping for non-integer translates anyway. |
(Just to be clear to everyone, this is discussion about the old system.) Pixel snapping occurs in |
|
(Continuing discussion in Zulip: #masonry > Reworking pixel snapping) |
## Background linebender#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-events` conversions 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. [on Zulip]: https://xi.zulipchat.com/#narrow/channel/317477-masonry/topic/Plan.20for.20scale.20factor.2C.20dpi.2C.20and.20layout.20units/with/592271589 ## 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-events` where 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 `Length` type 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 various `Widget::measure` implementations. ## 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. * Pixel snapping (linebender#1792) * Baseline snapping * Physical pixel aligned paint helpers e.g. hairlines
66635af to
2586950
Compare
Background
The old pixel snapping system rounded both the origin (top-left) and end point (bottom-right) of the layout border-box in the parent widget's layout border-box coordinate space. Specifically this happened during the layout pass, when the parent called
place_child.The major benefit of this approach is that adjacent geometry will be pixel snapped in a compatible way where there won't be any overlap or empty gaps. This has the promise of a very polished and sharp looking render.
The cost of this approach is that the layout size and pixel snapped size may diverge. The box might shrink, remain the same, or grow.
The problems keep coming
As we've lived with this system, more problems have become clear.
Masonry supports arbitrary transforms on widgets. Doing the pixel snapping in the parent's coordinate space means that none of the transforms that convert from the parent's space to the window's space are taken into account. This is a critical flaw, because the idea of snapping is to align to device pixels, not to parent widget local pixels.
Because snapping happens in the parent's space, there is no consistent rounding of boxes across nested widget levels. It's easy to run into situations where both parent and child have the exact same fractional size, yet they get snapped to different integer sizes. I give an example of this below, in the screenshot changes section.
We've known that the cost of this approach is that snapped box sizes will differ from layout sizes. However, what has not been as obvious and definitely not documented, is that this also effectively introduces a whole new coordinate space. The
(1,1)of the layout box isn't the same window pixel location as the(1,1)of the snapped box. Yet there is a bunch of code that ignores this shift and mixes layout and snapped coordinates. Sometimes that can be ok, but thus far very little of it seems intentional.Improving the situation
We can achieve much improved pixel snapping if we resolve the layout border-box via the full window transform, apply the DPI scale factor to get device pixels, round, and map the snapped box back into local layout border-box space. The rounding will also be consistent across arbitrary nested transforms, so equally sized parent-child combos will result in equal snapped sizes. Pixel snapping will be skipped for a specific widget if the transform has rotation or shear, as it will just add noise and won't be correct. In the future we can also skip the snapping of specific widgets during a smooth animation.
The differences between layout and snapped coordinates will still exist and there is no way around that. What we can do is be much clearer in our method/variable naming and documentation. We can also provide ergonomic helpers to convert between these spaces.
Changes in this PR
Widget::measure/layoutoperate in layout content-box space and other methods in visual content-box space.Ctx::visual_translationfor visual<->layout spaces in addition toCtx::border_box_translationthat does border<->content spaces.Ctx::layout_border_boxand otherlayout_methods for widgets that prefer layout geometry.Ctx::content_box_size,Ctx::border_box_size,Ctx::paint_box_sizebecause they were too big of a footgun. They often can't just be converted toRect, but that's what was sometimes happening.Ctx::border_boxetc remain and when really neededCtx::border_box().size()can be used.ComposeCtx::set_animated_child_scroll_translationas the regularset_child_scroll_translationno longer rounds and thus these two were equivalent.Ctx::window_originwhich was a major footgun and often used incorrectly. You can't just take the transformed origin and add non-transformed values to it. The proper results can still be achieved withCtx::window_transformandCtx::to_window.Ctx::get_scale_factortoCtx::scale_factorand made it available in the same contexts asCtx::window_transform.WindowEvent::Rescalehandler now also requests compose and runs rewrite passes so visual geometry is up-to-date before the next pointer event.tests/compose.rs.tests/layout.rs.Screenshot changes
All test screenshot changes are due to improved pixel snapping. Specifically that pixel snapping is now consistent across widget nesting levels as it is done in the window's coordinate space.
Let's take the
badged_button_no_badgescreenshot as an example, because it's visually just a simple button, though structurally it'sAlign(Badged(Button(Label))). The button is 1px narrower in the new screenshot.The old snapping system worked in the parent widget's coordinate space. So in this case the
Buttonwidth is70.6. TheButtonis placed at(0,0)in the parent's space (Badged), which means that the rounded x coords will be(round(0) = 0, round(70.6) = 71)and the visualButtonwill be71pixels wide.Badgedsizes itself also70.6during layout, but because it is centered in its parent's space (Align) withx0 = (240 - 70.6) / 2 = 84.7, the rounding will be different:(round(84.7) = 85, round(84.7 + 70.6 = 155.3) = 155)and so the visualBadgedwill be155-85 = 70pixels wide. So we end up in a situation where despite the child and parent having the exact same layout size, they end up with different pixel snapped visual sizes.With the new system all of this is solved. Everything is snapped in the window's coordinate space and so in this case both
BadgedandButtonend up with a visual width of70pixels.Follow-up work
These are related things that are either certainly or potentially broken that didn't receive full attention in this PR here yet, in order to limit PR size.
Portal.Switchand to a lesser degreeCheckboxandRadioButtonhave custom paint logic that behaves as before, but may contain logic flaws and deserve a deeper inspection at some later date.Draft because it depends on #1791 and currently includes all of those commits as well.