-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add AtomicLayout
, abstracing layouting within widgets
#5830
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
base: main
Are you sure you want to change the base?
Conversation
Preview available at https://egui-pr-preview.github.io/pr/5830-lucasexperimentswidgetlayout |
This is mostly in preparation for #5830 where I want to ensure that I don't introduce any regressions
9b01154
to
d468565
Compare
AtomicLayout
that abstracts layouting within widgetsAtomicLayout
, abstracing layouting within widgets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run some benchmarks (the ones that are already there, and maybe some new ones more focused on e.g. just Button
)
@@ -99,7 +99,7 @@ fn menu_close_on_click_outside() { | |||
harness.run(); | |||
|
|||
harness | |||
.get_by_label("Submenu C (CloseOnClickOutside)") | |||
.get_by_label_contains("Submenu C (CloseOnClickOutside)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the label not contain more than just the text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a todo for this here:
https://github.com/emilk/egui/pull/5830/files#diff-7fd879073a3ce1839ef5c8fdd4dc5e50afc139c018b71c35ca5660bf89420470R657-R673
The text now unfortunately contains the ⏵ from the submenu button. Not sure what a good solution is to handle this. Maybe a flag on the Atomic? Maybe the Atomic could have a alt_text you could use for images, and if you add a text like ⏵ you could do "⏵".a_alt_text("")
or something like that.
This would also solve accessibility for Icon Fonts.
Remember to run |
…h the current one. - offset frame margin for stroke width - handle expansion - add min_size - calculate image size based on font height - correctly handle alignment / ui layout
* In preparation of #5830, this should reduce the performance impact of that PR --------- Co-authored-by: Emil Ernerfeldt <[email protected]>
# Conflicts: # crates/egui/src/widgets/button.rs
Should it be |
…implement it as fallback for old Button::image methods instead
Ran the demo benchmark for main and the PR and it seems to be slightly slower but I feel the slow down is acceptable for the features we gain:
See this comment for the button benchmark results. |
@@ -87,6 +87,7 @@ ahash.workspace = true | |||
bitflags.workspace = true | |||
nohash-hasher.workspace = true | |||
profiling.workspace = true | |||
smallvec.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sort
pub grow: bool, | ||
pub shrink: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should have docstrings:
- What dimension can grow/shrink?
- What happens if they are both set?
- Can the atomic grow beyond the
max_size
? - How does it interact with
size
? - …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: I suspect this grid would be a lot easier to read if we replaced all cross_
/main_
with horiz_
/vert_
instead, e.g. so that the rightmost column is always horiz_align: Right
|
||
(Some(galley), desired_size) | ||
}; | ||
let text = atomics.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use Cow
here to avoid allocations in the common case of there being only one (or zero) AtomicKind::Text
in Atomics
. That should save an allocation for each widget, which could be quite significant
@@ -243,8 +243,8 @@ pub struct MenuButton<'a> { | |||
} | |||
|
|||
impl<'a> MenuButton<'a> { | |||
pub fn new(text: impl Into<WidgetText>) -> Self { | |||
Self::from_button(Button::new(text)) | |||
pub fn new(text: impl IntoAtomics<'a>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably rename text
to atoms
here and elsewhere
AtomicKind::Text(text) => { | ||
let galley = text.into_galley(ui, wrap_mode, available_size.x, TextStyle::Button); | ||
( | ||
galley.size(), // TODO(lucasmerlin): calculate the preferred size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to link this TODO to a issue
/// | ||
/// You can use this to first allocate a response and then modify, e.g., the [`Frame`] on the | ||
/// [`AllocatedAtomicLayout`] for interaction styling. | ||
pub struct AtomicLayout<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AtomicLayout
is 432 bytes now. Most of that is Atomics
(360 bytes), because it has a SmallVec 2x
of Atomic
(176 bytes), most of which is Image
(152 bytes).
Not something we need to fix right now; I just wanted to investigate it a bit.
A quick fix would be to change AtomicKind::Image
into a Box<Image>
, i.e. optimize for the case where there is no image. That shrinks AtomicLayout
from 432 to to 176 bytes, a 60% reduction.
/// Set the minimum size of the Widget. | ||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Set the minimum size of the Widget. | |
#[inline] | |
/// Set the minimum size of the Widget. | |
/// | |
/// This will find and expand atoms with `grow: true`. | |
/// If there are no growable atoms then everything will be left-aligned. | |
#[inline] |
self.layout.push_left(Atomic::grow()); | ||
self.layout.push_left(atomic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be push_right
?
self.layout.push_left(Atomic::grow()); | ||
self.layout.push_left(right_text.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely should be .push_right
. I think you just named the function wrong.
let grow_width = f32::max(extra_space / grow_count as f32, 0.0).floor_ui(); | ||
|
||
let aligned_rect = if grow_count > 0 { | ||
align2.align_size_within_rect(Vec2::new(width_to_fill, desired_size.y), inner_rect) | ||
} else { | ||
align2.align_size_within_rect(desired_size, inner_rect) | ||
}; | ||
|
||
let mut cursor = aligned_rect.left(); | ||
|
||
let mut response = AtomicLayoutResponse::empty(response); | ||
|
||
for sized in sized_atomics { | ||
let size = sized.size; | ||
let growth = if sized.is_grow() { grow_width } else { 0.0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle sizing issue lurking here, that it might be worth thinking about. Assume .round_ui
rounds to integers for now (makes it easier to explain the problem):
If extra_space
is 7, and we have 3 growable atoms, then the current code will grow each of those atoms by 2, leading to a total growth of 6, which is less than extra_space
. Is that fine? Maybe. But a better (though more complicated) approach might be to handle this by allowing the last atom to grow more or less than the others.
/// | ||
/// Use the `custom_rects` together with [`AtomicKind::Custom`] to add child widgets to a widget. | ||
/// | ||
/// NOTE: Don't `unwrap` rects, they might be empty when the widget is not visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment makes more sense on fn get_rect
self.custom_rects.iter().copied() | ||
} | ||
|
||
pub fn get_rect(&self, id: Id) -> Option<Rect> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a warning about that the id
might be missing for hidden/invisible widgets
self.custom_rects.iter().copied() | ||
} | ||
|
||
pub fn get_rect(&self, id: Id) -> Option<Rect> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
pub fn get_rect(&self, id: Id) -> Option<Rect> { | |
pub fn rect(&self, id: Id) -> Option<Rect> { |
Today each widget does its own custom layout, which has some drawbacks:
Image
toButton
but it will always be shown on the left sideImage
to a e.g. aSelectableLabel
This PR introduces
Atomics
andAtomicLayout
which abstracts over "widget content" and layout within widgets, so it'd be possible to add images / text / custom rendering (for e.g. the checkbox) to any widget.A simple custom button implementation is now as easy as this:
The initial implementation only does very basic layout, just enough to be able to implement most current egui widgets, so:
There is a trait
IntoAtomics
that conveniently allows you to constructAtomics
from a tupleto get a button with image and text.
This PR reimplements three egui widgets based on the new AtomicLayout:
I plan on updating TextEdit based on AtomicLayout in a separate PR (so you could use it to add a icon within the textedit frame).