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

Closed
abey79 opened this issue May 7, 2024 · 0 comments · Fixed by #6491
Closed

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

abey79 opened this issue May 7, 2024 · 0 comments · Fixed by #6491
Assignees
Labels
ui concerns graphical user interface

Comments

@abey79
Copy link
Member

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
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`.
@abey79 abey79 self-assigned this Jun 4, 2024
abey79 added a commit that referenced this issue Jun 4, 2024
### What

- Fixes #6246
- Very tiny bit of #4569

This PR uses emilk/egui#4588 to replace the
`full_span` mechanism, which requires less boilerplate and makes it
available in many more places (including tooltips). The
`get_full_span()` function now lives in a `Ui` extension trait, where
the rest of `re_ui` should eventually migrate (#4569).

This PR also updates egui/egui_tiles/egui_commonmark to the latest
commits.

### 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/6491?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/6491?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/6491)
- [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`.
abey79 added a commit that referenced this issue Jun 4, 2024
)

### What

- Fixes #4161
- Follow up to #6297
- Follow up to #6325

This PR uses `list_item` for the component list in the entity path
selection panel and tooltip.

~~Blocked by emilk/egui#4471 to resolve the
tooltip "first frame flicker"~~

TODO:
- [x] ⚠️ full_span_scope panic on hover on spatial space view:
#6246


#### selection panel

before:

<img width="519" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/afa51449-d7bf-4c0c-9d57-0ac25fedcf9f">


after:

<img width="345" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/1b524065-f5c0-4834-83ab-bc6b5f83ec37">


*Note*: the hover behaviour is the biggest change here: now full span,
previously just on the left-column button

#### tooltip

before:

<img width="511" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/cecb2337-4124-43ed-8b24-b895e71c3b26">

after:
<img width="510" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/4adb5113-898d-4585-be56-876d83b7694a">


### 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/6309?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/6309?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/6309)
- [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

Successfully merging a pull request may close this issue.

1 participant