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

No equivalent "IsDestroyed" method in World interface #561

Closed
opera-aberglund opened this issue Dec 27, 2023 · 6 comments
Closed

No equivalent "IsDestroyed" method in World interface #561

opera-aberglund opened this issue Dec 27, 2023 · 6 comments
Assignees
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug. Library For issues that effect the library and aren't specific to any particular application.
Milestone

Comments

@opera-aberglund
Copy link

The "IsDestroyed" methods in AabbTreeWorld doesn't exist in the World interface. For example:

bool IsDestroyed(const AabbTreeWorld& world, BodyID id) noexcept
{
    return world.m_bodyBuffer.FindFree(to_underlying(id));
}

There should be something like:

bool IsDestroyed(const World& world, BodyID id)
{
    return world.m_impl->IsDestroyed_(id);
}

and equivalent in the WorldConcept and WorldModel...

@louis-langholtz
Copy link
Owner

louis-langholtz commented Jan 3, 2024

Out of curiosity, what flavor of interface for this info would you prefer:

  1. IsDestroyed(const World&, EntityID).
  2. entity.IsDestroyed().
  3. std::optional<Entity> GetEntity(const World&, EntityID).

@opera-aberglund
Copy link
Author

Out of curiosity, what flavor of interface for this info would you prefer:

  1. IsDestroyed(const World&, EntityID)
  2. entity.IsDestroyed()
  3. std::optional GetEntity(const World&, EntityID)

I would prefer 1 I think because it matches the signature of the Destroy function.

@louis-langholtz
Copy link
Owner

I would prefer 1 I think because it matches the signature of the Destroy function.

Interesting to me that option 3 didn't get mentioned. Interesting in terms of discussion, because while I'm a fan of C++ vocabulary types like std::optional, I'm also concerned about minimizing memory usage.

Specifically, making the underlying storage arrays that the implementing world uses be arrays of std::optional<T> instead of arrays of T seems more likely to be suboptimal bit-density wise than another possibility I'm thinking of.

In this case, PlayRho types like Body and Contact already provide boolean properties stored directly in bits packed optimally. That leads most naturally for me to options 1 and 2.

The more abstracted PlayRho types Shape and Joint don't seem to have as optimal, or, at least, not such a straight forward path for providing an is-destroyed piece of info. Perhaps the most straight forward to me is using their has_value member function for this. That way, supporting types (like DiskShapeConf for example), don't have to all change their interfaces to support this.

So I think that's what I'll do then, and why. I.e. provide interface 1 (and 2) for the PlayRho Body, Contact, Joint, Shape types. Assuming of course that this works out; I'm concerned I may yet encounter wrinkles with this design.

Relatedly from my thinking, and see issue #547, I'm continuing to consider removing all of the World based functions GetBodies(const World&), GetContacts(const World&), GetJoints(const World&) (and their implementation based counterparts).

@louis-langholtz louis-langholtz self-assigned this Jan 7, 2024
@louis-langholtz louis-langholtz added Enhancement For suggestions or changes that enhance any part of the project and isn't a bug. Library For issues that effect the library and aren't specific to any particular application. labels Jan 7, 2024
@louis-langholtz louis-langholtz added this to the 2.0 Release milestone Jan 7, 2024
@louis-langholtz louis-langholtz added this to To Do in 100% Unit Test Coverage via automation Jan 7, 2024
@louis-langholtz louis-langholtz added this to To Do in Slim Down World Class via automation Jan 7, 2024
@louis-langholtz louis-langholtz added this to To do in Complete == and != operator support via automation Jan 7, 2024
@louis-langholtz louis-langholtz added this to To do in Support serialization/deserialization via automation Jan 7, 2024
@louis-langholtz
Copy link
Owner

The following is available now in current master branch:

  • IsDestroyed(const Body&).
  • IsDestroyed(const Contact&).
  • IsDestroyed(const Shape&).
  • IsDestroyed(const Joint&).
  • IsDestroyed(const World&, BodyID id).
  • IsDestroyed(const World&, ContactID id).

Relatedly, manifolds would use the info for contact.

I still have to add the following however - I'd forgotten these but meant to add these as well:

  • IsDestroyed(const World&, JointID id).
  • IsDestroyed(const World&, ShapeID id).

@opera-aberglund
Copy link
Author

The following is available now in current master branch:

Funny thing I just realised I don't need them anymore since I just use the MakeTouchingMap function now to filter the contacts. I think it's good to have them anyway!

@louis-langholtz
Copy link
Owner

This is complete now with PR #566.

100% Unit Test Coverage automation moved this from To Do to Done Jan 10, 2024
Slim Down World Class automation moved this from To Do to Done Jan 10, 2024
Complete == and != operator support automation moved this from To do to Done Jan 10, 2024
Support serialization/deserialization automation moved this from To do to Done Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug. Library For issues that effect the library and aren't specific to any particular application.
Development

No branches or pull requests

2 participants