Use rerun::Collection almost everywhere we'd use std::vector before#4247
Merged
Use rerun::Collection almost everywhere we'd use std::vector before#4247
rerun::Collection almost everywhere we'd use std::vector before#4247Conversation
…tion. Improve various aspects of rerun::Collection and the generic converting adapter.
Wumpf
commented
Nov 16, 2023
2 tasks
and namespace some macros while we're touching that
emilk
approved these changes
Nov 16, 2023
Member
emilk
left a comment
There was a problem hiding this comment.
Whoa, that's a lot of complex C++ 😓
Looks good though!
| namespace archetypes { | ||
|
|
||
| #ifdef EDIT_EXTENSION | ||
| #if 0 |
Member
There was a problem hiding this comment.
#if 0 looks wrong, though I know it is right. Perhaps add a comment, or standardize how we do this
Member
Author
There was a problem hiding this comment.
Hmmm. Need to do a pass over these. Slowly switched to the simpler if 0 variant because the other thing got implemented incorrectly too often
| return data(); | ||
| } | ||
|
|
||
| /// TODO(andreas): Return proper iterator |
Member
There was a problem hiding this comment.
Why? A raw pointer has the benefit of being simple and much, much faster in debug builds
Member
Author
There was a problem hiding this comment.
Correct, but it will break down soon once we have strides
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
... and various improvements to
rerun::Collectionin general to be up for the task. Added copy assignment/construction, added vector conversion, removed serialization method (handled at callsites now)Also, I overhauled and generalized our builtin
rerun::CollectionAdaptersfurther. They give now more exact guarantees on when they copy & move data.To support the considerably increased significance and abilitites of this data structure I added a lot more tests, in particular checking on the aforementioned move/copy guarantees.
Huge thanks to @AnkeAnke for pointing out to me how we can support std compatible contains generically in a trivial way (
rerun::type_traits::value_type_t!). This made it possible that this PR requires almost no changes in user/test/example code since most types just snap in the same as before.As expected this change improves the performance on the image logging benchmark (which logs 4 16k rgba images).
Timing on M1 Max went from ~2.9s to ~2.4s and on the AMD windows box I use from ~3.0s to ~2.5s
(did a bunch of runs, values are excluding prepare and process setup, AMD fluctuates a lot)
DRAFT until I'm sure this works on more compilers.
Checklist