-
Notifications
You must be signed in to change notification settings - Fork 376
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
Comments
A |
|
### 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`.
@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. => We’re decided unanimously for negative Radii == ui radius + utility methods (similar to re_renderer’s |
RadiusUi
to set ui based radius for points/lines etc., remove view default radius setting
With the introduction of per-view default components we're able to remove the old default line & point radii.
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
(orUiRadius
) component which can live side by side with regular Radius on affected archetypes. WheneverRadiusUi
is present, we pick it over regularRadius
.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:
Obsoletes:
Related to:
Discarded alternatives:
(this was a result of a meeting between @jleibs @emilk & @Wumpf + some additional thoughts that came up during writeup )
The text was updated successfully, but these errors were encountered: