PlayRho icon indicating copy to clipboard operation
PlayRho copied to clipboard

Remove all public functions that take a non-const Body reference.

Open LoneBoco opened this issue 2 years ago • 1 comments

Expected/Desired Behavior or Experience:

When learning how to use PlayRho, I would expect to not become trapped by finding functions that are unusable. It is confusing and takes time trying to figure out that this is not the expected way to use the library.

Actual Behavior:

You have functions like this hanging around: https://github.com/louis-langholtz/PlayRho/blob/a3a91554cf9b267894d06a996c5799a0479c5b5f/PlayRho/Dynamics/Body.hpp#L1211-L1214

As far as I can tell, there is no way to acquire a non-const Body, so functions like these just pollute the namespace and are beginner traps. I spent a lot of time trying to figure out how to alter bodies and these functions really threw me for a loop.

LoneBoco avatar Jul 04 '22 18:07 LoneBoco

Hi LoneBoco. Thank you for raising the issue. I agree with what you’re saying. I’d started work on this a while back actually. I haven’t had the time however to finish reorganizing things though.

louis-langholtz avatar Jul 07 '22 00:07 louis-langholtz

I just realized that I may have partly overlooked what you wrote in terms of my previous response.

Using a Body with a World

As far as I can tell, there is no way to acquire a non-const Body

Any of the following should work to acquire a non-const Body:

#include <PlayRho/PlayRho.hpp>

int main()
{
    static auto bodyWithStaticStorageDuration = playrho::d2::Body{};
    auto bodyWithAutomaticStorageDuration = playrho::d2::Body{};
    auto* bodyWithDynamicStorageDuration = new playrho::d2::Body{};
}

Do none of these work for you?

Is the confusion with relating a Body instance with a World instance? Things are different in this regard from Box2D as PlayRho is more value semantic oriented. This was also something that did seem like an issue to me in the pre-2.0 code interfaces which I've changed in the current master branch code.

For example...

In the release-1.1 branch code, a World instance takes a BodyConf to create a body while it takes a Body to override the current state of a created body. I.e. there's these two functions which are asymmetric in this regard:

  • BodyID World::CreateBody(const BodyConf& def = GetDefaultBodyConf());.
  • void World::SetBody(BodyID id, const Body& value).

In the master branch code however this has been changed to the following:

  • BodyID World::CreateBody(const Body& body);
  • void SetBody(BodyID id, const Body& value);

Note that there's also the following functions available to convert between a Body and a BodyConf:

Body::Body(const BodyConf& bd = GetDefaultBodyConf());
BodyConf GetBodyConf(const Body& body);

Unusable Functions

I would expect to not become trapped by finding functions that are unusable.

That's what I had read and was thinking about till now and I agree with you on this. The problem also comes up when creating or using the project as a dynamic library. And there's a tension in this regard against unit testing the code. I don't want users concerning themselves with implementation functions for example but I do want that code tested directly. I have it in mind and have made some starts at dealing with that.

louis-langholtz avatar Mar 08 '23 22:03 louis-langholtz

Added a new project of Reduce Namespace Pollution to address the concerns I share with this issue. Closing this particular issue in deference to this new project.

louis-langholtz avatar Apr 18 '23 01:04 louis-langholtz