Floor the origin for the Align widget #1091
Conversation
druid/src/widget/align.rs
Outdated
| @@ -121,7 +121,8 @@ impl<T: Data> Widget<T> for Align<T> { | |||
| let extra_height = (my_size.height - size.height).max(0.); | |||
| let origin = self | |||
| .align | |||
| .resolve(Rect::new(0., 0., extra_width, extra_height)); | |||
| .resolve(Rect::new(0., 0., extra_width, extra_height)) | |||
| .floor(); | |||
There was a problem hiding this comment.
I think this makes sense, but I have two questions:
- is it clear that floor is the right thing (as opposed to, say, round)?
- should this use
Scaleto do the rounding in pixels instead of dp? I don't think we currently make much of an effort to have sizes be integer multiples of pixels so maybe it doesn't matter. But I could imagine that if the scaling factor is 2.0 then it would be worthwhile to round to half-dps instead of dps.
There was a problem hiding this comment.
is it clear that floor is the right thing (as opposed to, say, round)?
It can be just a convention to use floor, or ceil or round. With floor it works fine.
should this use Scale to do the rounding in pixels instead of dp
I'll check Scale.
There was a problem hiding this comment.
is it clear that floor is the right thing (as opposed to, say, round)?
I'm pretty sure using floor is what is done elsewhere, but I can't seem to find the reference right now.
should this use Scale to do the rounding in pixels instead of dp
We should definitely be rounding to nearest pixel. I'm not familiar with this bit of code, so if the PR does this or not.
There was a problem hiding this comment.
Most (all?) of the units in druid are "display points" by default, not pixels (see #913). To round to pixels, you'll want to use Scale::to_px, then round, then convert back using Scale::to_dp.
One argument for using round() instead of floor() is that (for any sane scale factor) this whole procedure should be idempotent. With floor() there's a chance that if you round something twice then the second time you try to round, the conversion to pixels will be just less than an integer and so taking the floor will reduce it further.
Probably this isn't an issue in the Align widget, but I think this rounding will eventually be needed in more places. In particular, this patch will only work if the origin of the Align widget has integer pixel coordinates, which implies that other containers should also be rounding positions.
There was a problem hiding this comment.
This is interesting, I hadn't really thought of scaling to device pixels. If this is something we want to do, I wonder if we shouldn't either do this systematically (say in draw/stroke methods on the graphics context) or else provide some consistent and easy to use mechanism for doing this alignment, and encouraging its use everywhere?
There was a problem hiding this comment.
I'd argue against automatically doing rounding in the built-in methods. I was thinking of an API more like
let rect = ctx.scale().round_to_px(rect);the idea being that there would be relatively convenient ways to round various geometric things to pixel boundaries, and then the widget author would be in charge of manually invoking them when appropriate.
I think that would partially address the surprise factor where various built-in methods start acting differently with different DPI. Admittedly, it might still be surprising that Align (and presumably also Flex) change their behavior...
There was a problem hiding this comment.
I agree that being explicit here makes the most sense.
There was a problem hiding this comment.
I would like to avoid having such a function on Scale but instead have it on the types which can have this done to them (such as being added to the Scalable trait). For example we had Scale::to_px(Scalable) which ended up calling Scalable::to_px(Scale) when it made much more sense for the callsite to just call on the scalable to begin with especially with how the method is named. This next point is total bikeshedding but a dot-method with an imperative name sounds like it should operate on the value it is called on or at least return a new copy with the modification applied.
There was a problem hiding this comment.
Aah I found the reference, in BoxConstraints
The given size is also rounded away from zero, so that the layout is aligned to integers.
There was a problem hiding this comment.
Interesting! Unfortunately, it doesn't work in the presence of a non-integer scale factor...
jneem
added
S-waiting-on-author
There was a problem hiding this comment.
This feels like an improvement over our current situation, even if it raises larger questions about proper dpi-aware scaling.
I'd like to use expand instead of floor; we established that convention in the original work on BoxConstraints, and it's worth being consistent here.
Sorry for letting this sit around!
cmyr
added
S-ready
Fixes #1088 caused by the
Alignwidget.