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

Add ui.align_cursor() to for the cursor to be aligned with physical pixels #4928

Closed
abey79 opened this issue Aug 6, 2024 · 6 comments
Closed
Labels
egui epaint rerun Desired for Rerun.io

Comments

@abey79
Copy link
Collaborator

abey79 commented Aug 6, 2024

Unaligned cursor lead to various visual glitches. It would be nice to have an API to "fix" the cursor position. This would avoid using code like this:

ui.add_space(-ui.cursor().min.y.fract());

which is not robust to layout orientation and assumes 1:1 physical pixels.

Related:

@abey79 abey79 added egui rerun Desired for Rerun.io labels Aug 6, 2024
@emilk emilk added this to the Next Major Release milestone Aug 27, 2024
@emilk
Copy link
Owner

emilk commented Sep 16, 2024

There are three alternatives:

  • Round widgets to physical pixels
  • Round widgets to logical ui points
  • Only round when pixels_per_point is an integer
  • No rounding

I'd like to understand a bit more what the trade-offs are between these.

Regardless of how/if we round widgets during layout, we still want to round some visual shapes to physical pixels to make them crisper (text, thin lines, etc).

Rounding to physical pixels

Since we need to round some visual shapes to physical pixels anyway, always rounding to physical pixels may produce more even results.

Rounding to logical ui points

All calculations are done in logical ui points, and so if we make them integers we prevent rounding errors.


Unaligned cursor lead to various visual glitches.

I'd like a repro for this

@abey79
Copy link
Collaborator Author

abey79 commented Sep 17, 2024

I'd like a repro for this

use eframe::egui;

fn main() -> eframe::Result {
    env_logger::init(); // Log to stderr (if you run with `RUST_LOG=debug`).
    let options = eframe::NativeOptions {
        viewport: egui::ViewportBuilder::default().with_inner_size([320.0, 240.0]),
        ..Default::default()
    };
    eframe::run_native(
        "My egui App",
        options,
        Box::new(|cc| {
            // This gives us image support:
            egui_extras::install_image_loaders(&cc.egui_ctx);

            Ok(Box::new(MyApp))
        }),
    )
}

struct MyApp;

impl eframe::App for MyApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        egui::CentralPanel::default().show(ctx, |ui| {
            // unaligned cursor
            ui.add_space(0.1234);

            ui.spacing_mut().item_spacing.y = 0.0;

            for _ in 0..5 {
                let (rect, _) =
                    ui.allocate_exact_size(egui::vec2(100.0, 20.0), egui::Sense::hover());
                ui.painter()
                    .rect_filled(rect, 0.0, ui.style().visuals.selection.bg_fill);
            }
        });
    }
}
image



Commenting out the mis-alignment line gives the expect result:

image



One notch of zooming-in (cmd-+) brings back the glitch though:

image

@abey79
Copy link
Collaborator Author

abey79 commented Sep 17, 2024

There are three alternatives:

  • Round widgets to physical pixels
  • Round widgets to logical ui points
  • Only round when pixels_per_point is an integer
  • No rounding

In light of the repro above, I believe rounding to physical pixels is what's needed, but, ultimately, whatever fixes this 😅

@emilk emilk added the epaint label Sep 17, 2024
@emilk
Copy link
Owner

emilk commented Sep 17, 2024

The problem there is the feathering (anti-aliasing) of the boxes.

The easy fix there would be to always align boxes to the pixel grid, and turn of feathering for them.
The usefulness of feathering on axis-aligned boxes is minimal anyway. If you're animating boxes then it will make them move smoother, but that seems quite niche.

Related:

@emilk
Copy link
Owner

emilk commented Sep 25, 2024

I think the correct thing to do here is to align ui points to integers in order to avoid rounding errors in width calculations and similar (see e.g. #5084).

The problem of rendering rectangles side-by-side without a seem is a purely visual one, and can be solved separately by one of these means:

  • Tell the user to round rectangles to pixel grid before painting them, if they plan on putting two of them side-by-side
  • Always round filled rectangles to the pixel grid
  • Add a rounding mode option to RectShape

I've opened two new issues for this:

emilk added a commit that referenced this issue Sep 25, 2024
* Closes #5106
* Closes #5084


Protect against rounding errors in egui layout code.

Say the user asks to wrap at width 200.0.
The text layout wraps, and reports that the final width was 196.0
points.
This than trickles up the `Ui` chain and gets stored as the width for a
tooltip (say).
On the next frame, this is then set as the max width for the tooltip,
and we end up calling the text layout code again, this time with a wrap
width of 196.0.
Except, somewhere in the `Ui` chain with added margins etc, a rounding
error was introduced,
so that we actually set a wrap-width of 195.9997 instead.
Now the text that fit perfectly at 196.0 needs to wrap one word earlier,
and so the text re-wraps and reports a new width of 185.0 points.
And then the cycle continues.

So this PR limits the text wrap-width to be an integer.

Related issues:
* #4927
* #4928
* #5163

--- 

Pleas test this @rustbasic
hacknus pushed a commit to hacknus/egui that referenced this issue Oct 30, 2024
…k#5161)

* Closes emilk#5106
* Closes emilk#5084


Protect against rounding errors in egui layout code.

Say the user asks to wrap at width 200.0.
The text layout wraps, and reports that the final width was 196.0
points.
This than trickles up the `Ui` chain and gets stored as the width for a
tooltip (say).
On the next frame, this is then set as the max width for the tooltip,
and we end up calling the text layout code again, this time with a wrap
width of 196.0.
Except, somewhere in the `Ui` chain with added margins etc, a rounding
error was introduced,
so that we actually set a wrap-width of 195.9997 instead.
Now the text that fit perfectly at 196.0 needs to wrap one word earlier,
and so the text re-wraps and reports a new width of 185.0 points.
And then the cycle continues.

So this PR limits the text wrap-width to be an integer.

Related issues:
* emilk#4927
* emilk#4928
* emilk#5163

--- 

Pleas test this @rustbasic
@emilk
Copy link
Owner

emilk commented Dec 11, 2024

@emilk emilk closed this as not planned Won't fix, can't repro, duplicate, stale Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui epaint rerun Desired for Rerun.io
Projects
None yet
Development

No branches or pull requests

2 participants