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

Fix bug when joining cleared optional components #3726

Merged
merged 4 commits into from Oct 9, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Oct 6, 2023

What

Resolves the second issue identified from:

Even with the fix from:

We could still reproduce an error with code such as:

"""Log a simple line strip."""
import rerun as rr

rr.init("rerun_example_line_strip3d", spawn=True)

points = [
    [0, 0, 0],
    [0, 0, 1],
    [1, 0, 0],
    [1, 0, 1],
    [1, 1, 0],
    [1, 1, 1],
    [0, 1, 0],
    [0, 1, 1],
]

rr.log("strip", rr.LineStrips3D([points], radii=[]))

Which would still display no lines.

It turns out the single-row joining iterator introduced a bug when a component is cleared. This yields an empty cell for the component, but still passes the key-based check that was being done, and then subsequently yields no items.

This PR adds a unit-test reproing the issue, a debug_assertion where we were previously violating an assumption, and now guards against the condition.

Checklist

@jleibs jleibs added 📺 re_viewer affects re_viewer itself 🦟 regression A thing that used to work in an earlier release labels Oct 6, 2023
@jleibs jleibs added this to the 0.9.1 milestone Oct 6, 2023
@jleibs jleibs marked this pull request as ready for review October 6, 2023 18:16
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.

Good catch, but I'm nervous we still have the bug for splats!

Comment on lines 346 to 348
let is_empty = component.map_or(true, |c| c.is_empty());

if let (Some(component), false) = (component, is_empty) {
Copy link
Member

Choose a reason for hiding this comment

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

Please motivate this with a comment, i.e. why only non-empty components?

And does the code handle splat components (length=1) correctly?

@@ -357,6 +359,15 @@ impl<A: Archetype> ArchetypeView<A> {
// - If the data isn't the same, the cost of the comparison will be dwarfed by the cost
// of the join anyway.
if self.required_comp().instance_keys == component.instance_keys {
// This fast iterator is assumed to match the length of the primary component
// We shouldn't hit this if since the store should enforce matched lengths for
// non-empty components, and the outer if-guard should keep us from reaching this
Copy link
Member

Choose a reason for hiding this comment

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

again, I don't think this is true for splats, e.g. Points3D(positions=[a,b,c,d], colors=[rgba])

Copy link
Member Author

Choose a reason for hiding this comment

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

Splats are currently split into their own rows. See: #1893 -- adding a TODO linking to that issue though.

crates/re_query/tests/archetype_visit_tests.rs Outdated Show resolved Hide resolved
@jleibs
Copy link
Member Author

jleibs commented Oct 7, 2023

Good call on splats. We technically don't have a problem (yet) because splats are currently logged to a separate row, but I will add a unit test to make sure we don't regress on that front when we change that assumption.

@jleibs jleibs merged commit 9d5feae into main Oct 9, 2023
30 checks passed
@jleibs jleibs deleted the jleibs/fix_join_cleared_components branch October 9, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 📺 re_viewer affects re_viewer itself 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants