-
Notifications
You must be signed in to change notification settings - Fork 334
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
Add Ellipsoids3D
archetype.
#6853
Conversation
Currently, the visualizer is inefficient, in that it pipes everything through the `LineDrawableBuilder` every frame unnecessarily. However, this should not be worse than if the user were to create a similar number of lines directly.
Needs screenshot and instructions on how to test! |
I've added a screenshot to the PR description. One is still needed in the archetype docs, but I assume I don't actually have permissions to run For testing, |
What pull request labels are appropriate to satisfy the labels check? Should it be labeled with all three APIs since it touches all of them? What about the rendering aspect? Is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me!
crates/store/re_types/definitions/rerun/archetypes/ellipsoids.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/archetypes/ellipsoids.fbs
Outdated
Show resolved
Hide resolved
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Includes updating from `macaw::` to `re_math::`.
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
The snippets test is failing:
EDIT: fixed. |
The remaining unchecked checkbox says to test in the web viewer, but following the links says “pr/6853 is not a valid web viewer version”. Should I continue regardless? Does that need to be authorized? |
Sadly our CI for contributors doesn't build a web viewer. You can test it locally with |
### What Should fix link checker error in CI. Introduced in #6853 ### 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/{{pr.number}}?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/{{pr.number}}?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/{{pr.number}}) - [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`.
What
Implement an
Ellipsoids
archetype and visualizer. This may be used to render ellipsoids, or spheres wherePoints3D
is unsuitable because the actual boundary in space is significant. It is intended as a first step towards completing #1361, by adding ellipsoids and by sketching out a general mechanism for rendering procedurally-generated shapes, which will be reusable for cylinders and anything which needs many vertices to render.The archetype, extensions, etc. are largely forked off of
Boxes3d
.Unfinished parts / future work:
Currently, the spheres are always drawn as wireframes (with an automatically chosen mesh subdivision level). We will want to also support solid rendering (i.e. triangles in place of lines), but many use cases for spheres might want them to be transparent, which isn't yet properly supported in 3D space views (Draw order & (order independent) transparency #702), so I chose wireframe as the first version.
Currently, all of the wireframe lines are piped through the
LineDrawableBuilder
every frame. Instead, they should be drawn as instanced meshes; that will require implementing a new renderer that invokes the existing line shader with the different data layout. However, this current implementation should not be worse than if the user were to create a similar number of lines directly.The sphere mesh needs a choice of subdivision level. Currently, it is automatically derived from the size of the sphere in the scene, which may be inappropriate depending on the scale of scene units or the specific application. I'd like feedback on the best way to handle this. Just add another component, perhaps kind of like the
Radius
component?I‘m not sure what the best way to handle the (future) errors in generating meshes is. Should it return a
SpaceViewSystemExecutionError
(which hides unrelated data), just draw nothing and continue (which could lead to silent missing data), or something else?Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.