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

Align Container and debug borders to pixels. #669

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Mar 15, 2020

How stroke works

The coordinates provided to stroke are fully respected and not rounded to an integer. Furthermore the stroke is positioned exactly on top of the provided coordinates. This means that if you draw a 1px wide line at pixel boundaries, 0.5px of the line width will be on one side pixel and the other 0.5px on the other side. Of course the pixel is usually the smallest unit a display has, so the 1px line ends up looking like a 2px line, although usually alpha blended. The solution is to specify the midpoint of a pixel instead, i.e. if you want the line to appear on the 2nd pixel row you specify 1.5 as the point.

stroke-point

Steps towards improving druid

This PR changes the way the Container widget border and the debug borders are drawn. In both cases they are drawn inwards, i.e. within the widget size, with pixel accuracy.

This PR also moves the Container border drawing after the background drawing so that you can have both active at the same time.

Real life example (400% zoom)

real-changes

Future work

  • There is still more stroke usage in druid that isn't pixel perfect. These should all be addressed.
  • The widget layouts themselves should be quantized to integers. Otherwise even if the stroke usage is pixel-perfect, it will have a non-integer origin. This is currently affecting Label and probably everything that uses Label's layout in calculations of its own layout, e.g. Button.

@cmyr
Copy link
Member

cmyr commented Mar 15, 2020

Awesome, this is 100% welcome and important. There are a few places where I've done this manually (mostly in runebender I guess?) and I haven't written it up anywhere. Would be good to have this be in the docs somewhere...

@cmyr
Copy link
Member

cmyr commented Mar 15, 2020

and also 100% layouts should be on the grid. Maybe the simplest option would be to floor all sizes that are used as layouts? this could cause some weird issues though. Maybe also BoxConstraints::constrain should enforce that sizes are integers; this happens earlier, so we'd be less likely to do things like accumulate non-rounded numbers when doing layout math.

@xStrom
Copy link
Member Author

xStrom commented Mar 15, 2020

I think ceil is better than floor because otherwise there will be drawing overflow. I was thinking of sending a PR to kurbo to add both methods to Size.

Solving this in some more generic way like BoxConstraints::constrain could be worth it, as it will help people who aren't that familiar with this whole mid-point drawing business. That said, I haven't gone over all the widgets yet to properly comment on how to best do it.

@cmyr
Copy link
Member

cmyr commented Mar 15, 2020

Noted about ceil. That pr would be welcome in kurbo.

@xStrom xStrom force-pushed the quantize-int branch 2 times, most recently from a75fdf9 to 33dacdc Compare March 16, 2020 23:11
@xStrom
Copy link
Member Author

xStrom commented Mar 17, 2020

Okay so the kurbo changes have been merged and I'll try to tackle some of the future work (i.e. fix this issue more broadly and document it better) later this week. I'd prefer to do that in separate PRs.

So I'd like to move forward with merging these changes here. Let me know if anything needs adjusting.

@cmyr
Copy link
Member

cmyr commented Mar 17, 2020

looking now.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

okay a couple notes but nothing blocking. I'm going to add you as a collaborator and you can merge this when you're satisfied.

let rect = Rect::from_origin_size(
(BORDER_MIDPOINT, BORDER_MIDPOINT),
(size.width - BORDER_WIDTH, size.height - BORDER_WIDTH),
);
Copy link
Member

@cmyr cmyr Mar 17, 2020

Choose a reason for hiding this comment

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

another way to think about this is in terms of insets:

const BORDER_WIDTH: f64 = 1.0;
let bounds = ctx.size().to_rect();
let stroke_inset = Insets::uniform(BORDER_WIDTH / 2.0);
let stroke_rect = rect.inset(stroke_inset);
// can also just write rect.inset(BORDER_WIDTH / 2.0) for uniform insets

Copy link
Member Author

Choose a reason for hiding this comment

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

The inset helper methods are definitely helpful and made the code more concise. In this case the insets do have to be negative though, to shrink the rect.

self.corner_radius,
);

if let Err(e) = paint_ctx.save() {
Copy link
Member

Choose a reason for hiding this comment

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

I really get annoyed by this api, will look into a helper method (this is unrelated to the current PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the with_save method will reduce human failure chance.

if let Some(background) = self.background.as_mut() {
let bg_rect = RoundedRect::from_origin_size(
Copy link
Member

Choose a reason for hiding this comment

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

my current preferred way to do this (until linebender/kurbo#92 merges) is paint_ctx.to_rect().to_rounded_rect(self.corner_radius); 😒

Copy link
Member Author

Choose a reason for hiding this comment

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

Using the helper methods now!

@xStrom
Copy link
Member Author

xStrom commented Mar 18, 2020

I did some additional testing and found that Container had a bug where it was using paint instead of paint_with_offset even though it sets an offset in its layout method. Fixed that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants