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.

Sign up for GitHub

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

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

Merged
merged 36 commits into from
Dec 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
507ab57
first commit of generic proto conversion machinery
lia-viam Nov 21, 2024
9ff1696
insulate pose and pose in frame
lia-viam Nov 21, 2024
5128069
add api fwd namespace macro
lia-viam Nov 22, 2024
cf35091
more general macro
lia-viam Nov 22, 2024
7a3376a
change translation
lia-viam Nov 22, 2024
a440614
refactor orientation and related code cleanup
lia-viam Nov 25, 2024
924b794
insult api vector3 and make sdk vector3 a struct
lia-viam Nov 25, 2024
c438580
remove duplicated pose interface
lia-viam Nov 25, 2024
4fbd8bf
use std::tie for equality operators
lia-viam Nov 25, 2024
4c258ac
remove pose_proto
lia-viam Nov 25, 2024
d520711
clean up geometry.hpp and add vector/ptr_field conversion
lia-viam Nov 26, 2024
74b9582
Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into api-…
lia-viam Nov 26, 2024
c55948e
linter
lia-viam Nov 26, 2024
e2a5a3c
forgot to commit new file
lia-viam Nov 26, 2024
8479d34
add headers to file list
lia-viam Nov 26, 2024
27f147c
also do translation and linkconfig
lia-viam Nov 26, 2024
f69d797
linter semicolon
lia-viam Nov 26, 2024
42552ff
do worldstate
lia-viam Nov 26, 2024
34bfdd2
Merge branch 'main' into api-insulate
lia-viam Dec 2, 2024
8382629
remove proto_utils.hpp
lia-viam Dec 2, 2024
141487f
bring ProtoValue into the to_proto/from_proto fold
lia-viam Dec 3, 2024
7b7da5a
remove ctor semicolon
lia-viam Dec 3, 2024
cf7f60b
remove ctor semicolon
lia-viam Dec 3, 2024
9d876c5
Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into api-…
lia-viam Dec 4, 2024
ba66ea6
Merge branch 'api-insulate' of github.com:lia-viam/viam-cpp-sdk into …
lia-viam Dec 4, 2024
8128398
Merge branch 'ptr-vec-conversions' into proto-value-unify
lia-viam Dec 4, 2024
02ce15f
remove geo geometry stuff i forgot to delete
lia-viam Dec 4, 2024
2ec0610
set const
lia-viam Dec 5, 2024
52590fa
Merge branch 'ptr-vec-conversions' into api-insulate
lia-viam Dec 5, 2024
0716a23
Merge branch 'proto-value-unify' into api-insulate
lia-viam Dec 5, 2024
369e321
add or relocate nolints
lia-viam Dec 5, 2024
97841a5
change include order and replace macro with explicit namespaces
lia-viam Dec 5, 2024
fbe2ea9
document args_t trick and set container to mp_list
lia-viam Dec 5, 2024
670d2ef
make to/from repeated field its own function object and a private detail
lia-viam Dec 5, 2024
96f2742
use static_const for callable objects
lia-viam Dec 5, 2024
40cf73b
include repeated ptr conversion
lia-viam Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/viam/sdk/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

⬆️ - 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

../../viam/sdk/common/proto_value.hpp
../../viam/sdk/common/service_helper.hpp
../../viam/sdk/common/utils.hpp
Expand Down
38 changes: 14 additions & 24 deletions src/viam/sdk/common/linear_algebra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,55 +7,45 @@
namespace viam {
namespace sdk {

Vector3::Vector3(scalar_type x, scalar_type y, scalar_type z) : data_{x, y, z} {};
Vector3::Vector3() : Vector3(0.0, 0.0, 0.0){};

double Vector3::x() const {
return this->data_[0];
return this->data[0];
}

double Vector3::y() const {
return this->data_[1];
return this->data[1];
}

double Vector3::z() const {
return this->data_[2];
return this->data[2];
}

Vector3& Vector3::set_x(double x) {
this->data_[0] = x;
this->data[0] = x;
return *this;
}

Vector3& Vector3::set_y(double y) {
this->data_[1] = y;
this->data[1] = y;
return *this;
}

Vector3& Vector3::set_z(double z) {
this->data_[2] = z;
this->data[2] = z;
return *this;
}

const std::array<double, 3>& Vector3::data() const {
return this->data_;
}
namespace proto_convert_details {

std::array<double, 3>& Vector3::data() {
return this->data_;
void to_proto<Vector3>::operator()(const Vector3& self, common::v1::Vector3* proto) const {
proto->set_x(self.x());
proto->set_y(self.y());
proto->set_z(self.z());
}

viam::common::v1::Vector3 Vector3::to_proto() const {
viam::common::v1::Vector3 result;
result.set_x(x());
result.set_y(y());
result.set_z(z());
return result;
};

Vector3 Vector3::from_proto(const viam::common::v1::Vector3& vec) {
return {vec.x(), vec.y(), vec.z()};
Vector3 from_proto<common::v1::Vector3>::operator()(const common::v1::Vector3* proto) const {
return {proto->x(), proto->y(), proto->z()};
}

} // namespace proto_convert_details
} // namespace sdk
} // namespace viam
41 changes: 26 additions & 15 deletions src/viam/sdk/common/linear_algebra.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,22 @@
#include <boost/qvm/vec.hpp>
#include <boost/qvm/vec_traits.hpp>

#include <viam/api/common/v1/common.pb.h>
#include <viam/sdk/common/proto_convert.hpp>

VIAM_SDK_API_FWD_NAMESPACE_BEGIN(common)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


class Vector3;

VIAM_SDK_API_FWD_NAMESPACE_END

namespace viam {
namespace sdk {

// In the future, we may wish to inline this whole class
// for performance reasons.

class Vector3 {
public:
struct Vector3 {
using scalar_type = double;
Vector3(scalar_type x, scalar_type y, scalar_type z);
Vector3();

scalar_type x() const;
scalar_type y() const;
Expand All @@ -28,15 +32,22 @@ class Vector3 {
/// Set the z value of the vector (can be chained)
Vector3& set_z(scalar_type z);

const std::array<scalar_type, 3>& data() const;
std::array<scalar_type, 3>& data();
viam::common::v1::Vector3 to_proto() const;
static Vector3 from_proto(const viam::common::v1::Vector3& vec);
std::array<scalar_type, 3> data;
};

namespace proto_convert_details {

private:
std::array<scalar_type, 3> data_;
template <>
struct to_proto<Vector3> {
void operator()(const Vector3&, common::v1::Vector3*) const;
};

template <>
struct from_proto<common::v1::Vector3> {
Vector3 operator()(const common::v1::Vector3*) const;
};

} // namespace proto_convert_details
} // namespace sdk
} // namespace viam

Expand All @@ -51,20 +62,20 @@ struct vec_traits<viam::sdk::Vector3> {

template <int I>
static inline scalar_type& write_element(vec_type& v) {
return v.data()[I];
return v.data[I];
}

template <int I>
static inline scalar_type read_element(vec_type const& v) {
return v.data()[I];
return v.data[I];
}

static inline scalar_type& write_element_idx(int i, vec_type& v) {
return v.data()[i];
return v.data[i];
}

static inline scalar_type read_element_idx(int i, vec_type const& v) {
return v.data()[i];
return v.data[i];
}
};

Expand Down
73 changes: 43 additions & 30 deletions src/viam/sdk/common/pose.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,36 +5,6 @@
namespace viam {
namespace sdk {

common::v1::PoseInFrame pose_in_frame::to_proto() const {
common::v1::PoseInFrame pif;
*pif.mutable_reference_frame() = reference_frame;
common::v1::Pose proto_pose;
proto_pose.set_x(pose.coordinates.x);
proto_pose.set_y(pose.coordinates.y);
proto_pose.set_z(pose.coordinates.z);
proto_pose.set_o_x(pose.orientation.o_x);
proto_pose.set_o_y(pose.orientation.o_y);
proto_pose.set_o_z(pose.orientation.o_z);
proto_pose.set_theta(pose.theta);
*pif.mutable_pose() = std::move(proto_pose);
return pif;
};

pose_in_frame pose_in_frame::from_proto(const common::v1::PoseInFrame& proto) {
pose_in_frame pif;
pif.reference_frame = proto.reference_frame();
const auto& proto_pose = proto.pose();
pif.pose.orientation.o_x = proto_pose.o_x();
pif.pose.orientation.o_y = proto_pose.o_y();
pif.pose.orientation.o_z = proto_pose.o_z();
pif.pose.coordinates.x = proto_pose.x();
pif.pose.coordinates.y = proto_pose.y();
pif.pose.coordinates.z = proto_pose.z();
pif.pose.theta = proto_pose.theta();

return pif;
}

bool operator==(const pose_in_frame& lhs, const pose_in_frame& rhs) {
return lhs.pose == rhs.pose && lhs.reference_frame == rhs.reference_frame;
}
Expand All @@ -44,5 +14,48 @@ std::ostream& operator<<(std::ostream& os, const pose_in_frame& v) {
return os;
}

namespace proto_convert_details {

void to_proto<pose>::operator()(const pose& self, common::v1::Pose* proto) const {
proto->set_x(self.coordinates.x);
proto->set_y(self.coordinates.y);
proto->set_z(self.coordinates.z);
proto->set_o_x(self.orientation.o_x);
proto->set_o_y(self.orientation.o_y);
proto->set_o_z(self.orientation.o_z);
proto->set_theta(self.theta);
}

pose from_proto<common::v1::Pose>::operator()(const common::v1::Pose* proto) const {
pose pose;
pose.coordinates.x = proto->x();
pose.coordinates.y = proto->y();
pose.coordinates.z = proto->z();
pose.orientation.o_x = proto->o_x();
pose.orientation.o_y = proto->o_y();
pose.orientation.o_z = proto->o_z();
pose.theta = proto->theta();

return pose;
}

void to_proto<pose_in_frame>::operator()(const pose_in_frame& self,
common::v1::PoseInFrame* pif) const {
*(pif->mutable_reference_frame()) = self.reference_frame;
common::v1::Pose proto_pose;
to_proto<pose>{}(self.pose, &proto_pose);
*(pif->mutable_pose()) = std::move(proto_pose);
};

pose_in_frame from_proto<common::v1::PoseInFrame>::operator()(
const common::v1::PoseInFrame* proto) const {
pose_in_frame pif;
pif.reference_frame = proto->reference_frame();
pif.pose = from_proto<common::v1::Pose>{}(&(proto->pose()));

return pif;
}

} // namespace proto_convert_details
} // namespace sdk
} // namespace viam
40 changes: 34 additions & 6 deletions src/viam/sdk/common/pose.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
#pragma once

#include <common/v1/common.pb.h>
#include <viam/sdk/common/proto_convert.hpp>

#include <ostream>

VIAM_SDK_API_FWD_NAMESPACE_BEGIN(common)

class Pose;
class PoseInFrame;

VIAM_SDK_API_FWD_NAMESPACE_END

namespace viam {
namespace sdk {
Expand All @@ -20,16 +29,11 @@ struct pose {
pose_orientation orientation;
double theta;

static pose from_proto(const viam::common::v1::Pose& proto);
viam::common::v1::Pose to_proto() const;

friend bool operator==(const pose& lhs, const pose& rhs);
friend std::ostream& operator<<(std::ostream& os, const pose& v);
};

struct pose_in_frame {
viam::common::v1::PoseInFrame to_proto() const;
static pose_in_frame from_proto(const viam::common::v1::PoseInFrame& proto);
pose_in_frame(std::string reference_frame_, struct pose pose_)
: reference_frame(std::move(reference_frame_)), pose(std::move(pose_)) {}
pose_in_frame() {}
Expand All @@ -40,5 +44,29 @@ struct pose_in_frame {
friend std::ostream& operator<<(std::ostream& os, const pose_in_frame& v);
};

namespace proto_convert_details {

template <>
struct to_proto<pose> {
void operator()(const pose&, common::v1::Pose*) const;
};

template <>
struct from_proto<common::v1::Pose> {
pose operator()(const common::v1::Pose*) const;
};

template <>
struct to_proto<pose_in_frame> {
void operator()(const pose_in_frame&, common::v1::PoseInFrame*) const;
};

template <>
struct from_proto<common::v1::PoseInFrame> {
pose_in_frame operator()(const common::v1::PoseInFrame*) const;
};

} // namespace proto_convert_details

} // namespace sdk
} // namespace viam
4 changes: 2 additions & 2 deletions src/viam/sdk/common/private/proto_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ void vecToRepeatedPtr(const std::vector<Src>& vec, google::protobuf::RepeatedPtr
dest.Clear();
dest.Reserve(vec.size());
for (auto& x : vec) {
*dest.Add() = x.to_proto();
*dest.Add() = v2::to_proto(x);
acmorrow marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -41,7 +41,7 @@ void repeatedPtrToVec(const google::protobuf::RepeatedPtrField<Src>& src, std::v
vec.clear();
vec.reserve(src.size());
for (auto& x : src) {
vec.push_back(Dst::from_proto(x));
vec.push_back(v2::from_proto(x));
}
}

Expand Down
Loading