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

Provide an API endpoint to allow users to remove actors from the scene #85

Closed

Conversation

NickCaplinger
Copy link
Contributor

Fixes #84

Hi all. The relevant information for this pull request can be found in the issue above and the commit message for
6a96fe3. I'm very much open to discussion on this. Thanks for the crate again!

Cheers!
Nick

@NickCaplinger
Copy link
Contributor Author

It looks like the Windows tests are failing due to linker errors, but I don't think it's related to my change.

This is a partial solution to a flaw in the current crate API regarding actor deletion.
Previously, a BodyHandle was created for both articulations and static/dynamic actors.
However, the remove_body function found in the physics module called remove_articulation,
regardless of the type actually pointed to by the BodyHandle. This lead to actors never being removed and a PhysX warning being printed.

I've addressed this by replacing BodyHandle with ActorHandle and ArticulationHandle, which are handled separately.
ActorHandle also has a type field that allows handles to be removed from internally tracked lists.
I'm not very confident about some of the changes around articulations, and have not used articulations in PhysX,
so I could use some thorough review/testing in that area. I think we could really benefit from some tests. :)
I by no means think this is a perfect or final solution, but want to start the feedback process early.

This is a breaking change and warrants a bump in the crate's major verison, if using semantic versioning.
@NickCaplinger
Copy link
Contributor Author

Fixed by a recent redesign of the crate. See more details here: #84

@NickCaplinger NickCaplinger deleted the nc_remove_actors branch December 5, 2020 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing an actor from a scene fails and prints a PhysX warning instead
1 participant