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.

Sign up for GitHub

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

Jump to bottom

Refactor component ui registry to unify edit ui & display ui #6661

Open
Wumpf opened this issue Jun 27, 2024 · 1 comment
Open

Refactor component ui registry to unify edit ui & display ui #6661

Wumpf opened this issue Jun 27, 2024 · 1 comment
Labels
📺 re_viewer affects re_viewer itself ui concerns graphical user interface

Comments

@Wumpf
Copy link
Member

Wumpf commented Jun 27, 2024

Today, we have separate registration for edit & display ui on the "component ui registry". Whenever we want to display a value and there's no display ui, we fall back to a disabled version of the edit ui (and if none of those is present we use arrow based display).

The problem with this approach is that a lot of things look incorrect when shown as disabled. Most prominent & recent example:

We want to have these uis ideally in the same place and avoid an explosion of multiline/singleline edit/display ui methods!
Therefore, we should change the ui registration to methods that get a MaybeMutRef type which it can operate on independently. (do this for both single and multiline uis)

In the process we migrate existing display uis to the edit_ui crate (to be renamed). They live today in the data_ui crate even though they are strictly speaking not data uis.

Note that the display methods have very different signatures today and some may do additional queries (which are likely buggy in the new blueprint override/default world). Expect difficulties with:

  • Tensor display
  • ClassId / KeypointId
    (we need to find a way to pass through annotation context etc.!)

Once done, we can have a new DataUi implementation that pickts single/multiline component ui appropriately.
@Wumpf Wumpf added ui concerns graphical user interface 📺 re_viewer affects re_viewer itself labels Jun 27, 2024
@Wumpf Wumpf mentioned this issue Jun 27, 2024
Merged
5 tasks
Wumpf added a commit that referenced this issue Jun 27, 2024
@Wumpf
Reintroduce display color ui (#6663)
1a047fd 
### What

<img width="341" alt="image"
src="https://github.com/rerun-io/rerun/assets/1220815/267f8b80-cdf3-4792-a958-ae53c34275d8">

Reintroduce a display ui for color which was previously removed here:

https://github.com/rerun-io/rerun/pull/6645/files#diff-5426fd9a146d72f74d8f81c575b9c4fcf10098bdf08e573ed5f96eda7787110dL36

* Fixes #6658


Note that the new impl has to do its own deserialization because before
we tended to implement `DataUi` for individual components. Going
forward, we want to turn it around and implement a unified "componen_ui"
method per component and then have a blanket `DataUi` impl for
components (if needed). Details see
* #6661 


### 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/6663?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/6663?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/6663)
- [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`.
This was referenced Jul 1, 2024
Closed
Merged
emilk pushed a commit that referenced this issue Jul 4, 2024
@Wumpf
Bring back display ui for Name and Text components (#6764)
3a9ef1b 
### What

* Fixes the worst offender of
[#6661](#6661)


![Screenshot2024_07_04_141633](https://github.com/rerun-io/rerun/assets/1220815/e951e130-383d-4a1a-ad4c-02e15cb9ce9c)

### 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/6764?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/6764?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/6764)
- [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`.
@emilk emilk self-assigned this Jul 4, 2024
@abey79
Copy link
Member

abey79 commented Jul 4, 2024

As shortly discussed with @Wumpf, UiLayout remains a critically important piece for the display of data. Currently, edit UI assume UiLayout::List, but we could imagine a future where it would be nice to have edit UI that exploit the available space (e.g. a multiline editor when you select an override for a Text component—when that becomes possible).

Also, technically, Ui::stack() could be used to provide the same/similar context information as the current UiLayout argument, but it's unclear that migrating to this would be a net win.

@emilk emilk mentioned this issue Jul 4, 2024
Merged
5 tasks
@emilk emilk removed their assignment Jul 8, 2024
Wumpf pushed a commit that referenced this issue Jul 8, 2024
@emilk
Refactor edit_ui (#6778)
520e5d7 
### What
* Closes #6696
* Does a lot of the stuff in
#6661

This replaces the current edit UI:s with ones that take a `MaybeMutRef`,
forcing the callbacks to handle the case of non-editable values.

![Screenshot 2024-07-04 at 18 50
05](https://github.com/rerun-io/rerun/assets/1148717/9c06de01-1e6c-43b2-a086-1bbcd5a67b0b)


### 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/6778?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/6778?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/6778)
- [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`.
@Wumpf Wumpf mentioned this issue Jul 11, 2024
Merged
7 tasks
Wumpf added a commit that referenced this issue Jul 16, 2024
@Wumpf
New transform components: Translation3D & TransformMat3x3 (#6866)
e5d9535 
### What

* Part of #6831
* Followed by #6892

Starts the process of splitting up `Transform3D` into several
components.
Far from done overall, but establishes a lot of the new documentation &
test patterns for this effort.

This PR fully replaces the `TranslationAndMat3x3` variant & datatype and
puts `Translation3D` & `TransformMat3x3` into existence and to work.
It does not touch on anything directly related to out of tree transforms
and does not do away from the `Transform3D` component yet.

I added a new component edit/view ui for consistency:
<img width="532" alt="image"
src="https://github.com/rerun-io/rerun/assets/1220815/1e7122f0-bf27-4f50-bc23-2824c2607ab2">


<img width="552" alt="image"
src="https://github.com/rerun-io/rerun/assets/1220815/1b30af25-905f-49ed-952b-961350dfab1f">
Unfortunately, transform hierarchy doesn't get affected by overrides yet
(see #6743), which is why I had
to turn off editing itself for the moment. Also, we don't yet show
multiline on hover, so matrix3x3 inspection regressed a little bit for
the moment (part of the only partially solved
#6661)


### Checklist
* [x] pass `main` ci checks
* [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/6866?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/6866?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)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/6866)
- [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`.
@Wumpf Wumpf mentioned this issue Jul 24, 2024
Open
@Wumpf Wumpf mentioned this issue Aug 20, 2024
Merged
6 tasks
Wumpf added a commit that referenced this issue Aug 22, 2024
@Wumpf
Convert most remaining DataUI component implementations to componen…
abfe3df 
…t uis in `re_component_ui` (#7247)

### What

* Big chunk of #6661

what's left is quite compact but rather tricky. I also tried to capture
the status quo in some comments and do various cleanup to make it easier
for the next person stumbling into the treacherous world of drawing ui
for components!

### 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/7247?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/7247?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)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7247)
- [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 mentioned this issue Oct 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
📺 re_viewer affects re_viewer itself ui concerns graphical user interface
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@emilk @Wumpf @abey79