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

New transform components: Translation3D & TransformMat3x3 #6866

Merged
merged 27 commits into from
Jul 16, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jul 11, 2024

What

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:
image

image Unfortunately, transform hierarchy doesn't get affected by overrides yet (see https://github.com//issues/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 https://github.com//issues/6661)

Checklist

  • pass main ci checks
  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality 🍏 primitives Relating to Rerun primitives include in changelog 🪵 Log-API Affects the user-facing API for all languages labels Jul 11, 2024
Copy link

github-actions bot commented Jul 11, 2024

Deployed docs

Commit Link
fe140e2 https://landing-afyf593w4-rerun.vercel.app/docs

@Wumpf Wumpf marked this pull request as draft July 11, 2024 14:45
@Wumpf Wumpf force-pushed the andreas/translation-and-transformmat3-components branch from 28105bb to 7100140 Compare July 11, 2024 14:52
@Wumpf
Copy link
Member Author

Wumpf commented Jul 15, 2024

@rerun-bot full-check

Copy link

Started a full build: https://github.com/rerun-io/rerun/actions/runs/9937621641

@Wumpf Wumpf marked this pull request as ready for review July 15, 2024 10:27
@Wumpf
Copy link
Member Author

Wumpf commented Jul 15, 2024

@rerun-bot full-check

Copy link

Started a full build: https://github.com/rerun-io/rerun/actions/runs/9941007196

@Wumpf Wumpf added the do-not-merge Do not merge this PR label Jul 15, 2024
@Wumpf Wumpf mentioned this pull request Jul 15, 2024
7 tasks
@jleibs jleibs self-requested a review July 15, 2024 17:51
Comment on lines +12 to +13
/// E.g. if both a 4x4 matrix with a translation and a translation vector are present,
/// the matrix is applied first, then the translation vector on top.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are more and more establishing an alternative pattern of elsewhere of "if X is set, then Y and Z are ignored, with a warning". I still wonder if this might be the better thing to do with mat4x4 and mat3x3+transform.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a bad point, but the exclusion sets are arguably more complicated than including everything 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclusion sets are arguably more complicated than including everything

I agree with this. The new design is conceptually very simple: we have a bunch of different transform components, and we have a well-defined order in which they are applied.

@Wumpf Wumpf removed the do-not-merge Do not merge this PR label Jul 16, 2024
@Wumpf Wumpf merged commit e5d9535 into main Jul 16, 2024
73 of 77 checks passed
@Wumpf Wumpf deleted the andreas/translation-and-transformmat3-components branch July 16, 2024 08:01
Wumpf added a commit that referenced this pull request Jul 16, 2024
### What

* Part of #6831
* Based on #6866

Introduces a new `Scale3D` component that lives directly on the
`Transform3D` archetype.
The `Scale3D` _datatype_ which is part of `TranslationRotationScale3D`
is still around, but will be removed in a subsequent PR.

Additionally, reversed order of how transform components. Rationale:
"translation, rotation, scale" is a common way of expressing a simple
transform. What is meant is that we first scale an object, then rotate
it and _then_ translate it.

Commit by commit review possible.

### Checklist
* [x] pass `main` ci
* [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/6892?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/6892?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/6892)
- [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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🪵 Log-API Affects the user-facing API for all languages 🍏 primitives Relating to Rerun primitives 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants