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

Add new Transform3D partial updates snippet for all languages #8690

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 14, 2025

Adds some much needed snippet for partial updates on Transform3D.
Much needed because A) Transform3D has a long history of custom partial updatability hacks and B) users are very very likely to use that one.

@teh-cmc teh-cmc added 🔨 testing testing and benchmarks examples Issues relating to the Rerun examples include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Jan 14, 2025
Copy link

github-actions bot commented Jan 14, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
4deb6c4 https://rerun.io/viewer/pr/8690 +nightly +main

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

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Nice to have snippets for these, but I feel the code is maybe focusing a bit much on producing a nice/smooth animation, distracting from the actual meat of it (the partial updates). I'd like for our users to be able to open a snippet and right away find what they need, without too much extra fluff.

An easy fix would be to remove the step timeline (we already have log_tick, after all)

rec.set_time_sequence("step", step);
rec.log(
"box",
&rerun::Transform3D::clear_fields().with_axis_length(15.0),
Copy link
Member

Choose a reason for hiding this comment

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

Argueblt we should just demonste one thing here (clear_fields) and skip the with_axis_length

Copy link
Member Author

@teh-cmc teh-cmc Jan 15, 2025

Choose a reason for hiding this comment

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

purposefully showing both: people need to know they don't need to (and shouldn't) log multiple rows in order to clear all data and reset one or more specific fields (and similarly, we need to make sure in the roundtrips that this indeed results in a single row for all languages).

docs/snippets/snippets.toml Outdated Show resolved Hide resolved
@teh-cmc teh-cmc merged commit d747b6c into main Jan 15, 2025
37 checks passed
@teh-cmc teh-cmc deleted the cmc/transform_partial_snippets branch January 15, 2025 09:09
teh-cmc added a commit that referenced this pull request Jan 15, 2025
There really is not much to be said that wouldn't be made clearer by
just looking at the code.

Specifically:
* Look at the codegen changes.
* Now look at the changes to one of the generated archetypes (probably
`Transform3D`).
* Now look especially at the changes to `transform3d_ext`. That one's
important.
* Now look at how the different "partial updates" snippets evolved.

Overall I hate everything here, literally every single bit -- but that's
the best I managed to get out of the status quo without ridiculous
amounts of efforts.

* DNM: requires #8690 
* Closes #8582

---------

Co-authored-by: Antoine Beyeler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Issues relating to the Rerun examples include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 🔨 testing testing and benchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants