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

Use ListItem in blueprint tree UI #3118

Merged
merged 8 commits into from
Aug 29, 2023
Merged

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Aug 28, 2023

What

This PR back-ports ListItem into the blueprint tree UI. This includes multiple fixes and additions to ListItem itself.

Note that this PR strictly replicates the current behaviour, which has several issues:

  • containers look inactive although they are active
  • hidden elements look inactive although they are active
  • cannot unhide one children of a hidden group
  • hiding a container doesn't dim its children
  • more generally, it would be nice to improve the visual distinction between containers, space views, data group and data item (i.e. if you know what a space view vs. data group is, you'll find them, but the tree UI does very little to actually teach that distinction)

Fixes:

image

Checklist

@abey79 abey79 added the ui concerns graphical user interface label Aug 28, 2023
@abey79 abey79 requested a review from martenbjork August 28, 2023 08:41
@emilk
Copy link
Member

emilk commented Aug 29, 2023

There is some rather annoying flickering going on:

flickering

@abey79
Copy link
Member Author

abey79 commented Aug 29, 2023

There is some rather annoying flickering going on:

I can reproduce on all 3 major browsers but not on native 🤔

Edit: nvm I can reproduce on native as well with a "slow" example (e.g. opf)

Edit2: egui::Response::highlight() is partly responsible here, inducing a frame lag. I made changes to override hovered state "immediately" instead for the blueprint tree.

abey79 added 5 commits August 29, 2023 11:09
Instead, support for hover overriding is built in ListItem. This reduces the frame lag and related flicker.
# Conflicts:
#	crates/re_viewport/src/viewport_blueprint_ui.rs
let response = ListItem::new(ctx.re_ui, space_view.display_name.clone())
.selected(ctx.selection().contains(&item))
.subdued(!visible)
.override_hover(is_item_hovered)
Copy link
Member

@emilk emilk Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can come up with a more elegant solution to this. It's not obvious how though.

The best I can come up with is this: produce the Id of the list item first, then call egui::Context::highlight_widget (if needed) before calling ListItem::new(…).id(id).

let list_item_id = ui.id().with(space_view.id);
if is_item_hovered {
    ui.ctx().highlight_widget(list_item_id);
}

let response = ListItem::new(ctx.re_ui, space_view.display_name.clone())
    .id(list_item_id)

(…though this also requires a fix to egui to make highlight_widget take effect the same frame).


Alternative fix: add something to egui along the lines of ui.highlight_next_item(), but I don't really like that sort of statefullness (and it might accidentally affect only part of the next widget, e.g. the triangle).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this much either, but I'm in favour of punting on that.

My rationale is that piggy-backing on the hover state to show relations between disjoints UI elements related to the same "thing" is just wrong in the first place. With the new ListItem, it's visually too intrusive for my taste (I have that noted in #3120):
image

In the long run, we will instead need a new kind of dedicated highlighting, which will require adapting the Selection Panel UI as well. This new state will be unknown to egui, so it's probably not the right place to hack now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - though we could also make highlight something separate in egui from hovered. But I agree on the punt part

crates/re_ui/src/list_item.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/viewport_blueprint_ui.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants