Skip to content

Commit

Permalink
Use bitfield instead of bools in Response and Sense (#5556)
Browse files Browse the repository at this point in the history 
<!--
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 <#3862>.

Factoring the `bool` members of `Response` into a bitfield, the size of
`Response` is now 96 bytes (down from 104).

I gave `Sense` the same treatment, however this has no effects on
`Response` due to padding. I've decided not to pursue `PointerState`, as
it is quite large (_many_ members that are sized and aligned to
multiples of 8 bytes), so I don't expect any noticeable benefit from
making handful of `bool`s slightly leaner.

In any case, the changes to `Sense` are already quite a bit more
intrusive than those to `Response`.
The previous implementation overloaded the names of the attributes
`click` and `drag` with similarly named methods that _construct_ `Sense`
with the corresponding flag set. Now, that the attributes can no longer
be accessed directly, I had to introduce methods with new names
(`senses_click()`, `senses_drag()` and `is_focusable()`). I don't think
this is the cleanest solution: the old methods are essentially redundant
now that the named constants like `Sense::CLICK` exist. I did however
not want to needlessly break backwards compatibility.
I am happy to revert it (or go further 🙂) if there are concerns.
  • Loading branch information
@polwel
polwel authored Jan 6, 2025
1 parent 0fac8ea commit 3586041
Show file tree
Hide file tree
Showing 18 changed files with 265 additions and 278 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,7 @@ dependencies = [
"accesskit",
"ahash",
"backtrace",
"bitflags 2.6.0",
"document-features",
"emath",
"epaint",
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ ahash = { version = "0.8.11", default-features = false, features = [
"std",
] }
backtrace = "0.3"
bitflags = "2.6"
bytemuck = "1.7.2"
criterion = { version = "0.5.1", default-features = false }
dify = { version = "0.7", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ Light Theme:

* [`ab_glyph`](https://crates.io/crates/ab_glyph)
* [`ahash`](https://crates.io/crates/ahash)
* [`bitflags`](https://crates.io/crates/bitflags)
* [`nohash-hasher`](https://crates.io/crates/nohash-hasher)
* [`parking_lot`](https://crates.io/crates/parking_lot)

Expand Down
1 change: 1 addition & 0 deletions crates/egui/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ emath = { workspace = true, default-features = false }
epaint = { workspace = true, default-features = false }

ahash.workspace = true
bitflags.workspace = true
nohash-hasher.workspace = true
profiling.workspace = true

Expand Down
17 changes: 4 additions & 13 deletions crates/egui/src/containers/modal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,7 @@ impl Modal {
response,
} = area.show(ctx, |ui| {
let bg_rect = ui.ctx().screen_rect();
let bg_sense = Sense {
click: true,
drag: true,
focusable: false,
};
let bg_sense = Sense::CLICK | Sense::DRAG;
let mut backdrop = ui.new_child(UiBuilder::new().sense(bg_sense).max_rect(bg_rect));
backdrop.set_min_size(bg_rect.size());
ui.painter().rect_filled(bg_rect, 0.0, backdrop_color);
Expand All @@ -101,14 +97,9 @@ impl Modal {
// We need the extra scope with the sense since frame can't have a sense and since we
// need to prevent the clicks from passing through to the backdrop.
let inner = ui
.scope_builder(
UiBuilder::new().sense(Sense {
click: true,
drag: true,
focusable: false,
}),
|ui| frame.show(ui, content).inner,
)
.scope_builder(UiBuilder::new().sense(Sense::CLICK | Sense::DRAG), |ui| {
frame.show(ui, content).inner
})
.inner;

(inner, backdrop_response)
Expand Down
105 changes: 61 additions & 44 deletions crates/egui/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::{
os::OperatingSystem,
output::FullOutput,
pass_state::PassState,
resize, scroll_area,
resize, response, scroll_area,
util::IdTypeMap,
viewport::ViewportClass,
Align2, CursorIcon, DeferredViewportUiCallback, FontDefinitions, Grid, Id, ImmediateViewport,
Expand Down Expand Up @@ -1151,8 +1151,9 @@ impl Context {
/// same widget, then `allow_focus` should only be true once (like in [`Ui::new`] (true) and [`Ui::remember_min_rect`] (false)).
#[allow(clippy::too_many_arguments)]
pub(crate) fn create_widget(&self, w: WidgetRect, allow_focus: bool) -> Response {
let interested_in_focus =
w.enabled && w.sense.focusable && self.memory(|mem| mem.allows_interaction(w.layer_id));
let interested_in_focus = w.enabled
&& w.sense.is_focusable()
&& self.memory(|mem| mem.allows_interaction(w.layer_id));

// Remember this widget
self.write(|ctx| {
Expand All @@ -1173,15 +1174,15 @@ impl Context {
self.memory_mut(|mem| mem.surrender_focus(w.id));
}

if w.sense.interactive() || w.sense.focusable {
if w.sense.interactive() || w.sense.is_focusable() {
self.check_for_id_clash(w.id, w.rect, "widget");
}

#[allow(clippy::let_and_return)]
let res = self.get_response(w);

#[cfg(feature = "accesskit")]
if allow_focus && w.sense.focusable {
if allow_focus && w.sense.is_focusable() {
// Make sure anything that can receive focus has an AccessKit node.
// TODO(mwcampbell): For nodes that are filled from widget info,
// some information is written to the node twice.
Expand Down Expand Up @@ -1213,11 +1214,13 @@ impl Context {
#[deprecated = "Use Response.contains_pointer or Context::read_response instead"]
pub fn widget_contains_pointer(&self, id: Id) -> bool {
self.read_response(id)
.map_or(false, |response| response.contains_pointer)
.map_or(false, |response| response.contains_pointer())
}

/// Do all interaction for an existing widget, without (re-)registering it.
pub(crate) fn get_response(&self, widget_rect: WidgetRect) -> Response {
use response::Flags;

let WidgetRect {
id,
layer_id,
Expand All @@ -1237,61 +1240,72 @@ impl Context {
rect,
interact_rect,
sense,
enabled,
contains_pointer: false,
hovered: false,
highlighted,
clicked: false,
fake_primary_click: false,
long_touched: false,
drag_started: false,
dragged: false,
drag_stopped: false,
is_pointer_button_down_on: false,
flags: Flags::empty(),
interact_pointer_pos: None,
changed: false,
intrinsic_size: None,
};

res.flags.set(Flags::ENABLED, enabled);
res.flags.set(Flags::HIGHLIGHTED, highlighted);

self.write(|ctx| {
let viewport = ctx.viewports.entry(ctx.viewport_id()).or_default();

res.contains_pointer = viewport.interact_widgets.contains_pointer.contains(&id);
res.flags.set(
Flags::CONTAINS_POINTER,
viewport.interact_widgets.contains_pointer.contains(&id),
);

let input = &viewport.input;
let memory = &mut ctx.memory;

if enabled
&& sense.click
&& sense.senses_click()
&& memory.has_focus(id)
&& (input.key_pressed(Key::Space) || input.key_pressed(Key::Enter))
{
// Space/enter works like a primary click for e.g. selected buttons
res.fake_primary_click = true;
res.flags.set(Flags::FAKE_PRIMARY_CLICKED, true);
}

#[cfg(feature = "accesskit")]
if enabled
&& sense.click
&& sense.senses_click()
&& input.has_accesskit_action_request(id, accesskit::Action::Click)
{
res.fake_primary_click = true;
res.flags.set(Flags::FAKE_PRIMARY_CLICKED, true);
}

if enabled && sense.click && Some(id) == viewport.interact_widgets.long_touched {
res.long_touched = true;
if enabled && sense.senses_click() && Some(id) == viewport.interact_widgets.long_touched
{
res.flags.set(Flags::LONG_TOUCHED, true);
}

let interaction = memory.interaction();

res.is_pointer_button_down_on = interaction.potential_click_id == Some(id)
|| interaction.potential_drag_id == Some(id);
res.flags.set(
Flags::IS_POINTER_BUTTON_DOWN_ON,
interaction.potential_click_id == Some(id)
|| interaction.potential_drag_id == Some(id),
);

if res.enabled {
res.hovered = viewport.interact_widgets.hovered.contains(&id);
res.dragged = Some(id) == viewport.interact_widgets.dragged;
res.drag_started = Some(id) == viewport.interact_widgets.drag_started;
res.drag_stopped = Some(id) == viewport.interact_widgets.drag_stopped;
if res.enabled() {
res.flags.set(
Flags::HOVERED,
viewport.interact_widgets.hovered.contains(&id),
);
res.flags.set(
Flags::DRAGGED,
Some(id) == viewport.interact_widgets.dragged,
);
res.flags.set(
Flags::DRAG_STARTED,
Some(id) == viewport.interact_widgets.drag_started,
);
res.flags.set(
Flags::DRAG_STOPPED,
Some(id) == viewport.interact_widgets.drag_stopped,
);
}

let clicked = Some(id) == viewport.interact_widgets.clicked;
Expand All @@ -1304,20 +1318,22 @@ impl Context {
any_press = true;
}
PointerEvent::Released { click, .. } => {
if enabled && sense.click && clicked && click.is_some() {
res.clicked = true;
if enabled && sense.senses_click() && clicked && click.is_some() {
res.flags.set(Flags::CLICKED, true);
}

res.is_pointer_button_down_on = false;
res.dragged = false;
res.flags.set(Flags::IS_POINTER_BUTTON_DOWN_ON, false);
res.flags.set(Flags::DRAGGED, false);
}
}
}

// is_pointer_button_down_on is false when released, but we want interact_pointer_pos
// to still work.
let is_interacted_with =
res.is_pointer_button_down_on || res.long_touched || clicked || res.drag_stopped;
let is_interacted_with = res.is_pointer_button_down_on()
|| res.long_touched()
|| clicked
|| res.drag_stopped();
if is_interacted_with {
res.interact_pointer_pos = input.pointer.interact_pos();
if let (Some(to_global), Some(pos)) = (
Expand All @@ -1330,10 +1346,10 @@ impl Context {

if input.pointer.any_down() && !is_interacted_with {
// We don't hover widgets while interacting with *other* widgets:
res.hovered = false;
res.flags.set(Flags::HOVERED, false);
}

let pointer_pressed_elsewhere = any_press && !res.hovered;
let pointer_pressed_elsewhere = any_press && !res.hovered();
if pointer_pressed_elsewhere && memory.has_focus(id) {
memory.surrender_focus(id);
}
Expand Down Expand Up @@ -2152,11 +2168,12 @@ impl Context {
let painter = Painter::new(self.clone(), *layer_id, Rect::EVERYTHING);
for rect in rects {
if rect.sense.interactive() {
let (color, text) = if rect.sense.click && rect.sense.drag {
let (color, text) = if rect.sense.senses_click() && rect.sense.senses_drag()
{
(Color32::from_rgb(0x88, 0, 0x88), "click+drag")
} else if rect.sense.click {
} else if rect.sense.senses_click() {
(Color32::from_rgb(0x88, 0, 0), "click")
} else if rect.sense.drag {
} else if rect.sense.senses_drag() {
(Color32::from_rgb(0, 0, 0x88), "drag")
} else {
// unreachable since we only show interactive
Expand Down Expand Up @@ -3131,7 +3148,7 @@ impl Context {
// TODO(emilk): `Sense::hover_highlight()`
let response =
ui.add(Label::new(RichText::new(text).monospace()).sense(Sense::click()));
if response.hovered && is_visible {
if response.hovered() && is_visible {
ui.ctx()
.debug_painter()
.debug_rect(area.rect(), Color32::RED, "");
Expand Down
Loading

0 comments on commit 3586041

Please sign in to comment.