Datablock editor crash - THREED-3163

[DmitryGarageGames]

Issues: When a vehicle using a datablock is placed in a scene, and the datablock editor is used to replace the vehicle's shapeFile with one without a collision mesh, the engine crashes. Steps to repeat: Have two vehicle shapes, one with collision and one without. Set the datablock to use the one with. Run the game and add a vehicle with that datablock to the scene. Open the datablock editor and change the shapeFile for that datablock to the other shape.

[DmitryGarageGames]

Fixed at least two crashes on changing shapes caused by non singular matrices. Going to test a bit more.

[crabmusket]

Okay, I've had no trouble reproducing this, but I'm not sure it's got anything to do with matrices. I duplicated the Cheetah_Body shape to Cheetah_Body_Fine and copy/pasted the cs file, renaming the shape file and constructor names as appropriate.

I did the same process with a new Cheetah_Body_NoCol file, and edited the DAE file with a text editor to remove all the COLBOX and COLMESH geometry and nodes.

In the datablock editor, switching from Cheetah_Body to Cheetah_Body_Fine caused a crash unless the Fine mesh had already been imported, in which case it worked fine. Switching from Fine to NoCol caused a crash always. Both crashes happened here:

bool TSShapeInstance::buildPolyList(AbstractPolyList * polyList, S32 dl)
{
   ...
   // get subshape and object detail
   const TSDetail * detail = &mShape->details[dl];

However, if I edited the Cheetah datablock to use NoCol (which shows up in the shape editor as having 0 collision meshes), then the databloc fails to register because of the lack of collision geometry.

[LuisAntonRebollo]

Okay, I've had no trouble reproducing this, but I'm not sure it's got anything to do with matrices. I duplicated the Cheetah_Body shape to Cheetah_Body_Fine and copy/pasted the cs file, renaming the shape file and constructor names as appropriate.

I have reproduced using this.

739 fixes the crash and set vehicle after change in datablock.

Invalid matrix are caused becouse we don't update MatrixF* ShapeBaseConvex::nodeTransform with correct data from new TSShapeInstance. See void ShapeBaseConvex::findNodeTransform()

Not tested with No Collision

[LuisAntonRebollo]

The problem are a bit more complicated :(

Datablockcalculate some data on preload , we need to update required data when a Datablock are modified.

For example:

bool ShapeBaseData::preload(bool server, String &errorStr)
{
...
      // Resolve details and camera node indexes.
      static const String sCollisionStr( "collision-" );

      for (i = 0; i < mShape->details.size(); i++)
      {
         const String &name = mShape->names[mShape->details[i].nameIndex];

         if (name.compare( sCollisionStr, sCollisionStr.length(), String::NoCase ) == 0)
         {
            collisionDetails.push_back(i); // if we change shape... we need to update this
            collisionBounds.increment(); // if we change shape... we need to update this
...
}

I will dedicate more time tomorrow.

[crabmusket]

Yeah, it looked to me like the datablock was trying to access old nodes in the new shape (which didn't have them). So we probably need to run through preload again when the shape field changes.

And then, do we have to re-sync the datablock to the client? Not sure how the editor handles this.

[LuisAntonRebollo]

The bad news are preload are not coded for call on existing `Datablock, cause some problems. I'm trying to fix this.

[crabmusket]

Is this issue not the same for any other datablock that uses a shape? Why is Vehicle special?

[LuisAntonRebollo]

Vehicle::mConvex are the cause of problems, not used on other ShapeBaseData.

Anyone can test? Remember the correct shape are DAE files, cached.dts are only a part of DAE shape.

[crabmusket]

I'll test your PR, but should it deal with the issue of changing the shape entirely so the stuff that was setup in preload is no longer valid?

[crabmusket]

I suspect the way to deal with this is to break all the shape-specific setup from preload into a separate function, and call this function every time mShapeFile changes using protected fields. Though I'm not sure what other issues that will bring up :/. I'm going to give it a go.

EDIT. I predict this will have silly results for client/server stuff... though you shouldn't be editing datablocks while clients are connected.

EDIT: I tried that and didn't get too far with it. I also implemented a Signal that fires when the datablock is edited, so that all Vehicles know to update their collision information. However there are still errors with other shape classes caused by what I did to ShapeBase::preload... need to go back and look at that. I'm really tempted to say this just won't fix without a large amount of restructuring, or introducing hacks to handle this specific case of editor-modified data. Maybe the quick/honest solution is to treat datablock modifications as entirely new datablocks, as far as all objects are concerned, rather than attempting to patch specific problems that occur when specific fields are edited. Just act as if the entire datablock has been swapped out.

EDIT: Which still won't handle the updates that need to happen within the datablock itself. Right. Here comes the hard work.

[crabmusket]

The fundamental problem here is that datablocks were never designed to be dynamic. There's a ton of stuff built into the engine that assumes they will be static once they're created. We have two routes:

1. Accept the current behavior, and that the datablock editor will be a bit broken. TGE didn't have a datablock editor - it was introduced in T3D but does not seem to be properly integrated.
2. Fix datablocks, allowing full dynamic behavior. This will require touching most classes below GameBase, and probably everything below ShapeBase. #779 attempts to do this for a limited subset of datablock behavior (i.e. stuff involving the ShapeBase shape file).
3. Fix the datablock editor. This is my preferred approach. I think this will involve:
   • Some fields being immutable. For example, a vehicle's shape file. If you want to create a vehicle with a different shape, you'll need to create a new datablock and switch to it.
   • Reintroduce the 'builder' for objects that TGE's editor had, which would require you provide certain fields before creating an object. This would also solve #696 and improve usability.

Even with those changes to the datablock editor, there may still be problems with some classes' onNewDataBlock functions. But I still think 3 is the best option, and I don't think we can fit it into 3.6.

[crabmusket]

Moving out of 3.6 because there's a lot more behind this than it seems. Will debate further.