RSDK-3589 add wrapper for navigation service

[abe-winter]

What changed

• add abstract Navigation class, navigation server and client, register them
• incidental: utils for repeatedPtrToVec and vecToRepeatedPtr

Why

So people can write nav modules in C++.

Follow-ups

• someone with deeper understanding of the interface should rewrite docstrings in navigation.hpp
• maybe add example
• refactor repeatedPtrToVec and friends

[lia-viam]

overall this looks really solid, just added some suggestions about SDK coding standards changes which are currently in progress!

[lia-viam]

Not that we've announced this super loudly but we're currently trying to cut back on API and google:: types in headers, so I think we'll want to delete these helper functions even though they're being used a couple times.

As a suggested inline replacement, I think this could just be

dest.Assign(vec.begin(), vec.end());

You can also do that with the Add method, but note that if dest already had stuff then this function would be under-reserving potentially and adding to existing elements which is maybe not what I'd expect from an xToY function
As a suggested replacement for this one, can you just write inline

[abe-winter]

good call -- clearing the destination in d334572 to protect against adding to non-empty collection

I think Assign(begin, end) doesn't work because the types are different -- tried to get it working with constructor/operator implicit conversion approach, couldn't, guessing even harder now that the constructor/operators have been moved out of common headers

[lia-viam]

similarly I think this could be vec.assign(src.Begin(), src.End()), also with similar caveats as above about assigning vs appending

[lia-viam]

similar to the other comment about keeping API stuff out of headers, @stuqdog is currently working on a PR to take these kinds of conversion functions out of public headers, so maybe you guys can talk so that we have a similar approach in this PR as well

[lia-viam]

but one option for example is, since I think these are only being used in navigation_client.cpp to just inline them there

[abe-winter]

okay I removed all *.pb.h includes from the wrapper header (navigation.hpp)

how:

• I moved the implicit conversions for Waypoint and Path to functions in the navigation client/server.cpp files
• and gave the std::vector <-> RepeatedPointer conversions their own header

now the invocation looks like:

// when destination type has a `from_proto` method, use it:
repeatedPtrToVec(response.obstacles(), *ret);

// otherwise, pass in the conversion function:
repeatedPtrToVec(response.waypoints(), *ret, waypoint_from_proto);

[acmorrow]

Just taking a look because I'm interested. The client/server parts all look great, and I'm glad to see that the helpers are making it much easier to add these implementations. I think the proto_utils stuff could be simplified and generalized.

[acmorrow]

A semi-near-term goal for the C++ SDK is to move all the proto stuff out of public headers. Could you push this down into viam/sdk/common/private/proto_utils.hpp? That's how we indicate that files shouldn't be consumed except by files that form part of the SDK or published as part of the install step.

[acmorrow]

add namespace impl here, so viam::sdk::impl. That's the namespace we use for any private things that shouldn't escape.

[acmorrow]

I have a slight preference for out parameters being taken by pointer, as it makes it clearer at the call site that the field is likely being mutated:

vecToRepeatedPtr(my_vec, dest);  // err, who is getting modified here?

vs

vecToRepeatedPtr(my_vec, &dest);  // ah, classic out parameter.

Others dislike this because they worry that it allows dest to be nullptr. I don't find that compelling because requiring it to be a reference doesn't prevent the opposite mistake:

RepeatedPtrField<X>* pdest = ...
vecToRepeatedPtr(my_vec, *pdest);  // oops, what if pdest is nullptr?

[lia-viam]

So generally speaking I'm very much against output parameters being passed as pointers, it's one of the things I dislike the most about google's C++ coding guidelines. I think with your counterexample above the point is that, in C++ and as high level as this code is, we generally aren't passing around pointer types in the first place, and the point of references is that they must point to a valid object or else it's UB. So as for

oops, what if pdest is nullptr

That's on the caller for dereferencing a null pointer in the first place, and is equally true of if my_vec was *p_my_vec for some reason, and this is kind of the point of C++ having reference types.

Callsite legibility I'm somewhat sympathetic to but would be better done with something like std::ref(arg) or gsl::output_parameter, because then you don't lose the non-null guarantee.

.....All that said, I think this could be a pointer parameter just for ABI insulation reasons

[acmorrow]

More idiomatic would just to be take

template <typename Src, typename Callable>
void vecToRepeatedPtr(const std::vector<Src>& vec,
                      google::protobuf::RepeatedPtrField<Dst>& dest,
                      Callable&& c) {

Because then you can pass any sort of callable (std::function, lambda, function pointer as you have now).

You can then enforce the shape of the callable with a static assert or similar.

[abe-winter]

explain this one -- this is better because it would select this specialization and return a static assert error, rather than giving a long 'no matching template' error?

[acmorrow]

I think, as written, yours will only accept function pointers. I could be wrong, I'd need to test it out on godbolt.

[acmorrow]

This would work with other containers than just std::vector right? std::array, std::list, etc.?

[acmorrow]

You could eliminate the distinction between the converting and non-converting by just always taking a transformation function and defaulting it to std::identity (might need to get it from boost for now)

That actually has me wondering whether all of these are really glosses on std::transform and would be better implemented that way?

[acmorrow]

Since this is the public facing header, this is the one that should have doc comments. Also, please add blank lines between the end of the declaration of one method and the beginning of the next.

    virtual LocationResponse get_location(const std::string name, const ProtoStruct& extra) = 0;

    virtual std::unique_ptr<std::vector<Waypoint>> get_waypoints(const std::string name,
                                                                 const ProtoStruct& extra) = 0;

That, moreso, once comments are added:

    /// Some comment
    virtual LocationResponse get_location(const std::string name, const ProtoStruct& extra) = 0;

    /// Another comment
    virtual std::unique_ptr<std::vector<Waypoint>> get_waypoints(const std::string name,
                                                                 const ProtoStruct& extra) = 0;

[lia-viam]

So just as a general guideline it's kind of a C++ anti-pattern to have the type name in the name of the function--since C doesn't have function overloading you're kind of forced to do things like f_int f_double etc, but in this case function overloading is able to resolve from_proto(some_waypoint) to call the overload which takes a const Waypoint&.

That said, this function and path_from_proto are only used in this .cpp file, so I would probably just define them inline as a lambda passed to your conversion function, ie,

repeatedPtrToVec(src, dest, [](const auto& proto) { return Navigation::Waypoint{proto.id(), geo_point::from_proto(proto.location())}; });

[abe-winter]

renamed to bare from/to proto in 4e9f32c

(holding off on bigger changes bc it sounds like sdk team will do a design pass on repeatedPtrToVec stuff after this merges)

[lia-viam]

I think just template this as template <typename T, typename U, typename Func> and have the second argument be Func f

[abe-winter]

that made it sad

could specify template param I guess? but can I leave as-is?

/host/src/viam/sdk/tests/test_navigation.cpp: In lambda function:
/host/src/viam/sdk/tests/test_navigation.cpp:78:14: error: no matching function for call to 'mapOver(std::vectorviam::sdk::Navigation::Waypoint&, std::__cxx11::basic_string (*)(const viam::sdk::Navigation::Waypoint&))'
78 | });
| ^
/host/src/viam/sdk/tests/test_navigation.cpp:46:16: note: candidate: 'template<class T, class U, class Func> std::vector viam::sdktests::navigation::mapOver(const std::vector&, Func)'
46 | std::vector mapOver(const std::vector& source, Func f) {
| ^~~~~~~
/host/src/viam/sdk/tests/test_navigation.cpp:46:16: note: template argument deduction/substitution failed:
/host/src/viam/sdk/tests/test_navigation.cpp:78:14: note: couldn't deduce template parameter 'T'
78 | });