Fix copy of TrajOptProblem

[abussy-aldebaran]

Hi,

When I tried to run the solo12 scenario from https://gitlab.laas.fr/gepetto/quadruped-reactive-walking, I got crashes from aligator.
I identified the following bug : the pointer init_state_error_ in TrajOptProblemTpl was not updated across copy/move, which could result in wrong values or crashes.
I tried to make TrajOptProblemTpl non copyable nor movable, but I couldn't find a fix that would not break the python binding.

So I went for the obvious fix. Please let me know if you prefer another solution.

[ManifoldFR]

Hi @abussy-aldebaran, thanks for using aligator! And thanks for opening a PR 😁

This class is definitely expected to be copyable and movable. I guess we were missing a test for that though!

I was a bit surprised when reading your issue that the pointer wasn't properly copied over.
But thinking about it, it's because the pointer would always be pointing at... The initial condition from the old TrajOptProblem object. Because pointers are addresses 🙄

As for your fix, I'd prefer letting the compiler generate the ctor and operators, because it'd be easier to maintain and update this class. The problem is that pointers aren't "regular" values semantically speaking.

We could find a better way to cache the fact we can downcast the pointer to the initial error. I initially just stored the downcasted pointer to avoid dynamic_cast at runtime.

What we could do is store a flag which indicates the initial condition is a StateErrorResidual, and perform static_casts at runtime at no extra cost

What do you think?

[abussy-aldebaran]

I agree, I was not a fan of adding the ctor/assignement and rather stick to the rule of zero, for the reason you mentionned.
I had considered your idea, but it wouldn't work in case of multiple inheritance, where the dynamic_cast would work, but not return the same pointer as a static_cast (see https://godbolt.org/z/zhT7hnPb8).

But maybe you don't need to support multiple inheritance ? A more robust solution could be to use an enum, instead of a flag, to indicate which cast should be perform, e.g. None, Static, Dynamic. This enum would be set by comparing the results of static_cast and dynamic_cast at construction

• if dynamic_cast == nullptr -> None
• if dynamic_cast == static_cast -> Static
• if dynamic_cast != static_cast -> Dynamic

Then we would perform the right cast when accessing the initial condition. The optimization would be lost in the case of multiple inheritance.

Maybe that's what you had in mind ?

Note that I'm not an expert in polymorphism/inheritance 🙄 .
If this solution if ok with you, I will update the PR.

[ManifoldFR]

I think we just need to have a Boolean flag which states whether the initial condition is a StateErrorResidual. There's no use for (the troublemaker concept of) multiple inheritance here.

Once we have that flag we can just static_cast our way to victory.

[ManifoldFR]

Thanks for the changes @abussy-aldebaran ! I just have some modifications to suggest. What do you think !

[ManifoldFR]

Also @abussy-aldebaran, could you please format your code? You just need to install pre-commit from either pip or Conda, and run pre-commit run --all --verbose locally.

[abussy-aldebaran]

I added your suggestions. I also added:

1. an assert in initCondIsStateError()
2. an extra check in checkIntegrity()

[ManifoldFR]

Thanks a lot for this PR @abussy-aldebaran !

Feel free to add yourself to the README if you want (especially if you think you would send in more contributions).

[abussy-aldebaran]

You're welcome ! Thanks for your reactive feedback !