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++ extensions, tests and example for Transform3D #2940

Merged
merged 12 commits into from
Aug 9, 2023
Merged

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Aug 8, 2023

What

Roundtrip test is part of #2937
Does not yet work in viewer because of #2871

C++ extensions now get additional includes copied into the generated header

Extensions circle around making use of the Transform3D archetype easy.
Note that the willingness of a c++ compiler to convert arrays and floats to bool causes us quite a lot of extra overloads (luckily the added unit tests catches these and there are warnings emitted).
The amount of overloads introduced leaves a bit of a bad taste, but the expectation is that this is rather special to Transform3D. Some of the provided overloads may be generated in the future if a pattern emerges.

A bunch of code example added to demonstrate what it looks like now to use Transform3D:

// translation only
rr_stream.log("translated", rr::artchetypes::Transform3D({1.0f, 0.0f, 0.0f}));

// translation/rotation/uniform-scale
rr_stream.log(
    "rotated_scaled",
    rr::archetypes::Transform3D(
        rrd::RotationAxisAngle({0.0f, 0.0f, 1.0f}, rrd::Angle::radians(pi / 4.0f)),
        2.0f
    )
);

// 3x3 matrix
rec_stream.log(
    "translation_and_mat3x3/rotation",
    rr::archetypes::Transform3D({
        {1.0f, 4.0f, 7.0f},
        {2.0f, 5.0f, 8.0f},
        {3.0f, 6.0f, 9.0f},
    })
);

Checklist

@Wumpf Wumpf added 🍏 primitives Relating to Rerun primitives 🌊 C++ API C/C++ API specific labels Aug 8, 2023
@teh-cmc teh-cmc self-requested a review August 9, 2023 07:17
Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Splendid 👏

@Wumpf Wumpf force-pushed the andreas/cpp/union-support branch from 9ac33f0 to 088e540 Compare August 9, 2023 07:48
auto rr_stream = rr::RecordingStream("transform3d");
rr_stream.connect("127.0.0.1:9876");

auto arrow = rr::archetypes::Arrows3D({0.0f, 1.0f, 0.0f});
Copy link
Member

Choose a reason for hiding this comment

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

I think the api looks really nice but have one thought. In Python we expose archetypes in the top level namespace (module) as well as under rerun.archetypes. That makes it possible to write some simple logging code without introducing the concept of archetypes yet. Is that something we might want to do in C++ as well? Might be a bit unclean for C++, but it is consistent with Python and we definitely want it there. Perhaps a bit magic but we could potentially do it inside rerun.hpp since that's what you include when you want the simple stuff (although that might be a little confusing)

Copy link
Member Author

Choose a reason for hiding this comment

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

would be very simple to do in rerun.hpp, yup. I'm leaning towards agreeing, a lil bit torn. If we do it here we should do so in Rust as well 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If we do it here we should do so in Rust as well 🤔

That's the plan yep. We don't do so yet because we were still "hiding" the new APIs until now (this wasn't a problem in python because we exposed those archetypes at the root of rr2, not rr).

The death of MsgSender will mark the beginning of archetypes at the root of the rerun crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

then let's do it! :)

For C++ this means afaik just

namespace rerun {
   using namespace archetypes;
}

inside of rerun.hpp

follow-up pr ofc

Base automatically changed from andreas/cpp/union-support to main August 9, 2023 08:02
@Wumpf Wumpf force-pushed the andreas/cpp/transform3d branch from 949c120 to 1c3167f Compare August 9, 2023 08:14
@Wumpf Wumpf force-pushed the andreas/cpp/transform3d branch from 0fbd20a to 547df7f Compare August 9, 2023 11:55
@Wumpf Wumpf merged commit d1b79cf into main Aug 9, 2023
@Wumpf Wumpf deleted the andreas/cpp/transform3d branch August 9, 2023 12:19
Wumpf added a commit that referenced this pull request Aug 9, 2023
### What

Check codegen'd files why this was necessary :)
Came up on #2940 but was a pre-existing problem

### 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/2943) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/2943)
- [Docs
preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Ffix-escaped-doc-comments/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Ffix-escaped-doc-comments/examples)
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 🍏 primitives Relating to Rerun primitives
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform3D + DisconnectedTransform migration to archetypes
3 participants