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

Make tagged columnar updates work with mono-components too #8769

Merged
merged 4 commits into from
Jan 22, 2025

Conversation

teh-cmc
Copy link
Member

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

  • Generate plural methods even for mono-components, so they can be used with send_columns.
  • Update scalar_send_columns snippet to use new columnar APIs.
const STEPS: i64 = 64;

let times = TimeColumn::new_sequence("step", 0..STEPS);
let scalars = (0..STEPS).map(|step| (step as f64 / 10.0).sin());

rec.send_columns_v2(
    "scalars",
    [times],
    rerun::Scalar::update_fields()
        .with_many_scalar(scalars)
        .columns(std::iter::repeat(1).take(STEPS as _))?
        .filter(|column| !column.descriptor.component_name.contains("Indicator")),
)?;

@teh-cmc teh-cmc added enhancement New feature or request 🦀 Rust API Rust logging API exclude from changelog PRs with this won't show up in CHANGELOG.md 🔩 data model labels Jan 21, 2025
Copy link

github-actions bot commented Jan 21, 2025

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

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

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

rerun::Scalar::update_fields()
.with_many_scalar(scalars)
.columns(std::iter::repeat(1).take(STEPS as _))?
.filter(|column| !column.descriptor.component_name.contains("Indicator")),
Copy link
Member Author

Choose a reason for hiding this comment

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

The other languages do not log an indicator here, and for good reasons: indicators in a scalar chunk have huge overhead, and are completely useless as far as I can tell (i.e. they aren't needed for heuristics).

In a follow-up, I will introduce a hardcoded exception so that Scalar::columns just doesn't generate it.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 21, 2025

At least half of the generated code seems to be for blueprint archetypes. We don't need it, I'll add a hardcoded exception to ignore blueprint archetypes tomorrow.

@Wumpf Wumpf self-requested a review January 22, 2025 08:22
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.

lgtm!
We should be able to use this on the image_send_column example. Can you have a look there as well?

@@ -1876,13 +1876,35 @@ fn quote_builder_from_obj(reporter: &Reporter, objects: &Objects, obj: &Object)
}
}
} else {
let quoted_many = obj.scope().is_none().then(|| {
let method_name_many = format_ident!("with_many_{field_name}");
Copy link
Member

Choose a reason for hiding this comment

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

with_many_scalar is a bit bumpy 🤔
Can't come up with anything better either though. English's 's' pluralization is too imperfect and we don't want to change field names...
Well at least it stands out and the doc string then tells you what's up 👍

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 22, 2025

lgtm! We should be able to use this on the image_send_column example. Can you have a look there as well?

Ah? will do in the upcoming final clean up pass that removes all the legacy.

@teh-cmc teh-cmc merged commit 85ba0dc into main Jan 22, 2025
37 checks passed
@teh-cmc teh-cmc deleted the cmc/tagged_column_updates_rust_forced_plural branch January 22, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔩 data model enhancement New feature or request exclude from changelog PRs with this won't show up in CHANGELOG.md 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants