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

Make Transform3D always log all its components #6909

Closed
Wumpf opened this issue Jul 16, 2024 · 2 comments · Fixed by #7111
Closed

Make Transform3D always log all its components #6909

Wumpf opened this issue Jul 16, 2024 · 2 comments · Fixed by #7111
Assignees
Labels
🍏 primitives Relating to Rerun primitives

Comments

@Wumpf
Copy link
Member

Wumpf commented Jul 16, 2024

Why is this important? Example:

  • log just a translation (via Transform3D archetypes)
  • log just a rotation (via Transform3D archetypes) -> Is the translation still there? We think it shouldn't be
@emilk
Copy link
Member

emilk commented Aug 5, 2024

This will add noise to the UI, and also add some logging CPU overhead, but I suggest we wait with fixing both those problems until we tackle all of #3381

@Wumpf Wumpf removed their assignment Aug 7, 2024
@emilk emilk self-assigned this Aug 8, 2024
@emilk
Copy link
Member

emilk commented Aug 8, 2024

An alternative approach to this is to do a special query when querying transforms. It would go something like this:

  • Query all transform components (Translation3D, RotationAxisAngle, RotationQuat, Scale3D, TransformMat3x3).
  • Find the latest RowId of all of them.
  • Ignore all components with a different RowId.

This would means each new log call of any of these transforms would effectively reset the other ones.

The advantage of this idea is that we don't have to do the work in this PR (always log all components to reset any previously logged one).

However, it is a special snow-flake querying model, just for Transform. I would need to be implemented in multiple places (transform hierarchy, selection view, table view, …) and be explained to the user

We also want to support logging transform components without using the Transform3D archetype.
For instance, a user should realistically be able to do this:

rr.log("planet", rr.Position3D(…));
rr.log("planet", rr.Colors(…));
rr.log("planet", rr.Translation3D(…));
rr.log("planet", rr.Rotation3D(…));

It would be surprising to the user if the translation was ignored in the viewer.

There are more advanced queries that could solve this, e.g. "if the transform components are non-overlapping (like translation and rotation), then still use both…" but this now becomes a complex decision tree that again needs to be replicated and explained to users.

In contrast, the approach argued by this PR:

emilk added a commit that referenced this issue Aug 9, 2024
### What
* Closes #6909
* First steps on the road to
#3381
* Creates a new issue: #7117

Best reviewed ignoring whitespace changes.

The serializers for `Transform3D` and `LeafTransform3D` will now always
write every component, effectively clearing all previously logged
values.

`Transform3D` uses `Option<C>` for each component, treating it as a
single-element list component list where `None` is empty. If we ever
want to support updating some components using the transform (as
outlined in #3381) we would need
to turn this into a tristate, i.e. something like `NoneOrZeroOrOne<C>`.

`LeafTransform3D` uses `Option<Vec<C>>` for its components, but has no
interface yet for logging an update.

Both `Transform3D` and `LeafTransform3D` has a new `::clear()`
constructor in Rust, that replaced both `::new()` and `::default()`.
This is for added clarity, and to make the transition easier if we ever
add an `::update()` constructor, as per
#3381


![image](https://github.com/user-attachments/assets/588224b8-daec-463d-b238-8263f578ae19)

### 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/7111?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/7111?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/7111)
- [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`.

---------

Co-authored-by: Andreas Reich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍏 primitives Relating to Rerun primitives
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants