Feature/python bindings/navigation#3310
Conversation
elandini84
added
PR Type: Feat/Enh
Summary of ChangesHello @elandini84, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances YARP's Python interoperability by introducing bindings for its 2D navigation, mapping, and localization interfaces. This allows Python developers to seamlessly integrate and control YARP-based robotic navigation systems, facilitating tasks such as querying robot pose, managing environmental maps, and orchestrating navigation goals directly from Python applications. The changes expand the utility of YARP within the Python ecosystem for robotics development. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
elandini84
temporarily deployed
to
code-analysis
GitHub Actions
Inactive
elandini84
had a problem deploying
to
code-analysis
GitHub Actions
Failure
elandini84
temporarily deployed
to
code-analysis
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds Python bindings for the YARP navigation interfaces INavigation2D, ILocalization2D, and IMap2D. The changes involve updating the SWIG interface file (yarp.i) to include the necessary headers, type definitions, and wrapper functions. Release notes and a central header file are also updated.
The implementation is mostly correct and follows existing patterns. I have a couple of suggestions to improve maintainability:
- In
bindings/yarp.i, the new%includedirectives could be sorted alphabetically within their logical groups to improve readability. - Also in
bindings/yarp.i, the newviewmethods for namespaced interfaces are manually implemented. Creating a new macro for this would reduce code duplication and make future extensions easier.
Overall, this is a good addition that expands YARP's Python bindings to important navigation functionalities.
| %include <yarp/dev/ILocalization2D.h> | ||
| %include <yarp/dev/IMap2D.h> | ||
| %include <yarp/dev/INavigation2D.h> | ||
| %include <yarp/dev/Map2DLocationData.h> | ||
| %include <yarp/dev/Map2DObjectData.h> | ||
| %include <yarp/dev/Map2DPathData.h> | ||
| %include <yarp/dev/Map2DAreaData.h> | ||
| %include <yarp/dev/Map2DLocation.h> | ||
| %include <yarp/dev/Map2DObject.h> | ||
| %include <yarp/dev/Map2DPath.h> | ||
| %include <yarp/dev/Map2DArea.h> | ||
| %include <yarp/dev/MapGrid2D.h> | ||
| %include <yarp/dev/NavTypes.h> |
There was a problem hiding this comment.
The new %include directives for navigation interfaces and types are not sorted alphabetically within their logical groups. Sorting them would improve readability and maintainability of this file.
%include <yarp/dev/ILocalization2D.h>
%include <yarp/dev/IMap2D.h>
%include <yarp/dev/INavigation2D.h>
%include <yarp/dev/Map2DAreaData.h>
%include <yarp/dev/Map2DLocationData.h>
%include <yarp/dev/Map2DObjectData.h>
%include <yarp/dev/Map2DPathData.h>
%include <yarp/dev/Map2DArea.h>
%include <yarp/dev/Map2DLocation.h>
%include <yarp/dev/Map2DObject.h>
%include <yarp/dev/Map2DPath.h>
%include <yarp/dev/MapGrid2D.h>
%include <yarp/dev/NavTypes.h>
| yarp::dev::Nav2D::INavigation2D *viewINavigation2D() { | ||
| yarp::dev::Nav2D::INavigation2D *result; | ||
| self->view(result); | ||
| return result; | ||
| } | ||
|
|
||
| yarp::dev::Nav2D::ILocalization2D *viewILocalization2D() { | ||
| yarp::dev::Nav2D::ILocalization2D *result; | ||
| self->view(result); | ||
| return result; | ||
| } | ||
|
|
||
| yarp::dev::Nav2D::IMap2D *viewIMap2D() { | ||
| yarp::dev::Nav2D::IMap2D *result; | ||
| self->view(result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
These view methods for Nav2D interfaces are manually implemented, leading to code duplication. This is similar to what the CAST_POLYDRIVER_TO_INTERFACE macro does for interfaces in the yarp::dev namespace.
To improve maintainability and reduce code duplication, you could define a new macro that handles interfaces within a specific namespace. The comment on line 732 suggests this was considered, but no macro was implemented.
For example, you could define a macro like this somewhere appropriate:
%define CAST_POLYDRIVER_TO_INTERFACE_IN_NAMESPACE(ns, interface)
ns::interface *view ## interface() {
ns::interface *result;
self->view(result);
return result;
}
%enddefAnd then replace the manual implementations with macro calls. This would make the code cleaner and easier to extend with more namespaced interfaces in the future.
yarp::dev::Nav2D::INavigation2D *viewINavigation2D();
yarp::dev::Nav2D::ILocalization2D *viewILocalization2D();
yarp::dev::Nav2D::IMap2D *viewIMap2D();
|
2 participants
Bindings
devINavigation2D,ILocalization2D,IMap2Dyarp::dev::INavigation2D,yarp::dev::IMap2D,yarp::dev::ILocalization2Dinterfaces