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

All UI containers should ideally define full span ranges themselves #6246

Open
abey79 opened this issue May 7, 2024 · 1 comment
Open

All UI containers should ideally define full span ranges themselves #6246

abey79 opened this issue May 7, 2024 · 1 comment
Labels
ui concerns graphical user interface

Comments

@abey79
Copy link
Contributor

abey79 commented May 7, 2024

Context

#6211 introduced a mechanism to save "full span ranges" on a scoped stack. These range define the total width of the container, excluding any margin, that should be used by "full span" widget, ie widget which draw highlighting and other decoration all the way to the container margin, potentially outside of ui.max_rect().

For some containers, such as side panels, it's easy to define the full_span_scope(). For others, it's tricker, e.g. popup menu (emilk/egui#4452). Work around exists but can be annoying. And then there is the case of hover tooltip.

Hover tooltips

Hover tooltips are created using egui::Response::on_hover_ui and related, which is all over the place in the code base. When delegating to e.g. DataUi, we're facing two solution, neither of which is agreeable:

  • Every single call to on_hover_ui which may indirectly call into "full span" code (eg through DataUi) should wrap its content with full_span_scope. Error prone and verbose/repetitive.
  • DataUi implementation, or indeed any ui code that may end up in a tooltip, should ensure they create a full_span_scope. In the case of DataUi, this means testing for verbosity == UiVerbosity::Reduced (which is problematic in itself, see Change UiVerbosity to UiLayout and tighten up related implementations #6245). In other case, it might be very tricky to even know if we're in a tooltip context.

This is further aggravated by the fact that it's actually hard to devise what the actual full span is (emilk/egui#4452), leading to bad UI such as this:

image

Solution

  • Don't use full span widgets in popup/tooltip/etc..
  • Fallback to ui.clip_rect(). This is currently done in release mode (+ warning). In debug build, we debug_assert that a full-span scope is there when needed. We could remove these checks and warning and make the clip-rect fallback official.
    • This would require egui to always set the clip rectangle for its container, e.g. hover ui.
    • By "chance", this would fix the case of interactive(false) list items in hover ui. The clip rect is not set, but the use of it by list item (background -> not drawn when not interactive; interaction -> impossible by virtue of the hover tooltip being, by construction, impossible to interact with). Still not a great situation by any measure.
  • Build the full-span mechanism right into egui (Sizing-pass flag emilk/egui#4535).

EDIT: we're going for the egui way:

Related

@abey79 abey79 added the ui concerns graphical user interface label May 7, 2024
@emilk
Copy link
Member

emilk commented May 7, 2024

I created an egui issue to improve the sizing of tooltips:

With that, I think this should be solvable in user space.

abey79 added a commit that referenced this issue May 8, 2024
### What

Migrate all widgets to the "full span scope" mechanism introduced in
#6211, including the legacy `ListItem`. This completes the migration
away from the clip rect hack, but does highlight a number of issues and
improvements:
- #6245 
- #6246

Showcase of `full_span_scope()` nestability:

<img width="324" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/be8e72b1-7f1d-4533-a456-870a8c51572d">
<br/> <br/>


- Fixes #6156

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6248?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6248?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6248)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
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

No branches or pull requests

2 participants