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

Always log the primary component last so it is joined properly #1217

Merged
merged 3 commits into from
Feb 12, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Feb 11, 2023

When investigating #1215 I discovered that even though I was specifying a radius, it was not being applied to the very first points.

The way range-queries work they always start with an element containing all the data based on the latest time the primary key was logged. Since we log the splat after the point, any splatted data was missing from this context and the initial set of points was logged with auto.

Then, I think a point-cloud heuristic was shrinking auto as more points got added to the scene?

Logging the primary component last seems like a generally safe thing to do and will make the range-queries behave properly for now. I also opened #1218 for longer-term work.

Visible history on, and points showing the correctly logged radii:
image

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

@teh-cmc
Copy link
Member

teh-cmc commented Feb 12, 2023

@emilk
Copy link
Member

emilk commented Feb 12, 2023

I don't think them being splats are the important part. It is the secondary components (color, radii) that need to be logged before the primary component (positions), and the primary component is never a splat.

And yes, the better solution is to use the exact same time point when logging the different components of an entity.

@emilk
Copy link
Member

emilk commented Feb 12, 2023

Note: same needs to happen in Rust SDK (https://github.com/rerun-io/rerun/blob/main/crates/re_sdk/src/msg_sender.rs#L254-L270)

In Rust, all messages of the same MsgSender correctly use the same timepoint (afaict), so this is not a problem there (unless we introduce a receive_time timeline…).

@jleibs
Copy link
Member Author

jleibs commented Feb 12, 2023

In Rust, all messages of the same MsgSender correctly use the same timepoint

I think the store will still treat these separably based on insertion order. You still end up with multiple rows in the store.

@teh-cmc if multiple rows have identical time stamps i assume the initial range-result is just the first row, correct?

@teh-cmc
Copy link
Member

teh-cmc commented Feb 12, 2023

In Rust, all messages of the same MsgSender correctly use the same timepoint

I think the store will still treat these separably based on insertion order. You still end up with multiple rows in the store.

@teh-cmc if multiple rows have identical time stamps i assume the initial range-result is just the first row, correct?

Yes.

In fact I thought that this was the exact thing that this PR was trying to account for to begin with. I didn't even know that the python SDK attributed different log times to the differents part of a log event when splitting it.

@emilk
Copy link
Member

emilk commented Feb 12, 2023

So this means we always need to log the primary component last, correct?

It really has nothing to do with instanced vs splatted.

All instanced components is in the same MsgBundle, but could there still be a problem there?

@jleibs jleibs changed the title Always log splats first so they are joined properly Always log the primary component last so it is joined properly Feb 12, 2023
@jleibs jleibs merged commit a5a6a01 into main Feb 12, 2023
@jleibs jleibs deleted the jleibs/log_splats_first branch February 12, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants