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

Add C++ visualization API and some macros for scalar contexts #2574

Merged
merged 32 commits into from
Feb 17, 2025

Conversation

ManifoldFR
Copy link
Member

@ManifoldFR ManifoldFR commented Feb 12, 2025

This pull request upstreams the contents of the single-header library Simple-Robotics/pinocchio-visualizers, as discussed on issue #2573.
This API reproduces the behaviour of the existing Pinocchio Python bindings' pinocchio.visualizers.BaseVisualizer class (which is in Python).

  • Add header <pinocchio/visualizers/base-visualizer.hpp>
  • Add new CMake target pinocchio_visualizers
  • Add namespace pinocchio::visualizers
  • Add base class pinocchio::visualizers::BaseVisualizer, for the context::Scalar scalar type. Common functionality (updating geometry placements, handling frames, the viz.play(qs) function) is compiled into the new pinocchio_visualizers target.
  • Add pinocchio::python::VisualizerPythonVisitor visitor for exposing visitor classes

Resolves #2573

Lingering questions

  • As a way of forcing implementers to implement the right functions, should we:
    • Keep using a virtual class with pure virtual members?
    • Or just have the API throw a runtime error if unimplemented members get called?
    • Or expose a CRTP class with the actual common logic implemented in a private base class?
  • Better name or mechanism for the protected virtual member function BaseVisualizer::displayPrecall() - the idea is to be able to inject a hook before the kinematics and geometry get updated, e.g. locking the GeometryData using a mutex.
  • Whether to name all of the unimplemented functions XXXImpl() or XXX_impl() and whether to make them protected.
  • How to fuse this into the actual Python BaseVisualizer class, which we could significantly cut down.

@ManifoldFR ManifoldFR self-assigned this Feb 12, 2025
@ManifoldFR ManifoldFR requested a review from jcarpent February 12, 2025 14:02
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:

  • build_collision (build Pinocchio with coal support)
  • build_casadi (build Pinocchio with CasADi support)
  • build_autodiff (build Pinocchio with CppAD support)
  • build_codegen (build Pinocchio with CppADCodeGen support)
  • build_extra (build Pinocchio with extra algorithms)
  • build_mpfr (build Pinocchio with Boost.Multiprecision support)
  • build_sdf (build Pinocchio with SDF parser)
  • build_accelerate (build Pinocchio with APPLE Accelerate framework support)
  • build_all (build Pinocchio with ALL the options stated above)

Thanks.
The Pinocchio development team.

@ManifoldFR ManifoldFR force-pushed the topic/cpp-visualization-api branch 4 times, most recently from 54ea6cf to 7efa24f Compare February 13, 2025 10:35
@ManifoldFR ManifoldFR force-pushed the topic/cpp-visualization-api branch from 26a0002 to 198c360 Compare February 13, 2025 17:47
@ManifoldFR ManifoldFR marked this pull request as ready for review February 13, 2025 19:22
@ManifoldFR
Copy link
Member Author

I ended up introducing a new pinocchio::pinocchio_visualizers CMake target which is a shared library for the single base-visualizer.cpp file.

Do you think this is reasonable @jorisv ? We might rework or extend the API in the future, not sure where it would fit right now (outside of the pinocchio/utils headers which are not their own target but shared between core and parsers).

jcarpent
jcarpent previously approved these changes Feb 14, 2025
Copy link
Contributor

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ManifoldFR
Copy link
Member Author

ManifoldFR commented Feb 14, 2025

I forgot to update the README... I've fixed that and rebased on devel.

@ManifoldFR ManifoldFR force-pushed the topic/cpp-visualization-api branch 2 times, most recently from cf27f0a to 0005b31 Compare February 14, 2025 17:57
@ManifoldFR ManifoldFR marked this pull request as draft February 14, 2025 18:02
@ManifoldFR ManifoldFR force-pushed the topic/cpp-visualization-api branch from 9f73685 to 53fb70c Compare February 14, 2025 18:08
@ManifoldFR ManifoldFR marked this pull request as ready for review February 14, 2025 18:08
@ManifoldFR ManifoldFR force-pushed the topic/cpp-visualization-api branch from cab985c to 97d6752 Compare February 17, 2025 12:18
@ManifoldFR
Copy link
Member Author

I think this is ok to merge @jcarpent :)

@ManifoldFR ManifoldFR changed the title Add C++ visualization API Add C++ visualization API and some macros for scalar contexts Feb 17, 2025
@jcarpent jcarpent merged commit 6b614ba into stack-of-tasks:devel Feb 17, 2025
18 of 26 checks passed
@ManifoldFR ManifoldFR deleted the topic/cpp-visualization-api branch February 17, 2025 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a basic C++ visualization API / upstream Simple-Robotics/pinocchio-visualizers
2 participants