RSDK-9337: Introduce proto API bridge and insulate various geometry and math types from API #332

lia-viam · 2024-11-26T15:21:40Z

This PR partially does RSDK-9337 but I figured I would open it now because it is already getting high surface area and there are some API changes. (edit: OK sorry I just went ahead and added the rest of them)

Here we introduce the API bridge approach which will be used for opting in/out of inclusion of the viam API. The magic happens in sdk/common/proto_convert.hpp (open to renaming) which defines some template types which can be overridden to provide sizeless proto conversion interfaces, and the omni-callables which will "just work" on everything which specializes the templates. This will need to be documented somewhere, but now the approach for conversions is, for some viam::sdk::SdkType and viam::common::v1::ApiType, provide the following specializations:

namespace viam::sdk::proto_convert_details {
template<>
struct to_proto<SdkType> {
    void operator()(const SdkType&, ApiType*) const; // convert SdkType to ApiType
};

template<>
struct from_proto<ApiType> {
    SdkType operator()(const ApiType*) const; // convert ApiType to SdkType
};
}

The header proto_convert.hpp also contains the definition of two callable objects called to_proto and from_proto, and with the specializations above you get v2::to_proto(const SdkType&) which returns ApiType by value, and v2::from_proto(const ApiType&) which returns SdkType by value. There are also type traits EquivalentSdkType<ApiType> and EquivalentApiType<SdkType> which will tell you the corresponding types.

The v2 namespace is temporary pending resolution of a name clash with the to_proto and from_proto in proto_value.hpp.

There is also proto_convert_vector which handles conversion between std::vector and RepeatedPtrField. Note however that for a given std::vector<SdkType> and google::proto::RepeatedPtrField<ApiType>, if the specializations above are present then you can do v2::to_proto(const std::vector<SdkType>&) and v2::from_proto(const google::proto::RepeatedPtrField<ApiType>&) without any further intervention. This obviates the conversions in proto_utils.hpp which will be removed in a subsequent PR.

In the process of doing these changes I made some more-than-drive-by changes to some of the classes. Namely

Vector3 is now an aggregate with all public members and no user-defined constructors
some getters that were copying data members now return reference to const
GeometryType is now an enum class, and GeometryType::unknown is gone
GeometryConfig has some constructors instead of a bunch of trivial setters
GeometryConfig's GeometryType cannot vary independently of the stored geometry_specifics
OrientationConfig has been replaced with Orientation, and there is now a get_type(const Orientation&) rather than letting the type vary independently of the Orientation

…insulate

stuqdog

Nice, this looks great! I love how much cleaner all this looks, thanks for the work both on the broader change and on particular type refactors.

There's a lot going on here so I might want to give it a second pass, but right now this all looks great for me beyond some confusion around Orientation's to_proto method.

stuqdog · 2024-12-02T19:44:46Z

src/viam/sdk/spatialmath/orientation.cpp

+void to_proto<Orientation>::operator()(const Orientation& self, app::v1::Orientation* o) const {
+    struct Visitor {
+        void operator()(const axis_angles& a) {
+            app::v1::Orientation_AxisAngles aa;
            aa.set_x(a.x);
            aa.set_y(a.y);
            aa.set_z(a.z);
            aa.set_theta(a.theta);
            *orientation.mutable_axis_angles() = std::move(aa);


(q) I think I must be misunderstanding something, how does this compile? Where is orientation defined? It looks to me like the proto Orientation value has been renamed o, from orientation. What am I missing?

edit: it looks like orientation is being defined after the Visitor struct, is that correct? I'm a little bit surprised that's allowed in general, but also I wonder if I'm interpreting that correctly because it seems to me we should be modifying the passed-in o value, not the orientation value created on line 90.

So orientation is a data member of struct Visitor, it's defined after all the call operators have already been defined but still within Visitor. We are in fact modifying the passed-in o value, that is an Orientation* and it's being used to initialize Orientation& orientation because Visitor is initialized with *o when we say Visitor{*o}

You can think of this similar to a lambda capture of [&orientation = *o] which is also how a lambda works under the hood, it's defining an anonymous, function-local struct with a call operator whose data members are initialized to the capture list

…insulate

…api-insulate

acmorrow

Looks great. LGTM. I focused, per suggestion, mostly just on the new code. I did skim through the conversions, but I didn't want to spend a lot of time there as it seems @stuqdog is having a look there.

acmorrow · 2024-11-27T16:45:37Z

src/viam/sdk/common/linear_algebra.hpp

-#include <viam/api/common/v1/common.pb.h>
+#include <viam/sdk/common/proto_convert.hpp>
+
+VIAM_SDK_API_FWD_NAMESPACE_BEGIN(common)


I don't love the macro, but I see why you did it. However, we don't currently have the mechanisms in place to ensure that the macro is no longer defined after consuming our headers. Would a viam/sdk/viam_api_fwd.hpp header let you avoid the macro? I think we had discussed such an idea at one point. Maybe you tried it and there were undesirable consequences?

I'm open to getting rid of the macro, it's more of a convenience than anything, and also makes it easy to grep for where forward declarations are happening.

Putting everything in a single fwd header is doable, but I think that from a user's perspective I prefer that

if you do want to escape hatch into the proto conversions, the same header with the SDK types you're working with also shows you the relevant API types

you don't have to introduce all the other forward decls into scope

OK, those are good enough arguments against the fwd header that I think we can eliminate that.

How about a compromise? We keep the macro for now, but file a ticket that when the SDK goes to C++17, the macro gets removed, because then it becomes a lot less painful to write:

namespace ::viam::api::common::v1 { class Vector3; }

So, a ticket for that labeled as C++17? That or just do it explicitly now with C++14 namespaces. I've got a pretty strong aversion to macros in public headers if at all avoidable.

Also, it is probably worth filing a ticket now, if one doesn't exist, to introduce prelude/postlude headers to do macro push/pops so that the tooling for avoiding macro leakage is in place. It will be required in order to support adding ABI visibility annotations and/or calling convention annotations should we ever want to support windows.

acmorrow · 2024-12-04T20:42:58Z

src/viam/sdk/common/proto_convert.hpp

+template <typename ProtoType>
+struct from_proto;
+
+template <typename Callable>


I think this type is worth a comment, especially with the tricky args_t thing happening.

When we get to C++[17/20] does the standard give us tools to do this rather than needing boost? Mostly, is there a path to a polyfill here?

It honestly might even be doable in the current standard, just a little horrible maybe. I like callable traits especially for ease of use though especially given the following caveat: https://www.boost.org/doc/libs/1_86_0/libs/callable_traits/doc/html/index.html#callable_traits.introduction.motivation

That's pretty horrific. Maybe throw a TODO on here that it would be nice to eliminate this boost dependency in our headers if possible? Maybe future us finds a nice way to do it in some future standard.

acmorrow · 2024-12-04T20:46:50Z

src/viam/sdk/common/proto_convert.hpp

+constexpr proto_convert_details::to_proto_impl to_proto{};
+
+/// @brief Function object implementing conversion from an API type to an SDK type.
+/// This callable works for any type with a proto_convert_details::from_proto specialization as
+/// described above.
+constexpr proto_convert_details::from_proto_impl from_proto{};


These are objects defined in a header (though constexpr). Are there any concerns about linkage? I realize that's vague, but my mental model for how constexpr objects in headers work is pretty weak.

Let me look into that actually because I was modeling this off how range-v3 does it but I see now they have a macro: https://github.com/ericniebler/range-v3/blob/7e6f34b1e820fb8321346888ef0558a0ec842b8e/include/range/v3/detail/config.hpp#L568

acmorrow · 2024-12-04T20:47:15Z

src/viam/sdk/common/proto_convert.hpp

+#include <boost/callable_traits/return_type.hpp>
+#include <boost/mp11/algorithm.hpp>
+
+#include <type_traits>


Arguably this should come ahead of the boost headers

acmorrow · 2024-12-04T20:51:51Z

src/viam/sdk/common/proto_convert_vector.hpp

+struct to_proto<std::vector<SdkType>> {
+    void operator()(
+        const std::vector<SdkType>& vec,
+        ::google::protobuf::RepeatedPtrField<EquivalentApiType<SdkType>>* ptr_field) const {
+        ptr_field->Reserve(vec.size());
+        for (const auto& elem : vec) {
+            to_proto<SdkType>{}(elem, ptr_field->Add());
+        }
+    }
+};
+
+template <typename ApiType>
+struct from_proto<::google::protobuf::RepeatedPtrField<ApiType>> {


I'm not totally sold on making these part of our public API. Or, at least not yet. Could these be in common/private and under viam::sdk::impl?

I don't doubt the utility, but I'm not sure we need to be in the position of exposing helpers like this for the cases where consumers have opted into dealing with our low-level proto stuff.

It is easier to keep it private now and expose it if it proves worthwhile, than to take it away later.

I think that's a better call, I was actually worrying myself about this today because I think there may be cases where it might be RepeatedField vs RepeatedPtrField and this would be an annoying API lock-in

OK, I think let's move them to internal then, since it seems we have similar thoughts here.

apparently the choice of repeated field vs ptr field is actually categorical: https://protobuf.dev/reference/cpp/api-docs/google.protobuf.repeated_field/#

but for now I will still leave it in a private header and we can think of publicizing it if there seems to be a need

src/viam/sdk/common/private/proto_utils.hpp

acmorrow · 2024-12-05T15:01:03Z

src/viam/sdk/common/linear_algebra.hpp

-#include <viam/api/common/v1/common.pb.h>
+#include <viam/sdk/common/proto_convert.hpp>
+
+VIAM_SDK_API_FWD_NAMESPACE_BEGIN(common)


OK, those are good enough arguments against the fwd header that I think we can eliminate that.

How about a compromise? We keep the macro for now, but file a ticket that when the SDK goes to C++17, the macro gets removed, because then it becomes a lot less painful to write:

namespace ::viam::api::common::v1 { class Vector3; }

So, a ticket for that labeled as C++17? That or just do it explicitly now with C++14 namespaces. I've got a pretty strong aversion to macros in public headers if at all avoidable.

Also, it is probably worth filing a ticket now, if one doesn't exist, to introduce prelude/postlude headers to do macro push/pops so that the tooling for avoiding macro leakage is in place. It will be required in order to support adding ABI visibility annotations and/or calling convention annotations should we ever want to support windows.

acmorrow · 2024-12-05T15:03:08Z

src/viam/sdk/CMakeLists.txt

@@ -142,6 +142,8 @@ target_sources(viamsdk
      ../../viam/sdk/common/exception.hpp
      ../../viam/sdk/common/linear_algebra.hpp
      ../../viam/sdk/common/pose.hpp
+      ../../viam/sdk/common/proto_convert.hpp
+      ../../viam/sdk/common/proto_convert_vector.hpp


⬆️ - How close do the changes in this review get things w.r.t. removing this:

# This bit of magic is required so that we get the public build-time # include path of viamapi in *ahead* of the path anchored to `src` by # `BASE_DIRS` below, because otherwise we will pull in the static # versions of the proto files even if we are generating dynamically. target_include_directories(viamsdk PUBLIC "$<BUILD_INTERFACE:$<TARGET_PROPERTY:viam-cpp-sdk::viamapi,INTERFACE_INCLUDE_DIRECTORIES>>" )

Or at least reducing it to PRIVATE?

Ooh interesting, I think that will be doable once the epic to get all API imports out of public headers is complete!

acmorrow · 2024-12-05T15:06:25Z

src/viam/sdk/common/proto_convert.hpp

+template <typename ProtoType>
+struct from_proto;
+
+template <typename Callable>


That's pretty horrific. Maybe throw a TODO on here that it would be nice to eliminate this boost dependency in our headers if possible? Maybe future us finds a nice way to do it in some future standard.

acmorrow · 2024-12-05T15:07:43Z

src/viam/sdk/common/proto_convert_vector.hpp

+struct to_proto<std::vector<SdkType>> {
+    void operator()(
+        const std::vector<SdkType>& vec,
+        ::google::protobuf::RepeatedPtrField<EquivalentApiType<SdkType>>* ptr_field) const {
+        ptr_field->Reserve(vec.size());
+        for (const auto& elem : vec) {
+            to_proto<SdkType>{}(elem, ptr_field->Add());
+        }
+    }
+};
+
+template <typename ApiType>
+struct from_proto<::google::protobuf::RepeatedPtrField<ApiType>> {


OK, I think let's move them to internal then, since it seems we have similar thoughts here.

lia-viam added 12 commits November 21, 2024 15:16

first commit of generic proto conversion machinery

507ab57

insulate pose and pose in frame

9ff1696

add api fwd namespace macro

5128069

more general macro

cf35091

change translation

7a3376a

refactor orientation and related code cleanup

a440614

insult api vector3 and make sdk vector3 a struct

924b794

remove duplicated pose interface

c438580

use std::tie for equality operators

4fbd8bf

remove pose_proto

4c258ac

clean up geometry.hpp and add vector/ptr_field conversion

d520711

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into api-…

74b9582

…insulate

lia-viam requested review from acmorrow and stuqdog November 26, 2024 15:21

lia-viam requested a review from a team as a code owner November 26, 2024 15:21

lia-viam requested review from njooma and removed request for a team November 26, 2024 15:21

lia-viam added 6 commits November 26, 2024 10:32

linter

c55948e

forgot to commit new file

e2a5a3c

add headers to file list

8479d34

also do translation and linkconfig

27f147c

linter semicolon

f69d797

do worldstate

42552ff

stuqdog reviewed Dec 2, 2024

View reviewed changes

lia-viam added 5 commits December 2, 2024 15:05

Merge branch 'main' into api-insulate

34bfdd2

remove proto_utils.hpp

8382629

bring ProtoValue into the to_proto/from_proto fold

141487f

remove ctor semicolon

7b7da5a

remove ctor semicolon

cf7f60b

acmorrow mentioned this pull request Dec 3, 2024

RSDK-6574: Fix use of time_point #335

Merged

lia-viam added 4 commits December 4, 2024 10:33

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into api-…

9d876c5

…insulate

Merge branch 'api-insulate' of github.com:lia-viam/viam-cpp-sdk into …

ba66ea6

…api-insulate

Merge branch 'ptr-vec-conversions' into proto-value-unify

8128398

remove geo geometry stuff i forgot to delete

02ce15f

acmorrow approved these changes Dec 4, 2024

View reviewed changes

lia-viam added 4 commits December 5, 2024 09:21

set const

2ec0610

Merge branch 'ptr-vec-conversions' into api-insulate

52590fa

Merge branch 'proto-value-unify' into api-insulate

0716a23

add or relocate nolints

369e321

acmorrow reviewed Dec 5, 2024

View reviewed changes

lia-viam added 5 commits December 5, 2024 10:15

change include order and replace macro with explicit namespaces

97841a5

document args_t trick and set container to mp_list

fbe2ea9

make to/from repeated field its own function object and a private detail

670d2ef

use static_const for callable objects

96f2742

include repeated ptr conversion

40cf73b

lia-viam merged commit e59af19 into viamrobotics:main Dec 5, 2024
4 checks passed

lia-viam deleted the api-insulate branch December 5, 2024 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-9337: Introduce proto API bridge and insulate various geometry and math types from API #332

RSDK-9337: Introduce proto API bridge and insulate various geometry and math types from API #332

lia-viam commented Nov 26, 2024 •

edited

Loading

stuqdog left a comment

stuqdog Dec 2, 2024

lia-viam Dec 2, 2024

acmorrow left a comment

acmorrow Nov 27, 2024

lia-viam Dec 4, 2024

acmorrow Dec 5, 2024

acmorrow Dec 4, 2024

lia-viam Dec 4, 2024

acmorrow Dec 5, 2024

acmorrow Dec 4, 2024

lia-viam Dec 4, 2024

acmorrow Dec 4, 2024

acmorrow Dec 4, 2024

lia-viam Dec 4, 2024

acmorrow Dec 5, 2024

lia-viam Dec 5, 2024

acmorrow Dec 5, 2024

acmorrow Dec 5, 2024

lia-viam Dec 5, 2024

acmorrow Dec 5, 2024

acmorrow Dec 5, 2024

RSDK-9337: Introduce proto API bridge and insulate various geometry and math types from API #332

RSDK-9337: Introduce proto API bridge and insulate various geometry and math types from API #332

Conversation

lia-viam commented Nov 26, 2024 • edited Loading

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lia-viam commented Nov 26, 2024 •

edited

Loading