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

[API breaking] Use polymorphic value type holder for manifolds and constraint sets #90

Merged
merged 93 commits into from
Sep 12, 2024

Conversation

ManifoldFR
Copy link
Member

@ManifoldFR ManifoldFR commented May 29, 2024

This PR:

  • removes use of shared_ptr for manifold and constraint set types, switching to jbcoe/value_types's polymorphic template class everywhere as a holder with value semantics
  • enhances the library's ergonomics by avoiding having to call std::make_shared<T>(...) all the time, see changed C++ examples
  • updates the examples and tests accordingly
  • adds a util header proxsuite-nlp/python/polymorphic.hpp for dealing with values/references of polymorphic<T>, register conversions, etc
  • updates Python bindings for manifolds and constraint sets accordingly
  • adds a PolymorphicVisitor to the Python bindings which registers a type Y as convertible to the template argument polymorphic<X>
  • splits up the big constraint instantiation header constraints.txx: closes Template instantiation: split up constraints.hpp and constraints.txx mega-headers #87

Known issues

  • returning a polymorphic<X> by value does not check whether the underlying object is a boost::python::wrapper<U> for some type U which inherits from X
  • this PR is a breaking change for proxsuite-nlp's C++ API

Background

This is the first in a series of PR which will cut down on our overuse of shared_ptr across both proxsuite-nlp and aligator and replace it with a holder for polymorphic objects with value semantics.

The drawbacks to the current approach with shared_ptr was:

  • no real copy constructor for shared_ptr<T>, except if we add virtual clone member functions implemented in all derived classes
  • calls to members did not satisfy const-correctness (not a problem for manifolds as they so far have no non-const member functions, but a problem for namely BoxConstraint)
  • shared mutable state

The polymorphic value type was described by Jonathan Coe: https://www.youtube.com/watch?v=sjLRX4WMvlU. It allows composite classes (with polymorphic data members) to behave well using value-semantics all the way down (with const-correctness).

Why not unique_ptr ?

unique_ptr completely removes any ability to copy the underlying object (except if a virtual clone() was implemented) and disables default copy ctor/operators for all composite classes using it.

Many thanks to @edantec for his contribution.

@ManifoldFR ManifoldFR added enhancement New feature or request api labels May 29, 2024
@ManifoldFR ManifoldFR force-pushed the topic/manifold-polymorphic-value branch from 6f83d1c to 4f007fc Compare May 29, 2024 11:45
@ManifoldFR ManifoldFR changed the title [API breaking] Use polymorphic value type holder for manifolds [API breaking] Use polymorphic value type holder for manifolds and constraint sets May 29, 2024
@ManifoldFR ManifoldFR requested a review from jorisv May 29, 2024 12:27
@ManifoldFR ManifoldFR force-pushed the topic/manifold-polymorphic-value branch 4 times, most recently from 4d33d96 to d026846 Compare May 31, 2024 15:14
@ManifoldFR
Copy link
Member Author

ManifoldFR commented Jun 3, 2024

The last thing to do before merging, which is very relevant for downstream integration into aligator, is being able to extend interfaces from Python. Right now, inheriting from bp::wrapper<PythonClass> and using the current macros doesn't seem to work anymore. We need to:

  • add this case to the test
  • find a way to fix this. AFAIK, for shared_ptr<T>, Boost.Python handles calling virtual methods implemented in Python by fetching a pointer to the Python object that is hidden in the deleter - this reference to the PyObject* is injected by the shared_ptr holder

@ManifoldFR ManifoldFR force-pushed the topic/manifold-polymorphic-value branch 4 times, most recently from 031b3fa to 2141f6a Compare June 5, 2024 09:26
@ManifoldFR ManifoldFR force-pushed the topic/manifold-polymorphic-value branch 6 times, most recently from fe0d337 to 94830ff Compare June 6, 2024 11:34
@ManifoldFR ManifoldFR requested a review from edantec June 6, 2024 14:38
Copy link
Collaborator

@edantec edantec left a comment

Choose a reason for hiding this comment

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

After quick glance, PR seems ok

examples/ur5-ik.cpp Show resolved Hide resolved
examples/ur5-ik.cpp Show resolved Hide resolved
include/proxsuite-nlp/constraint-base.hpp Show resolved Hide resolved
@ManifoldFR
Copy link
Member Author

@jorisv @edantec I think Windows is finally fixed (my god...)

@ManifoldFR ManifoldFR enabled auto-merge June 6, 2024 16:44
@ManifoldFR ManifoldFR disabled auto-merge June 6, 2024 17:19
@ManifoldFR
Copy link
Member Author

ManifoldFR commented Jun 7, 2024

While discussing with @jorisv, we found that:

  • passing the wrapper class to a function/ctor taking polymorphic<T> does not properly copy the object
  • by properly copying, we mean that the reference to the initial object is not properly handled, there's a missing incref somewhere
  • in Boost.Python, the converter class shared_ptr_from_python, which is registered in the class_metadata.hpp header, takes the source PyObject* (see here), borrows it with bp::borrowed (which casts it to a pointer to the special type bp::detail::borrowed<PyObject> (see boost/python/detail/borrowed_ptr.hpp)), and shoves it into a bp::handle<> object, the constructor of which increfs the pointer to borrowed and upcasts it back to PyObject to store it (see here)

@jorisv jorisv force-pushed the topic/manifold-polymorphic-value branch from a6ef49c to 3308bfa Compare June 25, 2024 14:50
@ManifoldFR
Copy link
Member Author

Okay, we will be merging this today. This is not the perfect solution imo, I'd be willing to look at other types of type erasure in the future for the definitive API (and rip out the OOP class hierarchies here and in aligator, downstream).
Having value semantics is already good enough though and we can make progress on other fronts.

@ManifoldFR ManifoldFR merged commit 8cd084d into devel Sep 12, 2024
36 checks passed
@ManifoldFR ManifoldFR deleted the topic/manifold-polymorphic-value branch September 12, 2024 15:43
ManifoldFR added a commit that referenced this pull request Sep 16, 2024
…c-value

[API breaking] Use polymorphic value type holder for manifolds and constraint sets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template instantiation: split up constraints.hpp and constraints.txx mega-headers
3 participants