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

Implement AsComponents for tuples. #4627

Closed
wants to merge 2 commits into from
Closed

Conversation

kpreid
Copy link
Collaborator

@kpreid kpreid commented Dec 31, 2023

What

This allows preparing a set of multiple archetypes (e.g. Transform3D plus something else) before logging them all at the same entity path, rather than having to write multiple RecordingStream::log() calls.

However, this PR contains no tests yet, because I wasn't sure where to put them or in what form to write them. Ideally, I'd like to write a test that validates that stream.log(path, (a, b)) produces the same data as stream.log(path, a); stream.log(path, b), but I'm unclear on where to place a test of that type. There don't seem to be existing tests that are at a sufficiently high layer (contains RecordingStream) but which aren't example-application-shaped.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (not applicable)
  • I have tested the web demo (not applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

This allows preparing a set of multiple existing bundles/archetypes
(e.g. `Transform3D` plus something else) before logging them all at the
same entity path, rather than having to write multiple
`RecordingStream::log()` calls or creating a custom bundle.
@abey79 abey79 added the 🦀 Rust API Rust logging API label Dec 31, 2023
@emilk
Copy link
Member

emilk commented Jan 2, 2024

Nice idea!

Note that what you are after is very similar to log_component_batches. In fact, log more or less just calls log_component_batches with x.as_component_batches().iter().map(|b| b.as_ref()).

@kpreid
Copy link
Collaborator Author

kpreid commented Jan 2, 2024

Note that what you are after is very similar to log_component_batches.

That's good to know. I think it's still worth doing this, since log_component_batches requires more code to be used, whereas this is implicit (and, in my opinion, it's idiomatic Rust to make more things “just work” via trait implementations as long as there's already a trait involved and the implementations make sense for the trait).

Do you have any advice on where to put, and how to structure, tests for this code?

@teh-cmc
Copy link
Member

teh-cmc commented Jan 3, 2024

My 2 cents: exposing a generic N-tuple implementation for this is very footgun-ny: it's very common for different archetypes to share a subset of underlying components, and with this implementation they would either silently overwrite each other or dynamically fail at runtime depending on the data.

You could log each tuple entry in its own row, and then you might get away with it because of how the query model works deep down, but 1) that's really playing with fire and 2) that's obfuscating the fact that you logged N events rather than 1.

Since the case of Transform3D is really the only case I can think of where you would benefit from this, you could have a special case for (A, Transform3D) and (Transform3D, A) (assuming rustc even lets you 😒); but again the obfuscation cost outweighs the gains, you'd be better off being explicit and logging the two events independently imo.

@kpreid
Copy link
Collaborator Author

kpreid commented Jan 3, 2024

it's very common for different archetypes to share a subset of underlying components, and with this implementation they would either silently overwrite each other or dynamically fail at runtime depending on the data.

Yes, and that's an existing problem this isn't making worse, or so I thought …

Since the case of Transform3D is really the only case I can think of where you would benefit from this

… but I wasn't aware that Transform3D was that exceptional. In that case, I agree that this is not a good design. I wish the API documentation more explicitly called out that it is not expected for logging multiple archetypes to work other than that specific case.

I'll think about what, if any, more narrow change would be suitable here.

@kpreid
Copy link
Collaborator Author

kpreid commented Jan 3, 2024

Further observation: Transform3D isn't the only archetype supported on the same entity — so is Pinhole (see #2568). Pinhole is certainly transform-like but that points at some general class of “and also a transform-like component” logging.

@Wumpf
Copy link
Member

Wumpf commented Apr 4, 2024

This has been lingering for a while now! I'm interpreting @teh-cmc 's concern on n-tuple impls as a veto and leave it at that.

Transform3D isn't the only archetype supported on the same entity

correct! The more accurate statement is that any combination of archetype of non-overlapping components makes sense. Archetypes in Rerun are really just a vehicle of grouping components, in the end it's all about what components are logged and what matches the viewer can derive from that. Additionally, archetypes also log a marker component that the viewer uses to decide what visualizers to active; right now this is fairly opaque and can't be configured much, but we're getting closer to making this transparent and configurable.

@Wumpf Wumpf closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants