supertuxkart-sourceforge-migration / stk-migration-test2

0 stars 0 forks source link

Nolok wheel incorrect after battle mode #458

Open supertuxkart-sourceforge-migration opened 10 years ago

supertuxkart-sourceforge-migration commented 10 years ago

Author: hikerstk

Use nolok in battle mode and loses all wheels. From then on the rear left wheel is incorrect (too far to the inside).

Migrated-From: https://sourceforge.net/apps/trac/supertuxkart/ticket/458

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria With Pidgin, both rear wheels are wrong.

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Current working theory : irrlicht is confused by TrackObject reloading the wheel meshes as animated meshes

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria After more checks, that's not it. No idea what to check next

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Ahah, I think I got it. PhysicalObject does this :

    // 2. Adjust the mesh so that its center is where it is in bullet
    // --------------------------------------------------------------
    // This means that the graphical and physical position are identical
    // which simplifies drawing later on.
    scene::IMeshManipulator *mesh_manipulator = 
        irr_driver->getSceneManager()->getMeshManipulator();
    core::matrix4 transform(core::matrix4::EM4CONST_IDENTITY);  // 
    transform.setTranslation(offset_from_center.toIrrVector());
    mesh_manipulator->transform(mesh, transform);
supertuxkart-sourceforge-migration commented 10 years ago

Author: hikerstk Additional explanation: when the wheel of the karts are converted to physical objects, their mesh is modified (in the above mentioned code). So later when the wheels are re-attached to the kart, they are incorrect.

Solutions: 1) Use the m_centerOfMassOffset of the btDefaultMotionState objects used instead of modifying the mesh 2) create a copy of the mesh in PhysicalObject (difficult since I am not sure if there is a function that can clone an arbitrary (non-static) mesh 3) Translate the mesh back once the object is removed.

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Mostly fixed in r10044 by duplicating the wheel mesh before adding it to the track.

Main problem at is this piece of code in the destructor of TrackObject :

        m_mesh->drop();
        if(m_mesh->getReferenceCount()==1)
            irr_driver->removeMeshFromCache(m_mesh);

Joerg, can you remember what this code is for?? ->drop can result in the object being deleted, which makes the ->getReferenceCount call crash. And also, if the reference count is still 1 after dropping, I don't see why we would want to remove the mesh from the cache since it's still used.

Also, Joerg, if you could check what I did in terms of memory management :) I think I'm getting the hang of irrlicht ref counting but some code review won't hurt

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria r10045 fixes an issue with textures in previous commit and also in there I tried to correct the desctructor of TrackObject, Joerg please confirm that what I did was what you intended when you wrote that dtor

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Hmm I found this comment from you :

// If the ref count is 1, the only reference is in
// irrlicht's mesh cache, from which the mesh can
// then be deleted.
// Note that this test is necessary, since some meshes
// are also used in attachment_manager!!!

so probably my fix is not correct :( can you please check why I got a crash then? looks like when you duplicate meshes they don't get added to the mesh cache

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Ok think it's all fixed in r10046.

Joerg, review still welcome :)

Also the code I wrote to free the mesh is starting to be really silly. I'm pondering if we should perhaps write some kind of "mesh holder" smart pointer that would take care of grabbing the mesh, the textures, etc. and then dropping the mesh, the textures, the cache, etc. as we're doing that management manually all over the place and it's starting to get really silly (and error-prone).

supertuxkart-sourceforge-migration commented 10 years ago

Author: auria Arrg sorry for the huge number of commits I did something really silly in my previous commit, fixed in r10047. -.-

supertuxkart-sourceforge-migration commented 10 years ago

Author: hikerstk I've removed patched r10044 to r10047 in r10051, and instead applied r10051, which provides a much easier fix: instead of modifying the meshes to align graphical mesh to bullet collision body the graphical position is offset appropriately when drawing.