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

Deprecate DisconnectedSpace archetype/component in favor of implicit invalid transforms #8459

Merged
merged 13 commits into from
Dec 16, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Dec 13, 2024

Related

What

See title. In detail:

  • RotationAxisAngle will no longer be ignored when axis is non-normalizable
  • deprecation of DisconnectedSpace
    • slight fixes to codegen for handling this smoothly
  • lots of doc updates
  • wherever we had constants for identity, we now also have one for invalid
    • python didn't have any at all so far, but I didn't want to add this in this iteration since generally constants there are a bit tricky
    • we should have more of those generally

Copy link

github-actions bot commented Dec 13, 2024

Latest documentation preview deployed successfully.

Result Commit Link
85d151d https://landing-bymn45d9e-rerun.vercel.app/docs

Note: This comment is updated whenever you push a commit.

@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🍏 primitives Relating to Rerun primitives include in changelog labels Dec 13, 2024
Copy link

github-actions bot commented Dec 13, 2024

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
85d151d https://rerun.io/viewer/pr/8459 +nightly +main

Note: This comment is updated whenever you push a commit.

@Wumpf Wumpf force-pushed the andreas/undefind-transform branch from 03258b3 to e580ed1 Compare December 13, 2024 17:04
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Now that we have tagged components, does this actually need to be a Transform-specific component?

This could be rerun.components.InvalidData.

I'm also now wondering about Invalid = True vs Valid = False.

rerun.components.ValidityMask would feel the most arrow-like from a concept perspective, though I recognize this opens a can of worm of wanting to start supporting valid = False for every visualizer.

@Wumpf
Copy link
Member Author

Wumpf commented Dec 14, 2024

Now that we have tagged components, does this actually need to be a Transform-specific component?
This could be rerun.components.InvalidData.

That would imply that we make this more of an infrastructure thing and many things can be flagged as InvalidData. Which I really don't want to get into. Also, I wouldn't say it's invalid data per se. You might still have some valid transforms active, you just want to disable it.

On that note: we could also dodge this entire thing completely by saying any of the invalid-transform states like full nulled mat3x3 has the same semantics. Today, some of these "invalid transforms" are explicitly ignored since it's more practical - specifically we speced rotation-axis-angle with a null axis to just be no-rotation since that's more convenient (and I actually already fixed it up here so that the same is true for null quaternions)

I'm also now wondering about Invalid = True vs Valid = False.

Either isn't great, but Valid=False is indeed mentally easier. Maybe best to go with an enum to force making this explicit. In that case We end up with DataValidity/TransformValidity/ValidTransform == false/true

.... but overall I really don't want to make this any more complicated than is. The whole point of doing this now is to make a presumably rarely used feature not be as much in the way of performance enhancement & simplifying (!) refactors. That's why I wanted to deprecate DisconnectedSpace asap (something we already agreed upon) so I have simpler logic in the release after

@jleibs
Copy link
Member

jleibs commented Dec 14, 2024

You might still have some valid transforms active, you just want to disable it.

This is a great observation, which makes me realize any of the names related to Validity aren't quite correct.

What we really want to say then is simply "Don't use this transform" (which is also more Transform-specific concept than validity)

Other words: "Enable/Disable", "Apply", "Connect/Disconnect"... 🤔

@Wumpf Wumpf changed the title Introduce InvalidTransform component & deprecate DisconnectedSpace archetype/component Deprecate DisconnectedSpace archetype/component in favor of implicit invalid transforms Dec 16, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Dec 16, 2024

Talking this out we came to the conclusion that for the moment we won't introduce a new component and instead encourage invalid transforms like scale==0/matrix==0 etc.
Making this a much smaller change!

@Wumpf Wumpf marked this pull request as ready for review December 16, 2024 16:58
@Wumpf Wumpf requested a review from jleibs December 16, 2024 16:58
@Wumpf Wumpf merged commit 6288d04 into main Dec 16, 2024
36 of 37 checks passed
@Wumpf Wumpf deleted the andreas/undefind-transform branch December 16, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🍏 primitives Relating to Rerun primitives 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate DisconnectedSpace, replace with pure transform marker type InvalidTransform/UndefinedTransform (tbd)
2 participants