-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Round widget coordinates to integers #5163
Comments
This was referenced Sep 25, 2024
This was referenced Sep 25, 2024
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
emilk
added a commit
that referenced
this issue
Sep 25, 2024
This was referenced Sep 27, 2024
emilk
added a commit
that referenced
this issue
Sep 30, 2024
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
hacknus
pushed a commit
to hacknus/egui
that referenced
this issue
Oct 30, 2024
This tool highlights coordinates that are non-integer. * Closes emilk#4927 * Will be used for emilk#5163 This is disabled by default (even in debug builds), because so many widgets cause un-alignment currently.
hacknus
pushed a commit
to hacknus/egui
that referenced
this issue
Oct 30, 2024
* Closes emilk#5173 * Part of emilk#5163 * Related to emilk#5164
emilk
added a commit
that referenced
this issue
Dec 26, 2024
* Closes #5197 * Closes #5163 This should help prevent rounding errors in layout code. @lucasmerlin you may wanna test this with `egui_flex`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Rounding errors in size calculation can lead to subtle layout bugs.
In particularly when auto-sizing an area, the size is usually the sum of the sizes of many smaller widgets and border margins. If we have rounding errors, we might next frame tell the inner widgets to have a slightly different size than last frame, which could potentially cause drastic changes, like word-breaking some text earlier (see #5161).
Integers up to
2^24
(4Mi) can be represented exactly usingf32
, so has no rounding errors when adding and subtracting them.Using integers for layout coordinates therefore seems like a good idea.
This includes margin widths, etc.
To detect problems like these we first want to implement:
Once that is in, we can start fixing these issues.
There is a sliding scale of how aggressively we enforce this.
Best-effort
Make sure all default egui widgets and margins are integers, and that centering layouts (etc) round to integers.
This means the whole egui_demo_app should end up with integer coordinates.
Ui
-enforcedWe could also have
Ui
round itscursor
, allocated sizes etc to integers, basically pushing integer coordinates onto the user.Switch from
f32
toi32
This is the most aggressive stance, where we switch to integer coordinates everywhere.
I suspect this will be taking it too far because:
f32
for visual things (shapes), so it would requite having anRectf
and aRecti
,Pos2f
andPos2i
etc, and then have glue-code between the two everywhereRound to integers or less?
We want to avoid rounding errors due to limited precision, but that doesn't mean we need to round to integers.
We could just as well round to 1/16ths, for instance, and still have plenty of precision in a
f32
.The advantage of rounding to a fractional value is that we can get more fine-grained layouts, and smoother scrolling.
Optional: ban the use of
.round()/.ceil(),.floor()
inclippy.toml
to make sure we always either useround_gui
or round to physical pixels.Rounding to ui points vs physical pixels
Rounding to ui points is the only thing that will prevent rounding errors.
Rounding to physical pixels still makes sense when rendering, i.e. to align text and other shapes to the physical pixel grid.
This however should not affect the layout, so it should be done either by the widgets before handing shapes to the painter, or automatically by the tessellator.
See #5164 for more
Related issues
ui.align_cursor()
to for the cursor to be aligned with physical pixels #4928The text was updated successfully, but these errors were encountered: