Problematic code in PopcornFXRendererLoader::_AddGeometryToLoad

[mbalfour-amzn]

While loading the MultiplayerSample level in the Editor, I noticed the following errors:

[Error] (PopcornFX) - [O3DE] Failed to get model asset id with path 'PopcornFX/Meshes/JumpPad/Cylinder.fbx'
[Error] (PopcornFX) - [O3DE] Failed to get model asset id with path 'PopcornFX/Meshes/MalfunctionningShieldGenerator/SM_GeoSphere.fbx'

These are caused by PopcornFXRendererLoader::_AddGeometryToLoad(). Specifically, it appears that this function is taking in source asset paths (i.e. 'PopcornFX/Meshes/JumpPad/Cylinder.fbx'), which is the incorrect thing to pass in to GetAssetIdByPath, which is expecting a product asset path (ex: 'popcornfx/meshes/jumppad/cylinder.fbx.azmodel').

In general, source asset paths shouldn't be relied upon for runtime loading, since the runtime asset catalog has no knowledge of source assets or what they map to. Ideally, the runtime should use AssetId values, but at a minimum it needs to be a product asset path.

However, if that bug is fixed, the code that occurs directly after it is also problematic:

    AZ::Data::Asset<AZ::RPI::ModelAsset>    modelAsset = AZ::Data::AssetManager::Instance().GetAsset<AZ::RPI::ModelAsset>(assetId, AZ::Data::AssetLoadBehavior::QueueLoad);
    modelAsset.QueueLoad();
    if (modelAsset.IsReady())
        OnAssetReady(modelAsset);

Because modelAsset is local to this method, if it is the only reference to this asset, the asset will immediately get unloaded at the end of the method. The asset reference itself needs to be stored somewhere, perhaps in that _InsertAssetIFN asset dependency cache.

Interestingly enough, all of the mesh-based particles still seem to be working fine with these errors. The errors don't seem to be causing critical failures, they're just not ideal.

[ValPKFX]

Hi, thank you for the report. This was indeed a regression, this is now fixed on the development branch with 1fe9ca3 and will be in the next releases. Concerning the modelAsset, it doesn't seems to cause issues. Either the modelAsset is ready and we call the OnAssetReady directly which keep the AZ::Data::Instance in the geometry cache. Either it is not yet ready and the QueueLoad seems to correctly call the OnAssetReady when ready even if the local modelAsset variable is destroyed. I will close this issue but do not hesitate to tell us if you think the current way it works is really problematic.

[mbalfour-amzn]

If there's any other part of code that happens to have a reference to that Asset at that point in time, then everything should work fine exactly as you describe, because the asset will continue to exist for those other references.

However, if this is the only reference to the asset, then I believe the load will get cancelled as soon as the asset reference is destroyed. Basically, I suspect the code is working right now either out of luck, timing, or because somewhere else in the PopcornFX code it has a second reference to this same asset.

I would suggest changing the code to more explicitly hold on to the asset reference so that you aren't creating (and maintaining) implicit dependencies to the logic and data that exists in other parts of your code. It will help guarantee that this part of the code works reliably and won't have any race conditions.

[ValPKFX]

Sorry for the late reply, I was out of office. Indeed we already had the dependencies->m_AssetRef for other asset types used to keep the ref. I did the same thing for ModelAsset. Thank you again.