1-pixel separator lines are feathered #4776
Comments
|
I spent some time on this today with promising results. Hoping to have a PR ready soon. |
1 task
emilk
pushed a commit
that referenced
this issue
Aug 30, 2024
<!-- Please read the "Making a PR" section of [`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md) before opening a Pull Request! * Keep your PR:s small and focused. * The PR title is what ends up in the changelog, so make it descriptive! * If applicable, add a screenshot or gif. * If it is a non-trivial addition, consider adding a demo for it to `egui_demo_lib`, or a new example. * Do NOT open PR:s from your `master` branch, as that makes it hard for maintainers to test and add commits to your PR. * Remember to run `cargo fmt` and `cargo clippy`. * Open the PR as a draft until you have self-reviewed it and run `./scripts/check.sh`. * When you have addressed a PR comment, mark it as resolved. Please be patient! I will review your PR, but my time is limited! --> * Closes <#4776> * [x] I have followed the instructions in the PR template I've been meaning to look into this for a while but finally bit the bullet this week. Contrary to what I initially thought, the problem of blurry lines is unrelated to feathering because it also happens with feathering disabled. The root cause is that lines tend to land on pixel boundaries, and because of that, frequently used strokes (e.g. 1pt), end up partially covering pixels. This is especially noticeable on 1ppp displays. There were a couple of things to fix, namely: individual lines like separators and indents but also shape strokes (e.g. Frame). Lines were easy, I just made sure we round them to the nearest pixel _center_, instead of the nearest pixel boundary. Strokes were a little more complicated. To illustrate why, here’s an example: if we're rendering a 5x5 rect (black fill, red stroke), we would expect to see something like this:  The fill and the stroke to cover entire pixels. Instead, egui was painting the stroke partially inside and partially outside, centered around the shape’s path (blue line):  Both methods are valid for different use-cases but the first one is what we’d typically want for UIs to feel crisp and pixel perfect. It's also how CSS borders work (related to #4019 and #3284). Luckily, we can use the normal computed for each `PathPoint` to adjust the location of the stroke to be outside, inside, or in the middle. These also are the 3 types of strokes available in tools like Photoshop. This PR introduces an enum `StrokeKind` which determines if a `PathStroke` should be tessellated outside, inside, or _on_ the path itself. Where "outside" is defined by the directions normals point to. Tessellator will now use `StrokeKind::Outside` for closed shapes like rect, ellipse, etc. And `StrokeKind::Middle` for the rest since there's no meaningful "outside" concept for open paths. This PR doesn't expose `StrokeKind` to user-land, but we can implement that later so that users can render shapes and decide where to place the stroke. ### Strokes test (blue lines represent the size of the rect being rendered) `Stroke::Middle` (current behavior, 1px and 3px are blurry)  `Stroke::Outside` (proposed default behavior for closed paths)  `Stroke::Inside` (for completeness but unused at the moment)  ### Demo App The best way to review this PR is to run the demo on a 1ppp display, especially to test hover effects. Everything should look crisper. Also run it in a higher dpi screen to test that nothing broke 🙏. Before:  After (notice the sharper lines): 
486c
pushed a commit
to 486c/egui
that referenced
this issue
Oct 9, 2024
<!-- Please read the "Making a PR" section of [`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md) before opening a Pull Request! * Keep your PR:s small and focused. * The PR title is what ends up in the changelog, so make it descriptive! * If applicable, add a screenshot or gif. * If it is a non-trivial addition, consider adding a demo for it to `egui_demo_lib`, or a new example. * Do NOT open PR:s from your `master` branch, as that makes it hard for maintainers to test and add commits to your PR. * Remember to run `cargo fmt` and `cargo clippy`. * Open the PR as a draft until you have self-reviewed it and run `./scripts/check.sh`. * When you have addressed a PR comment, mark it as resolved. Please be patient! I will review your PR, but my time is limited! --> * Closes <emilk#4776> * [x] I have followed the instructions in the PR template I've been meaning to look into this for a while but finally bit the bullet this week. Contrary to what I initially thought, the problem of blurry lines is unrelated to feathering because it also happens with feathering disabled. The root cause is that lines tend to land on pixel boundaries, and because of that, frequently used strokes (e.g. 1pt), end up partially covering pixels. This is especially noticeable on 1ppp displays. There were a couple of things to fix, namely: individual lines like separators and indents but also shape strokes (e.g. Frame). Lines were easy, I just made sure we round them to the nearest pixel _center_, instead of the nearest pixel boundary. Strokes were a little more complicated. To illustrate why, here’s an example: if we're rendering a 5x5 rect (black fill, red stroke), we would expect to see something like this:  The fill and the stroke to cover entire pixels. Instead, egui was painting the stroke partially inside and partially outside, centered around the shape’s path (blue line):  Both methods are valid for different use-cases but the first one is what we’d typically want for UIs to feel crisp and pixel perfect. It's also how CSS borders work (related to emilk#4019 and emilk#3284). Luckily, we can use the normal computed for each `PathPoint` to adjust the location of the stroke to be outside, inside, or in the middle. These also are the 3 types of strokes available in tools like Photoshop. This PR introduces an enum `StrokeKind` which determines if a `PathStroke` should be tessellated outside, inside, or _on_ the path itself. Where "outside" is defined by the directions normals point to. Tessellator will now use `StrokeKind::Outside` for closed shapes like rect, ellipse, etc. And `StrokeKind::Middle` for the rest since there's no meaningful "outside" concept for open paths. This PR doesn't expose `StrokeKind` to user-land, but we can implement that later so that users can render shapes and decide where to place the stroke. ### Strokes test (blue lines represent the size of the rect being rendered) `Stroke::Middle` (current behavior, 1px and 3px are blurry)  `Stroke::Outside` (proposed default behavior for closed paths)  `Stroke::Inside` (for completeness but unused at the moment)  ### Demo App The best way to review this PR is to run the demo on a 1ppp display, especially to test hover effects. Everything should look crisper. Also run it in a higher dpi screen to test that nothing broke 🙏. Before:  After (notice the sharper lines): 
hacknus
pushed a commit
to hacknus/egui
that referenced
this issue
Oct 30, 2024
<!-- Please read the "Making a PR" section of [`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md) before opening a Pull Request! * Keep your PR:s small and focused. * The PR title is what ends up in the changelog, so make it descriptive! * If applicable, add a screenshot or gif. * If it is a non-trivial addition, consider adding a demo for it to `egui_demo_lib`, or a new example. * Do NOT open PR:s from your `master` branch, as that makes it hard for maintainers to test and add commits to your PR. * Remember to run `cargo fmt` and `cargo clippy`. * Open the PR as a draft until you have self-reviewed it and run `./scripts/check.sh`. * When you have addressed a PR comment, mark it as resolved. Please be patient! I will review your PR, but my time is limited! --> * Closes <emilk#4776> * [x] I have followed the instructions in the PR template I've been meaning to look into this for a while but finally bit the bullet this week. Contrary to what I initially thought, the problem of blurry lines is unrelated to feathering because it also happens with feathering disabled. The root cause is that lines tend to land on pixel boundaries, and because of that, frequently used strokes (e.g. 1pt), end up partially covering pixels. This is especially noticeable on 1ppp displays. There were a couple of things to fix, namely: individual lines like separators and indents but also shape strokes (e.g. Frame). Lines were easy, I just made sure we round them to the nearest pixel _center_, instead of the nearest pixel boundary. Strokes were a little more complicated. To illustrate why, here’s an example: if we're rendering a 5x5 rect (black fill, red stroke), we would expect to see something like this:  The fill and the stroke to cover entire pixels. Instead, egui was painting the stroke partially inside and partially outside, centered around the shape’s path (blue line):  Both methods are valid for different use-cases but the first one is what we’d typically want for UIs to feel crisp and pixel perfect. It's also how CSS borders work (related to emilk#4019 and emilk#3284). Luckily, we can use the normal computed for each `PathPoint` to adjust the location of the stroke to be outside, inside, or in the middle. These also are the 3 types of strokes available in tools like Photoshop. This PR introduces an enum `StrokeKind` which determines if a `PathStroke` should be tessellated outside, inside, or _on_ the path itself. Where "outside" is defined by the directions normals point to. Tessellator will now use `StrokeKind::Outside` for closed shapes like rect, ellipse, etc. And `StrokeKind::Middle` for the rest since there's no meaningful "outside" concept for open paths. This PR doesn't expose `StrokeKind` to user-land, but we can implement that later so that users can render shapes and decide where to place the stroke. ### Strokes test (blue lines represent the size of the rect being rendered) `Stroke::Middle` (current behavior, 1px and 3px are blurry)  `Stroke::Outside` (proposed default behavior for closed paths)  `Stroke::Inside` (for completeness but unused at the moment)  ### Demo App The best way to review this PR is to run the demo on a 1ppp display, especially to test hover effects. Everything should look crisper. Also run it in a higher dpi screen to test that nothing broke 🙏. Before:  After (notice the sharper lines): 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Separator lines which are 1 pixel wide, such as those used by default in panels and the
Separatorwidget, are rendered with their centers at a pixel coordinate rather than their edges at a pixel coordinate. This leads to them appearing as 2-pixel translucent lines rather than 1-pixel opaque lines.This was discussed somewhat in #1322, but I think it's worth revisiting. Right now, 1-pixel lines are very heavily used in egui, and this behavior can lead to some unintuitive and visually unappealing results (for instance, this contributes to #4773; the 1-pixel border of the panel feathers upwards, merging with the vertical separator above). If a 2-pixel transparent border is more aesthetically appealing, it should be explicitly represented as such for layout purposes.
I think this lies more on the "bug" side than the "feature" side because the code that draws these separator lines, for both separators and panels, does call
round_to_pixel--it's just that the rounding value is incorrect for lines with odd widths.To Reproduce
This can be seen in any egui application that uses panels or separators. For instance, all of these 2-pixel medium-grey outlines in the demo app are supposed to be 1-pixel light grey outlines:
Expected behavior
Lines which are 1 pixel (as a unit) wide should, at 1 pixel per point, be 1 physical pixel wide. It's not as simple as always rounding up or down-- for e.g. a border around a rectangle, you may want to round "outwards" or "inwards", which rounds differently depending on which side of the rectangle you're on. It may be worth looking into how CSS does this (which ties into #4019).
The text was updated successfully, but these errors were encountered: