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

Allow archetypes to clear and/or update existing components #3381

Open
emilk opened this issue Sep 20, 2023 · 6 comments
Open

Allow archetypes to clear and/or update existing components #3381

emilk opened this issue Sep 20, 2023 · 6 comments
Labels
codegen/idl enhancement New feature or request 🪵 Log & send APIs Affects the user-facing API for all languages ⛃ re_datastore affects the datastore itself
Milestone

Comments

@emilk
Copy link
Member

emilk commented Sep 20, 2023

Backstory

What should the following code do:

log("points", Points3D::new(positions).with_colors());log("points", Points3D::new(new_positions));

Should the new positions inherit the colors of the previous log call?

That would be quite non-intuitive.

Related to this: how does one update just the colors of a point cloud?

Design

We've all converged on the following API:

// We do NOT implement `Default` for archetypes.
// users need to explcitly use `clear` or `update`
struct Points3D {
   positions: Optional<Vec<Vec3>>,
   color: Optional<Vec<Color>>,
   radii: Optional<Vec<f32>>,
}

impl Points3D {
    /// Clear all components of this archetype to the empty list.
    fn clear() -> Self {
        Self {
            positions: Some(vec![]),
            color: Some(vec![]),
            radii: Some(vec![]),
        }
    }
    
    /// Create a new `Point3d` point cloud with some positions,
    /// and with emtpy lists for all other archetypes.
    fn new(positions: Vec<Vec3>) -> Self {
        Self {
            positions: Some(positions),
            ..Self::clear()
        }
    }
    
    /// An empty `Point3d` point cloud, with `None` for each component.
    fn update() -> Self {
        Self {
            positions: None,
            color: None,
            radii: None,
        }
    }
}

fn main() {
    // Log a point cloud:
    log("points", Points3D::new().with_colors());
    
    log("points", Clear{}); // will clear all components of all archetypes
    log("points", Points3D::clear()); // will clear this archetype of the entity
    log("points", Points3D::new(positions).with_colors()); // overwrites everything
    log("points", Points3D::update().with_colors()); // overwrites only colors

    // This is also possible, though a somewhat weird way to write it:
    log("points", Points3D::clear().with_colors()); // overwrites everything
}

Python:

log("points", Clear()); # will clear all components of all archetypes
log("points", Points3D.clear()); # will clear this archetype of the entity
log("points", Points3D(positions, colors=[…], radii=None); # overwrites everything, even radii
log("points", Points3D.update(colors=[…], radii=None)); # overwrites only colors and radii

Necessary changes

  • All components are Options, even the required ones
  • When querying, an empty lists should be treated the same as a missing components
  • In the UI we should probably hide empty components in the streams view

Component lists implications

A component list (e.g. radii) can have one of the four special values:

  • null/missing
  • 0 length ("empty")
  • 1 length ("splat")
  • N length (1<N)

null components aren't logged, but empty components are.
When querying the store, missing components are treated the same as empty components.

Related issues:

@emilk
Copy link
Member Author

emilk commented Sep 25, 2023

If this proves to be more complex than anticipated, then we'll push it to 0.10

@emilk
Copy link
Member Author

emilk commented Sep 25, 2023

Needs a follow-up where we hide empty components from the streams view

@emilk
Copy link
Member Author

emilk commented Sep 27, 2023

We need to think through how num_instances would work for an update. We could take the max-length of all its components, but that won't work if you're only logging splats. Perhaps we don't need num_instances for updates; it's really only used for the primary (e.g. the indicator).

@jleibs
Copy link
Member

jleibs commented Oct 10, 2023

This API should also handle specifying the InstanceKeys for the collection that's being updated.

@emilk emilk removed this from the 0.10 Polish (non-blocking) milestone Oct 23, 2023
@emilk emilk added this to the Triage milestone Nov 6, 2023
@teh-cmc teh-cmc added the 🪵 Log & send APIs Affects the user-facing API for all languages label Mar 15, 2024
@teh-cmc
Copy link
Member

teh-cmc commented Mar 15, 2024

Instance keys and num_instances are not a problem anymore (or won't be, soon).

@jleibs
Copy link
Member

jleibs commented Jul 16, 2024

@teh-cmc we were talking through this with respect to Transforms I realized this approach has the potential to cause some annoying performance issues for chunks.

In particular, every unused optional component will still require growing the offset array for each of the optional components by one element (in many cases, just a repeated 0). This is similar to the overhead of growing sparse unions (growing each child array), and strictly worse than the overhead of sparse unions (just growing one child array).

This suggests to me we're going to need some kind of chunk-specific optimization.

This also has an annoying ripple-effect on datafusion / dataframe UI of showing a bunch of empty / cleared (not nulled) columns all over the place.

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 <andreas@rerun.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen/idl enhancement New feature or request 🪵 Log & send APIs Affects the user-facing API for all languages ⛃ re_datastore affects the datastore itself
Projects
None yet
Development

No branches or pull requests

3 participants