Skip to content

Use weaker version of ComponentDescriptor for indicators #9990

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

Merged
merged 4 commits into from
May 19, 2025

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented May 19, 2025

Related

was curious and poked a lil bit into the ci failure: took me a while to notice but the primary issue presented by the failing images is that it draws lines instead of points.

Which implies an issue with indicators (wtf why is that the only test failing then.. but sure usually there's no choice; have only a vague hunch about that :/). Looking at impl<A: Archetype> crate::ComponentBatch for GenericIndicatorComponentArray<A> indicators are never logged with their full descriptor, but are advertised as such.

So I quickly hacked the generated SeriesPoints::descriptor_indicator to be name only and sure enough that fixed the test. What to do about this knowledge? Uhm.. yeah... that is definitely a Monday problem 😄

What

Because indicators are about to go away soon (™️) anyways, making them weaker (qualification-wise) is probably the easiest choice.

Doing this in a separate PR to help with potential git bisect in the future.

Copy link

github-actions bot commented May 19, 2025

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

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

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

@grtlr grtlr added the exclude from changelog PRs with this won't show up in CHANGELOG.md label May 19, 2025
@grtlr grtlr requested a review from Wumpf May 19, 2025 08:06
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.

thanks for separating it out 👍

@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality codegen/idl labels May 19, 2025
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.

actually, let's be a little bit more thorough to avoid surprises during this week - we should do the same in C++ and Python

@grtlr
Copy link
Contributor Author

grtlr commented May 19, 2025

It looks like C++ is only using the component_name for indicators already. I think the Python code, will also do the correct thing via the Rust-bindings.

@grtlr grtlr merged commit c4b613c into main May 19, 2025
37 checks passed
@grtlr grtlr deleted the grtlr/weak-indicator-component-descriptor branch May 19, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen/idl exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants