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

Add a C++ example of BarChart with added ergonomics #3837

Merged
merged 13 commits into from
Oct 13, 2023
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Oct 12, 2023

What

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

Plus all the plumbing needed to make that ergonomic
@emilk emilk added 🌊 C++ API C/C++ API specific exclude from changelog PRs with this won't show up in CHANGELOG.md labels Oct 12, 2023

namespace rerun {
/// IEEE 754 16-bit half-precision floating point number.
struct half {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can change this in the future - I just want a placeholder for now so that we don't have ugly holes in our C++ API

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

looking good but a few things to fix, most of them style related

docs/code-examples/bar_chart.cpp Outdated Show resolved Hide resolved
docs/code-examples/bar_chart.cpp Outdated Show resolved Hide resolved
Comment on lines +15 to +16
// --------------------------------------------------------------------
// Implicit constructors:
Copy link
Member

Choose a reason for hiding this comment

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

a bit redundant to call them "implicit constructors". All constructors are implicit unless they are explicitely marked as explicit

rerun_cpp/src/rerun/archetypes/bar_chart_ext.cpp Outdated Show resolved Hide resolved

#ifdef EDIT_EXTENSION
struct TensorDataExt {
#define TensorData TensorDataExt
Copy link
Member

Choose a reason for hiding this comment

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

Discussion topic: Maybe we should stop with those "emulated working enviroments" using defines and structs. I started putting them here to make the code compile if EDIT_EXTENSION is enabled and the code wasn't generated yet. But everyone else (well, also sometimes myself!) editing these files gets this wrong and copies the pattern without copying the part that actually make it compile :/
The only actually needed part is the comment markers and some means to make the code inside the comment marker not part of the sdk compilation (which is done with #ifdef EDIT_EXTENSION)

Copy link
Member Author

@emilk emilk Oct 12, 2023

Choose a reason for hiding this comment

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

Yeah, I'm just cargo-culting this. I'll leave it to you to clean up everywhere 😬

Copy link
Member

Choose a reason for hiding this comment

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

😄 . Feel free to keep this unchanged in this pr!

@emilk emilk requested a review from Wumpf October 12, 2023 14:10
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

neat, thanks for addressing everything and then some! :)

docs/code-examples/box3d_batch.cpp Outdated Show resolved Hide resolved
@emilk emilk merged commit ba897e6 into main Oct 13, 2023
@emilk emilk deleted the emilk/cpp-bar-charts branch October 13, 2023 08:14
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 exclude from changelog PRs with this won't show up in CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants