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

Expose ui based radii, remove view default radius setting #6540

Closed
Wumpf opened this issue Jun 11, 2024 · 3 comments · Fixed by #6678
Closed

Expose ui based radii, remove view default radius setting #6540

Wumpf opened this issue Jun 11, 2024 · 3 comments · Fixed by #6678
Assignees
Labels
🟦 blueprint The data that defines our UI 🍏 primitives Relating to Rerun primitives 🔺 re_renderer affects re_renderer itself
Milestone

Comments

@Wumpf
Copy link
Member

Wumpf commented Jun 11, 2024

With the introduction of per-view default components we're able to remove the old default line & point radii.
image

However, this ui allowed you to set radii in ui points, something which is not possible today from components.
We decided to solve this by introducing a new RadiusUi (or UiRadius) component which can live side by side with regular Radius on affected archetypes. Whenever RadiusUi is present, we pick it over regular Radius.

This has some (simplifying!) ripple effects in the renderer: point & line batches would have a flag to determine whether they are world or ui sized. We drop support for mixing ui & world sizes on the same batch/entity.

Note that this fits a fallback interaction case that we only speculated on so far: Naturally both RadiusUi & Radius have a fallback (all components have!), but visualizers need to know if any of them was a fallback so they can pick the other.

Tbd:

Affected archetypes:

  • arrows2d/3d
  • boxes2d/3d
  • line_strips2d/3d
  • points2d/3d (including key point connections)

Obsoletes:

Related to:

Discarded alternatives:

  • archetype variants which determine the semantics
    • Point vs Sphere, Line vs UiLine?, etc.
    • Con: Tricky naming, hard to switch back and forth right now
  • special datatype that mirrors what we do on re_renderer
    • Con: Requires user to bulk negate values if they want ui units
  • additional enum value to determine semantic of radius component
    • Con: Radius component no longer has full sematantic

(this was a result of a meeting between @jleibs @emilk & @Wumpf + some additional thoughts that came up during writeup )

@Wumpf Wumpf added 🔺 re_renderer affects re_renderer itself 🍏 primitives Relating to Rerun primitives 🟦 blueprint The data that defines our UI labels Jun 11, 2024
@Wumpf Wumpf added this to the 0.17 milestone Jun 11, 2024
@emilk
Copy link
Member

emilk commented Jun 24, 2024

A RadiusUi component should replace MarkerSize (which btw hasn't documented if it is a radius or a width, and consequently the code treats it differently in different places)

@emilk
Copy link
Member

emilk commented Jun 24, 2024

RadiusUi could potentially replace StrokeWidth too

emilk added a commit that referenced this issue Jun 25, 2024
### What
* Part of #6540

The existing code was using the same field to mean different things at
different times. I have no idea if the original code was buggy, since it
didn't document expected behavior.

### 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/6630?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/6630?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/6630)
- [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 self-assigned this Jun 27, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Jun 27, 2024

@jleibs and me rolled up the case again and looked at the pros and cons in the light of the new features that landed since we talked last about this two weeks ago.

One major issue we identified with the so far proposed side-by-side RadiusUi + Radius was that it sets an awkward precedence in the ui where you can’t tell whether a component participates in visualizing a thing - one of the problems that the new ui was supposed to tackle. This could be a systemic “use either or” thing, but it seemed rash to extend the system in such a way at this point.
The many-archetypes + RadiusUi component proposal has some merit - more than originally because switching visualizers is easy now - but creates a flood of code duplication on all sides.
A subproposal came up to only duplicate Visualizers, but that would break the 1:1 archetype visualizer relationship we wanted to gravitate towards
The enum proposal (single radius but extra enum to interpret it) was still discarded on grounds of sidestepping semantics, but there was some positive leaning toward it in the, ehrm, group.
Which leaves us with “just expose our internal data as is” == negative Radius implying ui type as well as making radius a union. Jeremy strongly supported the union idea, but we identified some practical issues with it upon processing the data (none of them unsurmountable)
We figured eventually that the union proposal is almost identical to exposing our internal data as-is: Negative radii are interpreted differently.
Previously, when this came up we were afraid of the user having to do large bulk operations on this, but we concluded that this is rarely the case: Having radius differ over the instances (individual points, individual line segments) is very rare and when it happens it is likely because there is a world size correlation!

=> We’re decided unanimously for negative Radii == ui radius + utility methods (similar to re_renderer’s Size type) + custom ui (in order to show semantic, not +/- signs)
This also happens to be the only proposal that we can implement relaistically for 0.17 as it minimal form requires almost no changes (docs + remove warnings + a bit of ui)

@Wumpf Wumpf changed the title Introduce RadiusUi to set ui based radius for points/lines etc., remove view default radius setting Expose ui based radii, remove view default radius setting Jun 27, 2024
@Wumpf Wumpf closed this as completed in 29eb75a Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI 🍏 primitives Relating to Rerun primitives 🔺 re_renderer affects re_renderer itself
Projects
None yet
2 participants