Assigning Flag in World Destructor

[AntonSynytsia]

Greeting everyone,

I'm attempting to use the world destructor callback to assign a validity flag to the world. I have a problem with NewtonWorldGetUserData. Here is the code snippet:

Code: Select all

typedef struct Vector3d
{
    dFloat x;
    dFloat y;
    dFloat z;
} Vector3d;

typedef struct WorldData
{
    bool valid;
    Vector3d gravity;
} WorldData;

const Vector3d DEFAULT_GRAVITY = {0.0f, 0.0f, -9.8f};

void world_destructor_callback(const NewtonWorld* const world)
{
    WorldData* data = (WorldData*)NewtonWorldGetUserData(world);
    data->valid = false; // This line causes the crash.
}

VALUE world_create(VALUE self)
{
    NewtonWorld* world = NewtonCreate();
    WorldData data;
    data.valid = true;
    data.gravity.x = DEFAULT_GRAVITY.x;
    data.gravity.y = DEFAULT_GRAVITY.y;
    data.gravity.z = DEFAULT_GRAVITY.z;
    NewtonWorldSetUserData(world, &data.valid);
    NewtonWorldSetDestructorCallback(world, world_destructor_callback);
    return LONG2NUM((LONG_PTR)world);
}

VALUE world_destroy(VALUE self, VALUE v_world)
{
    NewtonDestroy((NewtonWorld*)NUM2LONG(v_world));
    return Qnil;
}
 

Calling world = world_create works well, but after a moment, calling world_destroy(world) causes a crash. When I comment out data->valid = false; (in world_destructor_callback), the crash no longer occurs.

Is there an explanation/workaround to this? I don't have experience in C++, and don't know whether I do things correctly. Most of the time, I get things working by trial and error. Unfortunately, I don't always succeed. Meantime, I think it has to do with the fact that NewtonDestroy frees memory accumulated by Newton, which makes things go bad, but I still have no understanding of how to make it work

I would appreciate your help,
Anton

[JoeJ]

NewtonWorldSetUserData(world, &data.valid);

should be

NewtonWorldSetUserData(world, &data);

... if i'm not wrong, did not look closely.

[AntonSynytsia]

Hmm... I changed it, but the error still persists. Just to ensure that it happens in world destructor callback, I'll try calling data->value = false; before the world is destroyed. I'll post the results.

Edit: Okay, calling data->value = false; before and after the world destruction, doesn't crash, but if you call it inside the destructor (when the world is destroyed) it yields a crash.

[AntonSynytsia]

Speaking of body destructors, I also added a similar implementation to the bodies, but I don't get any crash when the body is destroyed.

[Julio Jerez]

if you destroy the world all bodies will be destroyed,
you should get a call to the destructor for each body.

[AntonSynytsia]

if you destroy the world all bodies will be destroyed,
you should get a call to the destructor for each body.

Yes, it works how you said. The prior post I posted was meant to inform that assigning value to user data didn't produce a crash in body destructor (i.e settings body user data valid parameter to false), but it did produce a crash in world destructor (i.e settings world user data valid parameter to false).

[JoeJ]

Code: Select all

VALUE world_create(VALUE self)
{
    NewtonWorld* world = NewtonCreate();
    WorldData data;
   NewtonWorldSetUserData(world, &data.valid);
}


Here the data object will be destroyed as soon as world_create returns.

It should be:

Code: Select all

VALUE world_create(VALUE self)
{
    NewtonWorld* world = NewtonCreate();
    WorldData *data = new WorldData ;
   NewtonWorldSetUserData(world, data);
}


Edit: For sure you need to store the pointer for the data object somewhere else.
Otherwise, after world has been destroyed, there is no more access to it
and you can not delete it on application exit.

[AntonSynytsia]

Thank you so much, JoeJ! I changed my code to what you suggested and it works quite well.

Here is my code for the body:

Code: Select all

VALUE body_is_valid(VALUE self, VALUE v_body)
{
    BodyData* data = (BodyData*)NewtonBodyGetUserData((NewtonBody*)NUM2LONG(v_body));
    return data->valid == true ? Qtrue : Qfalse;
}

void body_destructor_callback(const NewtonBody* const body)
{
    BodyData* data = (BodyData*)NewtonBodyGetUserData(body);
    data->valid = false;
}

VALUE body_create_dynamic(VALUE self, VALUE v_world, VALUE v_collision, VALUE v_matrix)
{
    dFloat matrix[16];
    for( int i = 0; i < 16; i++ )
        matrix[i] = (dFloat)NUM2DBL(rb_ary_entry(v_matrix, i));
    NewtonBody* body = NewtonCreateDynamicBody((NewtonWorld*)NUM2LONG(v_world), (NewtonCollision*)NUM2LONG(v_collision), matrix);
    BodyData data;
    data.valid = true;
    data.elasticity = DEFAULT_ELASTICITY;
    data.softness = DEFAULT_SOFTNESS;
    data.static_friction = DEFAULT_STATIC_FRICTION;
    data.dynamic_friction = DEFAULT_DYNAMIC_FRICTION;
    data.friction_enabled = DEFAULT_ENABLE_FRICTION;
    NewtonBodySetUserData(body, &data);
    NewtonBodySetForceAndTorqueCallback(body, force_and_torque_callback);
    NewtonBodySetDestructorCallback(body, body_destructor_callback);
    return LONG2NUM((LONG_PTR)body);
}


Do I have to apply your technique to the body user data, as well? I.E change BodyData data; to BodyData *data = new BodyData; (in body_create_dynamic function)

Edit: For sure you need to store the pointer for the data object somewhere else.
Otherwise, after world has been destroyed, there is no more access to it
and you can not delete it on application exit.

Does this mean I won't be able to get world data after the world destroyed? Does this mean I won't be able to access body data after the body is destroyed? I.E determine if the world/body is valid?

If that so, I will have to rethink my implementation a bit. I'm thinking of storing data in hash or something similar...

And if I just want to store data while the world is valid, I have to ensure that it is deleted when it is destroyed:

Code: Select all

void world_destructor_callback(const NewtonWorld* const world)
{
    WorldData* data = (WorldData*)NewtonWorldGetUserData(world);
    data->valid = false;
    delete data;
}

Is that correct?

[JoeJ]

And if I just want to store data while the world is valid, I have to ensure that it is deleted when it is destroyed:

This is correct, because the NewtonBody is the only thing that has the pointer to the data.
Note that the isValid flag does not make any sense if you delete the BodyData with the Body.
A flag like isActive would make sense, if you remove bodies from the world and add them later again.
(I do not know / understand the usecase you want)

Code: Select all

{
NewtonBody* body = NewtonCreateDynamicBody(...);
    BodyData data; // create object temporary on the stack
    data.valid = true;
    NewtonBodySetUserData(body, &data); // this is bad, because body lives longer than data
}
// closing the braces means destroying the data object.
// You need to allocate it pernamently on the heap with new or alloc
// too keep it valid after this point.


... Yes, you need to change this too.
It may seem to work, but after some time the memory will be overwritten with other stuff.
Then the bodies user data pointer points to memory with unknown content.
This can cause crashes, memory corruptions and hard to find bugs.

Does this mean I won't be able to get world data after the world destroyed? I.E determine if the world/body is valid?

Yes, the isValid makes no sense here. World is the deleted, thus no pointer to WorldData, and WorldData is deleted too.

An alternative would be:
Keep pointers not only inside newton objects, but also store them in your app.
In the data also store a pointer to its newton object.
Do not delete data on newton callback, but set its newton object pointer to zero.
While processing data pointers from app, always check newton pointer (zero = not valid, else valid)
Delete data objects on application exit, or when you don't need them anymore (if its newton objetct is still alive, zero it's user data pointer)

Maybe that's more what you want.

[AntonSynytsia]

Note that the isValid flag does not make any sense if you delete the BodyData with the Body.

Opps, I didn't acknowledge that

A flag like isActive would make sense, if you remove bodies from the world and add them later again.

How is it possible to remove body from world, and then re-add it? I mean it's possible to create body or destroy body, but removing body must be something new. Does what you say connect to the NewtonBodyEnableSimulation/NewtonBodyDisableSimulation functions?

An alternative would be:
Keep pointers not only inside newton objects, but also store them in your app.
In the data also store a pointer to its newton object.
Do not delete data on newton callback, but set its newton object pointer to zero.
While processing data pointers from app, always check newton pointer (zero = not valid, else valid)
Delete data objects on application exit, or when you don't need them anymore (if its newton objetct is still alive, zero it's user data pointer)

I had to reread this a couple of times to understand. Seems a good alternative.

Thank you for clarifying

[AntonSynytsia]

I actually mislead myself on how I wanted it to determine the validity of body, but here is the new technique I use:

Every time the body is created, the body pointer (in long form) is added to a hash (std::map). The body pointer in long form acts as hash key, and a true acts as key's value. Whenever a body is destroyed the body pointer (key) is deleted from hash.

To determine if body poitner is valid, you simply attempt to link it in hash. If it fails to link, or the return value of the linked object is false, then the body is invalid; otherwise, it's valid.

The data itself associated with the body is created when the body is created and destroyed when the body is destroyed. So, accessing body data after the body is destroyed will not work, neither it would be necessary. The validity hash will be there to determine if the body is valid and simply for security reasons, since the user can pass any sort of body address they might wish. If the user wants to access particular body data, they will have to use a function, like body_get_data(body). The function itself will check whether the specified body exists before attempting to return data, simply by linking it in hash. If the specified body pointer is not linked, the function will return NULL or raise an argument error saying, "The specified body does not exist!" If the body does exist, then the function will proceed to returning body data.

Same security technique is applied to all functions to avoid crashes or unexpected results. Who knows what the user might pass as a parameter? Validity checks are essential to make the API crash-proof.

Using hash (std::map) is a huge advantage over arrays or other iterating forms as they don't require iterating to determine if something is there. Along with that, hashes have no limit in size, which is a big plus over arrays. For example, if you wanted to determine if the body is valid, you would have had to iterate through an array contain all valid bodies to check if the specified body is valid (or exists), but with hashes the key (body address) is directly linked with it's value, so iterating through all keys won't be required.

Here is a little code snippet if any one is interested.

Code: Select all

std::map<long, bool> valid_bodies;

void body_destructor_callback(const NewtonBody* const body)
{
    if( valid_bodies.find((long)body) != valid_bodies.end() )
        valid_bodies.erase((long)body);
    BodyData* body_data = (BodyData*)NewtonBodyGetUserData(body);
    delete body_data;
}

VALUE body_is_valid(VALUE self, VALUE v_body)
{
    return (valid_bodies.find(NUM2LONG(v_body)) == valid_bodies.end()) ? Qfalse : Qtrue;
}

VALUE body_create_dynamic(VALUE self, VALUE v_world, VALUE v_collision, VALUE v_matrix)
{
    dFloat matrix[16];
    for( int i = 0; i < 16; i++ )
        matrix[i] = (dFloat)NUM2DBL(rb_ary_entry(v_matrix, i));
    matrix[12] *= INCH_TO_METER;
    matrix[13] *= INCH_TO_METER;
    matrix[14] *= INCH_TO_METER;
    NewtonBody* body = NewtonCreateDynamicBody((NewtonWorld*)NUM2LONG(v_world), (NewtonCollision*)NUM2LONG(v_collision), matrix);
    valid_bodies[(long)body] = true;
    BodyData* data = new BodyData;
    data->elasticity = DEFAULT_ELASTICITY;
    data->softness = DEFAULT_SOFTNESS;
    data->static_friction = DEFAULT_STATIC_FRICTION;
    data->dynamic_friction = DEFAULT_DYNAMIC_FRICTION;
    data->friction_enabled = DEFAULT_ENABLE_FRICTION;
    NewtonBodySetUserData(body, data);
    NewtonBodySetForceAndTorqueCallback(body, force_and_torque_callback);
    NewtonBodySetDestructorCallback(body, body_destructor_callback);
    return LONG2NUM((LONG_PTR)body);
}

// @return [Bool] success
VALUE body_destroy(VALUE self, VALUE v_body)
{
    if( valid_bodies.find(NUM2LONG(v_body)) == valid_bodies.end() )
        return Qfalse;
    NewtonDestroyBody((NewtonBody*)NUM2LONG(v_body));
    return Qtrue;
}

VALUE body_get_elasticity(VALUE self, VALUE v_body)
{
    if( valid_bodies.find(NUM2LONG(v_body)) == valid_bodies.end() )
        return Qnil;
    BodyData* data = (BodyData*)NewtonBodyGetUserData((NewtonBody*)NUM2LONG(v_body));
    return DBL2NUM(data->elasticity);
}

VALUE body_set_elasticity(VALUE self, VALUE v_body, VALUE v_elasticity)
{
    if( valid_bodies.find(NUM2LONG(v_body)) == valid_bodies.end() )
        return Qfalse;
    BodyData* data = (BodyData*)NewtonBodyGetUserData((NewtonBody*)NUM2LONG(v_body));
    data->elasticity = c_clamp((dFloat)NUM2DBL(v_elasticity), 0.01f, 2.00f);
    return Qtrue;
}