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

Detect, warn and gracefully handle corrupt cells in lookup_arrow #2055

Merged
merged 3 commits into from May 9, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented May 5, 2023

lookup_arrow called arrow's slice which is defined to panic on out of bounds slices.

example code that would create such an entity

    // And some points in front of the rectangle.
    MsgSender::new("2d_layering/points_between_top_and_middle")
        .with_timepoint(sim_time(1 as _))
        .with_component(
            &(0..256).map(|i| Point2D::new(...).collect::<Vec<_>>(),
        )?
        .with_component(&[DrawOrder(1.51)])?
        .send(rec_stream)?;

Checklist

PR Build Summary: https://build.rerun.io/pr/2055

`lookup_arrow` called arrow's `slice` which is defined to panic on out of bounds slices.

example code that would create such an entity

```rust
    // And some points in front of the rectangle.
    MsgSender::new("2d_layering/points_between_top_and_middle")
        .with_timepoint(sim_time(1 as _))
        .with_component(
            &(0..256).map(|i| Point2D::new(...).collect::<Vec<_>>(),
        )?
        .with_component(&[DrawOrder(1.51)])?
        .send(rec_stream)?;
```
@Wumpf Wumpf added 🔍 re_query affects re_query itself 💣 crash crash, deadlock/freeze, do-no-start 🪳 bug Something isn't working labels May 5, 2023
@Wumpf Wumpf requested a review from jprochazk May 5, 2023 15:50
Copy link
Member

@jprochazk jprochazk left a comment

Choose a reason for hiding this comment

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

From what I can tell, this will prevent the panic, but I don't understand any other consequences of making this change, so not sure if my review here is good enough to merge this! 🙂

crates/re_query/src/entity_view.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf added the do-not-merge Do not merge this PR label May 8, 2023
@teh-cmc teh-cmc changed the title Fix crash when clicking on an entity with varying number of components. Detect, warn and gracefully handle corrupt cells in loopup_arrow May 9, 2023
@teh-cmc teh-cmc changed the title Detect, warn and gracefully handle corrupt cells in loopup_arrow Detect, warn and gracefully handle corrupt cells in lookup_arrow May 9, 2023
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label May 9, 2023
@Wumpf Wumpf merged commit 32cc9f6 into main May 9, 2023
15 of 16 checks passed
@Wumpf Wumpf deleted the andreas/fix-entity-view-crash branch May 9, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start 🔍 re_query affects re_query itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants