NewtonBodyEnableSimulation memory corruption

Report any bugs here and we'll post fixes

Moderators: Sascha Willems, Thomas

NewtonBodyEnableSimulation memory corruption

Postby arkeon » Thu Sep 18, 2014 7:32 am

Hello,

calling NewtonBodyEnableSimulation on an already enabled body cause a memory corruption.
a body member with the enable state could permit to add a test on enable / disable to prevent multiple enable / disable call.
arkeon
 
Posts: 261
Joined: Sat Sep 13, 2014 5:25 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby arkeon » Thu Sep 18, 2014 8:12 am

My bad, I was not aware that calling NewtonBodyDisableSimulation should destroy the associated joints :/
arkeon
 
Posts: 261
Joined: Sat Sep 13, 2014 5:25 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby Julio Jerez » Thu Sep 18, 2014 8:18 am

I added the function
int NewtonBodyGetEnableDisableSimulationState(const NewtonBody* const body);

however the function
NewtonBodyEnableSimulation(const NewtonBody* const body)

should nor do cause any memory corruption, this is ho wit is implemented
Code: Select all
void dgWorld::BodyEnableSimulation (dgBody* const body)
{
   if (!body->m_masterNode) {
      m_disableBodies.Remove(body);
      dgBodyMasterList::AddBody(body);
      body->SetMassMatrix(body->m_mass.m_w, body->m_mass.m_x, body->m_mass.m_y, body->m_mass.m_z);
      m_broadPhase->Add (body);
      dgAssert (body->m_masterNode);
   }
}


when a body does not have a master node, is can no be remove more than once.
no matter ho many time you call the functions.
Is that a runtime crash, a char on exit or a crash when calling the function more than once?
Julio Jerez
Moderator
Moderator
 
Posts: 11045
Joined: Sun Sep 14, 2003 2:18 pm
Location: Los Angeles

Re: NewtonBodyEnableSimulation memory corruption

Postby Julio Jerez » Thu Sep 18, 2014 8:24 am

yes it will destroy the joints,
may it should not do that however that's a big change in many internal part of the engine.

Plus it Is no clear what the behavior should be.
Consider two ball connect by a ball and socket joint, if you disable one body,
then the other ball start to move.

what should happen when the second body is enabled again?
the two balls will be violating the constraint by a very large value.
Julio Jerez
Moderator
Moderator
 
Posts: 11045
Joined: Sun Sep 14, 2003 2:18 pm
Location: Los Angeles

Re: NewtonBodyEnableSimulation memory corruption

Postby arkeon » Thu Sep 18, 2014 8:39 am

my memory corruption were because of a bad implementation of the joint destroy callback.

Yes you're right this is some weird cases to manage.
Maybe you could simply disable/enable simulation on bodies shared on the joint.

so when you enable / disable a body, you look if there is joints and then do the same for all shared bodies on joints.

what about the case you create a joint after you disabled a body ?
just do the same if one of the bodies is disable you disable the second one.

what do you think about this ? it could be very usefull I think.

For example with leap motion I have several bodies with joints. and hide or show the hand when detected. with is working method I could disable the bodies easily on show / hide. by just disable the main hand body.
arkeon
 
Posts: 261
Joined: Sat Sep 13, 2014 5:25 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby Julio Jerez » Thu Sep 18, 2014 8:54 am

yes maybe disable should be renamed to disable island, that way is will no destroy joints.
or no remaning, just disable de constraction
I like that idea better.
Julio Jerez
Moderator
Moderator
 
Posts: 11045
Joined: Sun Sep 14, 2003 2:18 pm
Location: Los Angeles

Re: NewtonBodyEnableSimulation memory corruption

Postby arkeon » Fri Sep 19, 2014 6:10 am

yes maybe a NewtonBodyIslandEnableSimulation
arkeon
 
Posts: 261
Joined: Sat Sep 13, 2014 5:25 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby manny » Sat Sep 20, 2014 6:00 am

I added the function
int NewtonBodyGetEnableDisableSimulationState(const NewtonBody* const body);

That is an unfortunate name :). I commited a change that improves the API

Code: Select all
// Name: NewtonBodyGetSimulationState
// Gets the current simulation state of the specified body.
//
// Parameters:
// *const NewtonBody* *bodyPtr - pointer to the body to be inspected.
//
// Return: the current simulation state 0: disabled 1: active.
//
// See also: NewtonBodySetSimulationState
int NewtonBodyGetSimulationState(const NewtonBody* const bodyPtr)
{
   TRACE_FUNCTION(__FUNCTION__);
   dgBody* const body = (dgBody *)bodyPtr;
   dgWorld* const world = body->GetWorld();
   return world->GetBodyEnableDisableSimulationState(body) ? 1 : 0;
}

// Name: NewtonBodySetSimulationState
// Sets the current simulation state of the specified body.
//
// Parameters:
// *const NewtonBody* *bodyPtr - pointer to the body to be changed.
// *const int* state - the new similation state 0: disabled 1: active
//
// Return: Nothing.
//
// See also: NewtonBodyGetSimulationState
void NewtonBodySetSimulationState(const NewtonBody* const bodyPtr, const int state)
{
   TRACE_FUNCTION(__FUNCTION__);
   dgBody* const body = (dgBody *)bodyPtr;
   dgWorld* const world = body->GetWorld();

   if (state)
      world->BodyEnableSimulation(body);
   else
      world->BodyDisableSimulation(body);
}


There is now a single method to enable/disable the body, plus a single method the "getter" is properly named. The single method is great if bodys are disabled in a batch, a for loop or something. Reduces code bloat. Plus that EnableDisable state is just a double wording and that is something that should be avoided.

I have also marked the methods deprecated:
Code: Select all
   NEWTON_DEPRECATED_API void NewtonBodyEnableSimulation(const NewtonBody* const body);
   NEWTON_DEPRECATED_API void NewtonBodyDisableSimulation(const NewtonBody* const body);


so we can tighten up the api in a future release.
http://www.instaLOD.io - InstaLOD - State of the art 3D optimization
manny
Site Admin
Site Admin
 
Posts: 131
Joined: Tue Feb 11, 2014 6:49 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby manny » Sat Sep 20, 2014 6:06 am

also updated sandbox code.
http://www.instaLOD.io - InstaLOD - State of the art 3D optimization
manny
Site Admin
Site Admin
 
Posts: 131
Joined: Tue Feb 11, 2014 6:49 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby d.l.i.w » Sat Sep 20, 2014 7:36 am

I still would prefer to use NewtonBodyEnableSimulation / NewtonBodyDisableSimulation instead of the new one, as these are more clear what they do and easier to read.

The new one reads like this:
NewtonBodySetSimulationState(a_body, true)

Now - what does this mean simulation state = true?
Of course one could add constants / enums for the state(s), but this means more typing...

Btw. it's bad coding style to leave out the brackets on if - else.
d.l.i.w
 
Posts: 81
Joined: Mon Sep 26, 2011 4:35 am

Re: NewtonBodyEnableSimulation memory corruption

Postby manny » Sat Sep 20, 2014 9:07 am

d.l.i.w wrote:Btw. it's bad coding style to leave out the brackets on if - else.

No, it's not bad coding style. That is a matter of the coding style/style guide that is used. This not something that can be boiled down to a good or bad. In our company we use no brackets for one liners (and no nested if/esle to avoid hard to find dangling issues). This is done to avoid code bloat.

however, this project is using the if (..) {\n\n\t} style and I applied our company's style by mistake. A fix has been commited. and that is the issue here. not a style disucssion.

Now - what does this mean simulation state = true?
Of course one could add constants / enums for the state(s), but this means more typing...

well, where did you find that bool? it's an int, and in the API it's defined to 0 disabled and 1 enabled. It's allows for more states that just 0/1 as other physics libraries do. Right now what the code does (with the if/else) does not matter, it does what the API header doc says, and that is what's important here.
...and that follows the coding convetion of the rest of the api.
http://www.instaLOD.io - InstaLOD - State of the art 3D optimization
manny
Site Admin
Site Admin
 
Posts: 131
Joined: Tue Feb 11, 2014 6:49 pm

Re: NewtonBodyEnableSimulation memory corruption

Postby d.l.i.w » Sat Sep 20, 2014 10:48 am

Sorry, if it sounded too harsh, I wasn't intended to be...

This not something that can be boiled down to a good or bad.

Maybe you are right, I don't code in C / C++ most of the time, so I may have an different point of view.
If it's done correctly, I don't mind when brackets are missing, but unfortunately often enough you get to see big blocks of nested if - else and similar constructs with as few brackets as possible.

Some time ago I had to maintain and extend ("a bit") an existing code base with such bad quality (it was PHP actually). It took me almost a year to get any sense out of this big mess and the "small extensions" needed ended up in almost a whole rewrite of the code - just for you to know, why I reacted such "allergic"... :?

well, where did you find that bool? it's an int

That's not that clear with C...

...and that follows the coding convetion of the rest of the api.

Hm, you are right, so sorry for my complaint...

Maybe I did too much header conversion lately, being too much frustrated of deciding if an int is actually a bool or not :mrgreen:

It's allows for more states that just 0/1 as other physics libraries do

Although I wouldn't orientate on other libraries too much, that's a valid point, too...
d.l.i.w
 
Posts: 81
Joined: Mon Sep 26, 2011 4:35 am

Re: NewtonBodyEnableSimulation memory corruption

Postby manny » Sun Sep 21, 2014 5:55 am

If it's done correctly, I don't mind when brackets are missing, but unfortunately often enough you get to see big blocks of nested if - else and similar constructs with as few brackets as possible.

yeah, that was what i was referring to as the dangling issue: http://de.wikipedia.org/wiki/Dangling_else

d.l.i.w wrote:Some time ago I had to maintain and extend ("a bit") an existing code base with such bad quality (it was PHP actually). It took me almost a year to get any sense out of this big mess and the "small extensions" needed ended up in almost a whole rewrite of the code - just for you to know, why I reacted such "allergic"... :?
yeah, I can understand that. I am also a bit of a code jäger in our company, I always review code and if it does not match our style somebody gets blamed for it.

That's not that clear with C...
don't want to nitpick, but it's part of C99 <stdbool.h> :D and even if a project is not using C++ or C99 it's nearly always clearly typedef'd so the size is uniform in the project, it's either typedef uint8_t bool or typedef int32_t bool - but I'm sure you get the point: a bool type is clearly defined most of the time. And if the coder didn't know typedef back then, he probably did a #define bool int :)

Maybe I did too much header conversion lately, being too much frustrated of deciding if an int is actually a bool or not :mrgreen:

heh, yeah, then that codebase is terrible :(

Although I wouldn't orientate on other libraries too much, that's a valid point, too...

that is true, and newton certainly stands besides and above other physics libraries. but sometimes it's not necessary to do something different and probably worse.
it's good for the users if your API can expand without having to add a new method. it even helps to avoid confusion. an exmaple for that is. you have these methods:

NewtonBodyEnableSimulation -> sets state active internally
NewtonBodyDisableSimulation -> sets state disabled internally
NewtonBodyEnableLowPowerSimulation -> sets state lowpower internally

so the user creates a body that doesn't require lots of simulation and gets away with interpolation, so he sets NewtonBodyEnableLowPowerSimulation
after a while the body is culled in some fashion and doesn't need simulation anymore so NewtonBodyDisableSimulation
a few frames later the body gets back into the view and the user calls NewtonBodyEnableSimulation
unaware that all these methods modify the same field, his body is now running at regular simulation intervals.

IMHO the API should decide between interfaces to propertys and interfaces to methods.
a field/property interface should only have a single method changing the value to avoid confusion and a clear naming type.
For numbers/scalars and other types it is defined as:
getter: void <APINAME>Set<FIELDNAME>(const <FIELDTYPE> value)
setter: <FIELDTYPE> <APINAME>Get<FIELDNAME>()
It can be even a little bit more explicit for bools or 0/1 types so the user understands just from reading the API function names it's a boolean type property.

getter:
bool <APINAME>Is<FIELDNAME>Enabled()
void <APINAME>Set<FIELDNAME>Enabled(const bool isEnabled)
bool <APINAME>Is<FIELDNAME>Active()
void <APINAME>Set<FIELDNAME>Active(const bool isEnabled)

these patterns world incredibly well in 95% of the time. and if you review the newton API you can see that it also follows these rules most of the time:

Code: Select all
   NEWTON_API int NewtonJointIsActive (const NewtonJoint* const joint);

   NEWTON_API void NewtonJointGetInfo  (const NewtonJoint* const joint, NewtonJointRecord* const info);
   NEWTON_API int NewtonJointGetCollisionState (const NewtonJoint* const joint);
   NEWTON_API void NewtonJointSetCollisionState (const NewtonJoint* const joint, int state);
   NEWTON_API void NewtonUpVectorGetPin (const NewtonJoint* const upVector, dFloat *pin);
   NEWTON_API void NewtonUpVectorSetPin (const NewtonJoint* const upVector, const dFloat *pin);

   NEWTON_API int NewtonBodyGetSleepState (const NewtonBody* const body);
   NEWTON_API void NewtonBodySetSleepState (const NewtonBody* const body, int state);

   NEWTON_API int NewtonBodyGetAutoSleep (const NewtonBody* const body);
   NEWTON_API void NewtonBodySetAutoSleep (const NewtonBody* const body, int state);

   NEWTON_API int NewtonBodyGetFreezeState(const NewtonBody* const body);
   NEWTON_API void NewtonBodySetFreezeState (const NewtonBody* const body, int state);




TL;DR; dangling else: http://de.wikipedia.org/wiki/Dangling_else + bool is part of c99 + the Enable/DIsable methods were out of the line in terms of styleguide anyways...
http://www.instaLOD.io - InstaLOD - State of the art 3D optimization
manny
Site Admin
Site Admin
 
Posts: 131
Joined: Tue Feb 11, 2014 6:49 pm


Return to Bugs and Fixes

Who is online

Users browsing this forum: No registered users and 1 guest