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

Specify, test, and fix all broken AsComponents<>ComponentBatch interactions from blanket impls #8591

Merged
merged 9 commits into from
Jan 7, 2025

Conversation

teh-cmc
Copy link
Member

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

Commit by commit is probably best.

Turns out cascading blanket impls on &dyn T are hard to reason about and don't do what you expect 🤪:

  • A single instance (i.e. C: Component) should never be allowed to cast straight to a collection of batches (i.e. AsComponents).
    This is the source of all the bugs. We need to enforce that this goes through a ComponentBatch first, which in turn will be auto-cast to AsComponents.
  • Force-casting a simple batch to a collection is now verboten. It still works where it counts though (i.e. rec.log).
    This was extremely dangerous:
    let labels = [
        MyLabel("a".into()),
        MyLabel("b".into()),
        MyLabel("c".into()),
    ];
    
    // This does not do what you think it does (you're logging 3 batches of the
    // same component, with one instance each!!!).
    rec.log("labels", &labels as &dyn crate::AsComponents).unwrap();

@teh-cmc teh-cmc added 🪳 bug Something isn't working 🦀 Rust API Rust logging API include in changelog labels Jan 6, 2025
@teh-cmc teh-cmc changed the title Specify, test, and fix all broken AsComponents/ComponentBatch` interactions from blanket impls Specify, test, and fix all broken AsComponents<>ComponentBatch interactions from blanket impls Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

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

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

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

@teh-cmc teh-cmc marked this pull request as ready for review January 6, 2025 18:08
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 6, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Jan 6, 2025

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

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 7, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Jan 7, 2025

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

@Wumpf Wumpf self-requested a review January 7, 2025 09:57
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

oof tough! thanks for unraveling this knot!

@teh-cmc teh-cmc force-pushed the cmc/logging_traits_disaster branch from 729cd9a to d82fc66 Compare January 7, 2025 12:00
@teh-cmc teh-cmc merged commit 31787ec into main Jan 7, 2025
30 of 31 checks passed
@teh-cmc teh-cmc deleted the cmc/logging_traits_disaster branch January 7, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working include in changelog 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants