Casting down in class hierarchy

[DerThorsten]

Hi, I have a base class with virtual functions "b2Shape" and a derived class "b2PolygonShape"
and i exported them as suggested (using the "extra" pyb2Shape class/trampoline) and everything works well.

But at some point i need to "cast" a b2Shape ptr to a b2PolygonShape.
I used the following code.

pybox2dModule.def("b2PolygonShapeCast",[](b2Shape * shape) -> b2PolygonShape * { b2PolygonShape * ps = dynamic_cast<b2PolygonShape*>(shape); return ps; });

But I always get back a b2Shape object on the python side.
So far i have no idea why this should not work.
Is this a bug or intended?
If intended, whats the best way of doing such a "downcast"

[wjakob]

When Pybind11 sees an object with a pointer representation that has been turned into a Python object before, it will return that Python object instead of doing another conversion. This is to avoid serious headaches with reference counting (imagine two different Python objects pointing to the same memory region). If this bothers you, you could create a copy or return the desired type in the first place that created the object.

[DerThorsten]

the problem it, the API of Box2D just returns the base type, I noticed that a copy does the right thing, but
a copy will not do the trick in most cases.
So this seems to be a show stoper :(

edit: I'll think about some hacks and will report back if you are interested
edit edit: It is a show stopper.... if there is no way of doing a downcast without doing a copy one cannot do many cool things... :(

[wjakob]

This kind of thing is simply not intended by the current design, and changing that might complicate the library quite a bit (more involved reference counting & can have multiple different types associated with one pointer).

[DerThorsten]

Sure, I agree that this might complicate things.
But if use it for downcasting with py::keep_alive, I see no problem at all with reference counting.
I guess not many change would be needed to allow things like that.
But the benefit would be great.

[wjakob]

In this simple case, py::keep_alive would work indeed to keep things consistent. I'm more worried about the case where a function that does something more involved than just casting suddenly returns registered objects using a different type.

[wjakob]

I guess one potential solution will be to extend the registered_instance hashtable to include the type in addition to the instance pointer as key. It will be a fair bit of work though and is not something that I need for my own projects, so I fear that you are on your own.

[DerThorsten]

I am willing to do the work.
I'll search for ' registered_instance' in the code and will have a look at it.
Thank you very much for the help.

[DerThorsten]

I found a solution which is good enough for my self.
I introduced a new return_value_policy (with a very stupid name so far) py::return_value_policy::dont_cache_cast

And I made a few hacks in cast.h in the cast function.
If the policy is the new one, i set dont_cache to true, and I added a new bool to instance<void>, namely
isCasted which is false by default and true if dont_cache_cast is used.

In the dealloc function I check if the isCasted is true, if so, i do not deallocate

The actual casting function now looks like:

pybox2dModule.def("b2PolygonShapeCast",[](b2Shape * shape) -> b2PolygonShape * { b2PolygonShape * ps = dynamic_cast<b2PolygonShape*>(shape); return ps; }, py::return_value_policy::dont_cache_cast, py::keep_alive<1,0>() );
Maybe this trick is good enough for more guys than just me.
I can clean up my repo and make a pr

[wjakob]

As you mention this is a hack -- for a PR, it would need to be a more generic and clean solution.

[DerThorsten]

Would you consider to add the "isCasted" bool to instance<void>.
If so, i can implement a solution which is bullet proof from the users point of view.

I am thinking about the following:
I add a new function to class_, namely 'def_dynamic_cast' which is templated to the type it should cast to.
One can check if that type is in fact a derived type.
Internally I would to pretty much the same as described above ( dont_cache + keep_alive).

I guess one cannot use this wrong or mess up things?

Implemented it: code looks now like

auto shapeCls = py::class_<PyB2Shape>(pybox2dModule,"b2Shape");
shapeCls
    .alias<b2Shape>()
    .def(py::init<>())
    .def_dynamic_cast<b2Shape,b2PolygonShape>("AsPolygonShape")
;
py::class_<b2PolygonShape>(pybox2dModule,"b2PolygonShape",shapeCls)
        .def(py::init<>())
;

I would say from the users point of view that solution would be acceptable, under the hood I still do it as described, but it does not change much and can be replaced with a better solution without users ever knowing.

[DerThorsten]

I can set up an example and a PR, just in case you want to have a detailed look at It.

[wjakob]

To consider adding such a feature, I'd really be looking for a way to make it work in any setting, not just a cast. This means that the underlying hash data structure must be extended as I mentioned at the beginning of the ticket, I don't see another way.

[apmccartney]

As a work-around for the interum, dynamic casts can be refactored away using a visitor pattern.

[DerThorsten]

could you give an example?

@wjakob I sped a few hours trying to extend the underlying hash data structure.
I can setup a branch with where I got stuck.
I'll re-look into into It soon and might ask question ;)

[wjakob]

So it turns out that there is a very simple way to always return the best variant of a polymorphic type without the uglyness of a special casting function, which can be realized in just a few lines of code. This should hopefully resolve this issue.