Skip to content

Conversation

@topokel
Copy link
Member

@topokel topokel commented Oct 4, 2019

Specs-physics 0.4 Rewrite

This rewrites the specs-physics API to remove unnecessary synchronization and copying by implementing the new BodySet and ColliderSet traits with Component Storages, indexing handles for each object type using Entity. This new API reduces maintenance burden, complexity, and better exposes the entirety of nphysics, all while likely improving performance.

Background

I originally planned to more directly integrate with the traits provided by nphysics (such as with concrete storages for different body types, that together implement the set traits, or even borrowing a custom rigidbody type and its associated position together to implement Body, eliminating the local position data and associated pose synchronization), reducing indirection and making access more simple, but this proved to be difficult, maybe impossible, to implement. And it didn't seem if such an approach was beneficial enough for its maintenance costs.

However, I came up with the idea of doing this the opposite direction with the join extension trait API, and found that it could be a very nice solution that would make the crate a lot easier to maintain. This led to me opening dimforge/nphysics#237 when I discovered that given the BodySet parameters were removed from some types, I could very simply implement BodySet (albeit using Box<dyn Body>) and other associated Set types over Component Storages.

Changes

  • Adds support for 2D from @jmeggitt's PR.
  • Updates to latest nphysics/ncollide
  • Replaces old Body API with BodySet and ColliderSet implemented as Storages, with object handles implemented using the Entity type.
  • Removes parameter bits and their synchronization in lieu of simply exposing new nphysics resources.
  • Removes collider bits since bodies and their colliders are now easily addressable by the Entity type.

To Do

While the API itself is ready for use and review, I still have some of all of the following stuff to do:

  • Update documentation
  • Update examples
  • Add Amethyst example from my crusty old Amethyst example branch
  • Provide a nice EntityBuilder extension API Bodies and Colliders
  • Refactor pose synchronization

However, the bulk of the work is done, especially the spooky (happy October) unsafe blocks in handle.rs.

One might say that the library definitely shouldn't be called nphysics-ecs-dumb any longer!

@topokel
Copy link
Member Author

topokel commented Oct 22, 2019

This latest commit is based on the work in dimforge/nphysics#237 (I cannot reference that git branch from my manifest due to the crates themselves being in subdirectories of the repository). This fixes compilation errors in the library, and now what's remaining is to clean up the API and get the examples running.

@topokel
Copy link
Member Author

topokel commented Oct 24, 2019

The basic example is now fully updated and works with a nice EntityBuilder extension trait! Also, the fixed stepping bits from previous versions have returned, along with a new addition in the batch dispatcher inspired by @AndreaCatania (although I have yet to tango with it in full) While some specific pieces of the API are yet to be ironed out (BodyPartHandle component with pose sync, joins with automatic casting for builtin Body types like RigidBody, and implementing JointConstraintSet and ForceGeneratorSet as Storages), the core part of the API is done, and this PR is now ready for review! Just please mind the documentation and remaining examples which have not been updated. Also note that this PR uses a temporary Git submodule since this API is made possible by dimforge/nphysics#237, which has not landed.

@topokel topokel marked this pull request as ready for review October 24, 2019 07:20
@topokel topokel changed the title [WIP] 0.4.0 0.4.0 Rewrite Oct 24, 2019
@topokel
Copy link
Member Author

topokel commented Oct 27, 2019

The nphysics PR has landed WOOT! Will remove the git submodules with the next nphysics release.

@robert-w-gries
Copy link

Have you built with commit 51ce6c8? I'm not able to build using that refpoint

@robert-w-gries
Copy link

I was able to build after the following changes

@topokel
Copy link
Member Author

topokel commented Nov 4, 2019

Apologies... I posted incomplete work because I was rushing out the door. Ignore the amethyst dep stuff, that's just me writing an example with the amethyst nalgebra bump.

@robert-w-gries
Copy link

No worries, I was able to integrate the changes from this PR + my diff into my game project and it worked well! Overall, I was very happy with the cleaner API and the updated dependencies.

There were a couple surprises when I found that 1) I needed to manually sync the Transform::isometry() to all BodyComponents and 2) I needed to insert various Resources when I created my own PhysicsBundle. I am okay with these changes to behavior and don't think anything needs to be changed from your PR. Maybe add a note about the behavior changes to the release documentation?

@topokel
Copy link
Member Author

topokel commented Nov 4, 2019

  1. You're very right I need to document this but I will also personally recommend against writing code like this in the migration docs. If you have a Body/Transform which are sync'd in the Pose system, you should simply just change the position of the Body directly! I'm adding Marker types to make joining over RigidBodies etc more ergonomic so this will be easier since you can't mutate a Body/BodyPart's position generically anyways.

  2. PhysicsBundle should add everything necessary for you, could you post your code where you needed to manually insert a resource? That'd be a bug imo.

Thanks for testing stuff out and posting feedback! I super appreciate it 💜

@topokel topokel force-pushed the 0.4.0 branch 2 times, most recently from 1e5b7bf to 815878e Compare November 12, 2019 06:04
@Uriopass
Copy link

Uriopass commented Dec 9, 2019

I get an error when I try the build this branch

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
  --> C:\Users\paris\.cargo\git\checkouts\specs-physics-8734272910d5c1dd\30331dc\src\systems\physics_stepper.rs:18:24
   |
18 | impl<'a, N: RealField> System<'a> for PhysicsStepperSystem<N> {
   |                        ^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the impl at 18:6...
  --> C:\Users\paris\.cargo\git\checkouts\specs-physics-8734272910d5c1dd\30331dc\src\systems\physics_stepper.rs:18:6
   |
18 | impl<'a, N: RealField> System<'a> for PhysicsStepperSystem<N> {
   |      ^^
note: ...so that the types are compatible
  --> C:\Users\paris\.cargo\git\checkouts\specs-physics-8734272910d5c1dd\30331dc\src\systems\physics_stepper.rs:18:24
   |
18 | impl<'a, N: RealField> System<'a> for PhysicsStepperSystem<N> {
   |                        ^^^^^^^^^^
   = note: expected  `shred::system::System<'a>`
              found  `shred::system::System<'_>`
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the type `nphysics2d::world::mechanical_world::MechanicalWorld<N, world::BodySet<'_, N>, specs::world::entity::Entity>` will meet its required lifetime bounds
  --> C:\Users\paris\.cargo\git\checkouts\specs-physics-8734272910d5c1dd\30331dc\src\systems\physics_stepper.rs:19:5
   |
19 | /     type SystemData = (
20 | |         Entities<'a>,
21 | |         WriteExpect<'a, MechanicalWorldRes<'a, N>>,
22 | |         WriteExpect<'a, GeometricalWorldRes<N>>,
...  |
28 | |         Write<'a, ProximityEvents>,
29 | |     );
   | |______^

What am I doing wrong ? I'm using
specs-physics = {git = "https://github.com/amethyst/specs-physics/", branch = "v0.4.0", default-features = false, features=["dim2"]}
to import the crate. (Tested with stable and nightly)

@robert-w-gries
Copy link

Use the following instead:

- specs-physics = {git = "https://github.com/amethyst/specs-physics/", branch = "v0.4.0", default-features = false, features=["dim2"]}
+ specs-physics = {git = "https://github.com/distransient/specs-physics/", branch = "0.4.0", default-features = false, features=["dim2"]}

@AnneKitsune
Copy link
Contributor

I can't possibly review this, its way too big.

What's the status on this? If its in working condition I'll port my project to it and see how that all works.

@topokel
Copy link
Member Author

topokel commented Dec 26, 2019

Let me push real quick and I'll ping you on Discord.


#[cfg(all(feature = "amethyst", feature = "dim2"))]
impl Pose<f32> for amethyst::core::Transform {
fn sync(&mut self, pose: &Isometry<N>) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't compile with ["dim2", "amethyst"]. Changing this line to

Suggested change
fn sync(&mut self, pose: &Isometry<N>) {
fn sync(&mut self, pose: &Isometry<f32>) {

seems to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drager
Copy link

drager commented Feb 24, 2020

What's the status of this PR? By the looks of the description of the PR this seems almost finished? Would it make sense to start building towards your fork in the meantime @distransient?

This was referenced Apr 10, 2020
@AnneKitsune
Copy link
Contributor

We need to complete this soon. :)

bors bot added a commit that referenced this pull request Apr 29, 2020
31: Upgrade specs-physics to latest nphysics r=jojolepro a=willcrichton

Since #23 seems to have stagnated, I went ahead and updated specs-physics to the latest nalgebra/nphysics versions. It passes all the tests in the repo, but I haven't attempted a larger integration into Amethyst yet.



Co-authored-by: Will Crichton <wcrichto@cs.stanford.edu>
@tristanpemble
Copy link

what is left to be done?

@AnneKitsune
Copy link
Contributor

What's left: resolve the conflicts with master + test & fix issues.

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.

8 participants