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

[C++] Rename rerun::ComponentBatch to rerun::Collection (and related constructs) #4225

Closed
wants to merge 5 commits into from

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 14, 2023

What

Not much else has changed yet except some doc massaging. First step towards solving

rerun::Collection will evolve from here on and be the corner piece of our zero copy & component adaptability strategy (just like ComponentBatch is already!)

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality 🌊 C++ API C/C++ API specific include in changelog labels Nov 14, 2023
@Wumpf Wumpf force-pushed the andreas/cpp/rerun-collection branch from 734f5dc to 1c08991 Compare November 14, 2023 15:45
@@ -1,12 +1,9 @@
#include "../half.hpp"
#include "bar_chart.hpp"

// #define EDIT_EXTENSION
namespace rerun::archetypes {
Copy link
Member

Choose a reason for hiding this comment

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

can this be added to the clang-format file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only found https://clang.llvm.org/docs/ClangFormatStyleOptions.html#compactnamespaces which isn't quite the same

But as you pointed out elsewhere there's also the option to not indent after namespace, we should try that (in a separate pr)
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#namespaceindentation

}

// We want to change the dimension names if they are not specified.
// But rerun collections are strictly immutable, so create a new one if necessary.
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure using Collection for TensorDimension makes sense? When will it avoid allocations and memcpys, realistically?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's less about avoiding copies & allocs but more about being able to use the adapters for this type as well

@Wumpf
Copy link
Member Author

Wumpf commented Nov 15, 2023

oooops, that was the wrong source branch 😳 😳. The plan was to have a PR only for up to 3de46c8
The rest is still super experimental. Mopping up. Since this was already approved now I merge the rename pr and bring up a new one for the rest later!

@Wumpf Wumpf closed this Nov 15, 2023
Wumpf added a commit that referenced this pull request Nov 15, 2023
…ted constructs) (#4236)

### What

Not much else has changed yet except some doc massaging. First step
towards solving

* #3794

`rerun::Collection` will evolve from here on and be the corner piece of
our zero copy & component adaptability strategy (just like
ComponentBatch is already!)


_This was originally opened on the wrong branch by accident:_
*  #4225

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4225) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4225)
- [Docs
preview](https://rerun.io/preview/3de46c8a5d294eb5ad68e5beb35a64d0610e992a/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/3de46c8a5d294eb5ad68e5beb35a64d0610e992a/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
@teh-cmc teh-cmc mentioned this pull request Dec 12, 2023
18 tasks
@Wumpf Wumpf deleted the andreas/cpp/rerun-collection branch January 10, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌊 C++ API C/C++ API specific include in changelog 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants