Partially integrated collision groups into nphysics #58

Kerollmops · 2016-04-06T16:11:39Z

Don't really know if this integration is good for you, I didn't implement this from #55:

the modification will have no effect if it is performed after the rigid body has been added to the scene). If the rigid body is static, the corresponding group should be set automatically after the user modified the group

…and advices !

Kerollmops · 2016-04-06T16:12:19Z

examples2d/nphysics_testbed2d/src/testbed.rs

@@ -94,7 +94,7 @@ impl<'a> Testbed<'a> {
            minor_version:      1
        };
        let window =
-            match RenderWindow::new(mode, "nphysics 2d demo", Close, &setting) {
+            match RenderWindow::new(mode, "nphysics 2d demo", Close, &ContextSettings::default()) {


What did I have to do about that ?

It's best to keep your modification. I believe the setting variable initialization just above could be removed now.

sebcrozet · 2016-04-06T18:17:11Z

src/object/rigid_body.rs

@@ -244,7 +247,7 @@ impl<N: Scalar> RigidBody<N> {
    }

    /// Creates a new rigid body that can move.
-    pub fn new_dynamic<G>(shape: G, density: N, restitution: N, friction: N) -> RigidBody<N>
+    pub fn new_dynamic<G>(shape: G, density: N, restitution: N, friction: N, groups: Option<RigidBodyCollisionGroups>) -> RigidBody<N>


I'd prefer not to add groups as a constructor argument. For many applications, a custom value is not needed at all.

sebcrozet · 2016-04-06T18:30:56Z

src/object/rigid_body.rs

+
+    /// Mutable reference to the collision groups of this rigid body.
+    #[inline]
+    pub fn collision_groups_mut(&mut self) -> &mut RigidBodyCollisionGroups {


I am not 100% sure but I think it is better not to return a mutable reference, but instead have something like .set_collision_groups(&mut self, new_groups: &mut RigidBodyCollisionGroups). In the future, the rigid body will have to know that the groups have been tempered with in order to notify the collision detectors.

Do you want to keep collision_groups(&self) -> &RigidBodyCollisionGroups ?

Yes, immutable access might be useful and has no impact on the physics engine anyway.

Why do you want new_groups: &mut RigidBodyCollisionGroups to be mutable ?

I don't, sorry, my mistake ^^ It should be new_groups: RigidBodyCollisionGroups of course.

Done! Implemented like this now ^^

sebcrozet · 2016-04-06T18:58:32Z

src/object/rigid_body.rs

@@ -272,7 +278,8 @@ impl<N: Scalar> RigidBody<N> {
    pub fn new(shape:           Arc<Box<Repr<Point<N>, Matrix<N>>>>,
               mass_properties: Option<(N, Point<N>, AngularInertia<N>)>,
               restitution:     N,
-               friction:        N)
+               friction:        N,
+               groups:          RigidBodyCollisionGroups)


That one should not have groups on its constructor either. On this method, the static or dynamic status of the rigid body is automatically deduced from mass_properties. If it is set to None, then the rigid body is static. It is dynamic otherwise.

Kerollmops · 2016-04-06T19:55:53Z

Do you think fn get_internal_collision_groups(&self) -> &CollisionGroups is a good function name ? or is a good thing to have ?

sebcrozet · 2016-04-06T20:11:50Z

It is a must-have so that the user can perform queries on the CollisionWorld from ncollide with those groups (without having to use the World from nphysics as an intermediate).

As for the name, I'd say collision_groups(...) is enough... unless you find it confusing in which case I am okay with internal_collision_groups(...). The choice is yours! In any case, the get_ prefix has to be dropped as it is not idiomatic in rust.

Kerollmops · 2016-04-06T20:16:41Z

The function is mainly there to help world::add_body 😄
But you are right, the users will need this too !

Kerollmops · 2016-04-06T20:18:04Z

Do you think world/collision_groups.rs is in the right place ?

sebcrozet · 2016-04-06T20:18:06Z

Actually, it might be even clearer to name it as_collision_groups(...) (prefix as_), or, even better, to implement the Deref trait instead !

sebcrozet · 2016-04-06T20:25:25Z

Since it is directly related to rigid bodies, it should go on object/rigid_body_collision_groups.rs (btw. as a convention the file name usually matches the name of the structure it defines) and be re-exported by object/mod.rs.

Kerollmops · 2016-04-06T20:38:25Z

src/object/rigid_body_collision_groups.rs

    #[inline]
-    pub fn internal_collision_groups(&self) -> &CollisionGroups {
-        &self.collision_groups
+    pub fn as_collision_groups(&self) -> &CollisionGroups {


Do you want to keep this function ?

Kerollmops · 2016-04-06T21:13:00Z

src/object/rigid_body_collision_groups.rs

+// internal reserved static group id
+const STATIC_GROUP_ID: usize = 29;
+
+/// TODO: better explanations or redirect user to `ncollide` documentation ?!?


What do I do here for the doc ? ❓ ❔ ⁉️

For the moment I suggest you just remove the TODO line and keep your rest of your comment. I will have to reword it when sensors will be added anyway.

(Same remark about all the FIXME better documentation bellow.)

Kerollmops · 2016-04-06T21:15:55Z

What do I do about that too ?

the modification will have no effect if it is performed after the rigid body has been added to the scene). If the rigid body is static, the corresponding group should be set automatically after the user modified the group.

sebcrozet · 2016-04-06T21:21:12Z

Your current implementation already does that:

the modification will have no effect if it is performed after the rigid body has been added to the scene).

The rest is irrelevant as long as a full integration (allowing collision groups changes at any time) is not done.

Kerollmops · 2016-04-06T21:23:38Z

Ok, see you later for a better implementation!
Will you publish this version of collisionGroups integration for the moment ?

sebcrozet · 2016-04-06T21:28:02Z

src/world/world.rs

@@ -9,7 +9,7 @@ use ncollide::utils::data::hash_map::{HashMap, Entry};
 use ncollide::utils::data::hash::UintTWHash;
 use ncollide::broad_phase::{BroadPhase, DBVTBroadPhase};
 use ncollide::narrow_phase::ContactSignalHandler;
-use ncollide::world::{CollisionWorld, CollisionObject, CollisionGroups};
+use ncollide::world::{CollisionWorld, CollisionObject/*, CollisionGroups*/};


Could you just remove this comment? Then I should be ready for a merge.

sebcrozet · 2016-04-06T21:58:32Z

Thanks!

This will be published with a minor version number change, namely 0.2.1, as this does not includes breaking changes.

Kerollmops · 2016-04-06T22:02:07Z

Thanks to you !
I'm ok with that, have a good day/night 😄

Kerollmops · 2016-04-06T23:11:18Z

STOP ! there is a big problem !
all examples are fucked up !
Due to my bad implementation of collisionGroups !

Partially integrated collision groups into nphysics, need many fixes …

c5fec09

…and advices !

Kerollmops reviewed Apr 6, 2016
View reviewed changes

[FIX] examples workings

7238180

sebcrozet reviewed Apr 6, 2016
View reviewed changes

collisionGroups not at construction

772963c

sebcrozet reviewed Apr 6, 2016
View reviewed changes

no collision groups at creation

8c7a8e0

sebcrozet reviewed Apr 6, 2016
View reviewed changes

no collision groups in RigidBodyCollisionGroups::new()

8892c64

set_collision_groups implemented

b9ff1bc

rename get_internal_collision_groups to internal_collision_groups

bf7852b

implemented Deref Trait on RigidBodyCollisionGroups

8e4a233

Kerollmops reviewed Apr 6, 2016
View reviewed changes

Deref trait to .as_collision_groups()

e0c55bd

Kerollmops reviewed Apr 6, 2016
View reviewed changes

FIXME deleted

2e814dc

sebcrozet reviewed Apr 6, 2016
View reviewed changes

clean up comment

2503a7e

sebcrozet changed the title ~~Partially integrated collision groups into nphysics, need many fixes and advices !~~ Partially integrated collision groups into nphysics Apr 6, 2016

sebcrozet merged commit e94c2c9 into dimforge:master Apr 6, 2016

sebcrozet mentioned this pull request Apr 9, 2016

Examples execution errors on OSX #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially integrated collision groups into nphysics #58

Partially integrated collision groups into nphysics #58

Kerollmops commented Apr 6, 2016

Kerollmops Apr 6, 2016

sebcrozet Apr 6, 2016

sebcrozet Apr 6, 2016

sebcrozet Apr 6, 2016

Kerollmops Apr 6, 2016

sebcrozet Apr 6, 2016

Kerollmops Apr 6, 2016

sebcrozet Apr 6, 2016

Kerollmops Apr 6, 2016

sebcrozet Apr 6, 2016

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops Apr 6, 2016

Kerollmops Apr 6, 2016

sebcrozet Apr 6, 2016

sebcrozet Apr 6, 2016

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

sebcrozet Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Partially integrated collision groups into nphysics #58

Partially integrated collision groups into nphysics #58

Conversation

Kerollmops commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kerollmops commented Apr 6, 2016

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Choose a reason for hiding this comment

sebcrozet commented Apr 6, 2016

Kerollmops commented Apr 6, 2016

Kerollmops commented Apr 6, 2016