Allow upcasting

[captain-yoshi]

Unit tests with desired behavior from #943.

Any Tests Failures

/home/captain-yoshi/ws/ros/mimik_ws/src/BehaviorTree.CPP/tests/gtest_any.cpp:271: Failure
Expected: auto res = a.cast<std::shared_ptr<Greeter>>() doesn't throw an exception.
  Actual: it throws.
/home/captain-yoshi/ws/ros/mimik_ws/src/BehaviorTree.CPP/tests/gtest_any.cpp:273: Failure
Value of: a.castPtr<std::shared_ptr<Greeter>>()
  Actual: false
Expected: true

Blackboard Test Failure

C++ exception with description "The creation of the tree failed because the port [hello_greeter] was initially created with type [std::shared_ptr<HelloGreeter>] and, later type [std::shared_ptr<Greeter>] was used somewhere else." thrown in the test body.

[captain-yoshi]

There maybe a better way to allow type casting of a polymorphic class.

​This pull request introduces support for upcasting and downcasting std::shared_ptr instances (must be polymorphic and has their base registered) within the Any class, facilitating polymorphic type handling. By leveraging type traits, the Any class can now perform safe and efficient conversions between shared pointers of related types.

• Introduced any_cast_base<T> trait to register base types for polymorphic casting.
• Added is_shared_ptr and IsPolymorphicSharedPtr traits to detect eligible types.
• Modified Any to store shared_ptr as its registered base class if available.
• Enhanced castPtr() and tryCast() to support safe downcasting from stored base class to derived class using static or dynamic pointer casting.
• Ensures shared_ptr can be stored and retrieved as shared_ptr when registered.

So users who wants this behavior needs to register their classes to it's base class. I don't think we can get away without this process...

// Register cast base type for self to allow direct cast, otherwise defaults to a dynamic cast
template <>
struct BT::any_cast_base<Greeter>
{
  using type = Greeter;
};

// Register cast base type for HelloGreeter
template <>
struct BT::any_cast_base<HelloGreeter>
{
  using type = Greeter;
};

Optionally we could add a macro to simplify this process (not in this PR):

#define BT_REGISTER_BASE_TYPE(Derived, Base)    \
  template <>                                   \
  struct BT::any_cast_base<Derived>             \
  {                                             \
    using type = Base;                          \
  };
  
BT_REGISTER_BASE_TYPE(Greeter, Greeter)
BT_REGISTER_BASE_TYPE(HelloGreeter, Greeter) 

It's a little bit more permissive, then what I intended initially. You can now downcast to something that is invalid (must still be a valid class of the stored base class), e.g. the blackboard will not see a mismatch, but it will fail when trying to retrieve it in a BT::Node.

Regarding the previous point, should we try to downcast early, in the blackboard to stop the tree from running early if it is invalid ? Instead of running the tree and let it fail down the line ?

---

The Sonarcube check fails, but it does not seem related to this PR.

[captain-yoshi]

The castPtr function is a little bit tricky when downcasting...

I would prefer solution 2 or 3.

Solution 1

Returns a T::element_type* castPtr() when T is a shared pointer with a polymorphic class and a registered base. For everything else T* is returned.

Pros	Cons
No complexity	2 different return types depending on the type. Cannot retrieve the shared pointer, only access to the raw pointer.

Implemented in 3021ff0

Solution 2

Clean API: T* castPtr() for all types.

Pros	Cons
Returns T*	Need to store a std_shared<void> member to return a pointer to a Derived class. Lifetime of pointer not garanteed*, another call to castPtr may change this cached value. *If getLockedPortContent is used, there is no problem.

Implemented in d11512a

Solution 3

Seperate castPtr function when T is a shared pointer with a polymorphic class and a registered base, e.g. castShared.

Pros	Cons
No complexity	User has 2 different functions depending on what is stored in the _any.

Not implemented

[facontidavide]

There is a conflict that need to be resolved. Also, correct me if i am wrong, but isn't this change allowing two ports with derived classes to connect?

For instance, given Cat <- Animal and Dog <- Animal, I would be allowed to connect a Cat to a Dog...

[Copilot]

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

[nitpick] When Base and Derived are identical, consider using static_cast instead of reinterpret_cast to improve type safety and clarify conversion intent.

Suggested change

	return reinterpret_cast<T*>(base_ptr);
	return static_cast<T*>(base_ptr);

[captain-yoshi]

For instance, given Cat <- Animal and Dog <- Animal, I would be allowed to connect a Cat to a Dog...

At the moment yes. If we want to make it fail when creating a Tree node from xml, we would need to store the type id of the base class and the original class, because at this stage, I think the any values are still empty.

But trying to have a SphinxCat into a registered Cat port would also fail because only the the SphinxCat and Animal typeid would be stored when registering SphinxCat. We could fix this by storing a vector (or limit with a max depth) of typeid's from current class to root base class to enable this case.

I would also prefer to make it fail early when trying to connect incompatible types. What wold be the way to go ? Only allow a depth of 2 base classes, allow a max range of depth or allow any depth ? Or something else entirely...

[captain-yoshi]

I have now replaced the Greeter hierarchy with the Animal one (easier too understand).

I added some new tests. Cat <- Animal and Dog <- Animal now throws when constructing the TreeNode.

---

I think the last thing missing is the serialization of the TreeNodeModel in XML ? I think we would need to store the chain base, e.g. :

<output_port name="collision_object"
             type="std::shared_ptr&lt;Sphinx&lt;std::allocator&lt;void&gt; &gt; &gt;"
             base_chain="Sphinx;Cat;Animal" />

Do we need something for deserializing the base_chain ? Is this even needed, I got the impression that we always need to register the node type in the BehaviorTreeFactory...

[facontidavide]

thanks for working on this.

the proposed solution seems to work but it adds some (necessary) complexity and I need to take my time (something that right now I don't have in abundance, to think about alternatives... if they exist.

Please be patient, i will look at this in the next couple of weeks.