There’s a set of issues that together cause a data race during cooking, which leads to non-deterministic cook results.
- Consider a large map with world partitioning enabled. One of the streaming cells has several external actors of type AStreamingLandscapeProxy, each of which contains LandscapeNaniteComponent, each of which points to a unique mesh called ‘LandscapeNaniteMesh’. During cook, UWorldPartitionRuntimeLevelStreamingCell::OnPopulateGeneratedPackageForCook is called for this cell.
- While executing FWorldPartitionLevelHelper::LoadActors, all actors/components/meshes are loaded, and UStaticMesh::PostLoad start asynchronous FStaticMeshAsyncBuildTask tasks.
- After LoadActors returns, we call FWorldPartitionLevelHelper::MoveExternalActorsToLevel, which moves all actors and objects they point to from their /Temp/… packages to the streaming cell package. While moving second mesh (by calling Object->Rename(nullptr, LevelPackage, …) ), UObject::Rename detects name conflict with the first moved mesh, and renames the second mesh to ‘StaticMesh’.N
- Concurrently, FStaticMeshAsyncBuildTask calls UStaticMesh::ExecutePostLoadInternal -> CacheDerivedData -> FStaticMeshRenderData::Cache. At some point this function checks Owner->IsNaniteLandscape() - if it’s true, it returns early (keeping CardRepresentationData null), otherwise it creates an object. IsNaniteLandscape() is implemented by checking whether name starts with ‘LandscapeNaniteMesh’. This obviously races with the rename - depending on thread timings, the mesh will end up with or without this card data built. This in turn causes cook non-determinism (and maybe other issues…).
IsNaniteLandscape implementation seems to be extremely non robust.
Besides, is there a reason why UObject::Rename calls MakeUniqueObjectName passing default-constructed base name, rather than old name? Intuitively it would make more sense to generate new unique name by incrementing a number on the original name rather than on class name…
We have a fix for that we’re testing. In the meantime you can use this code instead of the old rename to avoid the issue.
else if (!Object->IsInOuter(Actor))
{
// Move objects in the destination level package if not outered to the Actor (and thus wouldn't follow)
// We handle name clashes in a custom way by increasing a suffix on the already existing basename instead
// of using the default behavior of having the class name be the base name, this is to maximally preserve
// information present in the object name in case somebody is using the name as significant information
if (Object->Rename(*Object->GetName(), LevelPackage, REN_NonTransactional | REN_DoNotDirty | REN_DontCreateRedirectors | REN_Test))
{
Object->Rename(*Object->GetName(), LevelPackage, REN_NonTransactional | REN_DoNotDirty | REN_DontCreateRedirectors);
}
else
{
Object->Rename(*MakeUniqueObjectName(LevelPackage, Object->GetClass(), *Object->GetName()).ToString(), LevelPackage, REN_NonTransactional | REN_DoNotDirty | REN_DontCreateRedirectors);
}
}
Without being too definitive, using the classname as a basename to handle conflicts, is most likely a perf/mem decision that maximizes reuse of the same strings in the FName table.
Hi - where do you suggest putting that, somewhere in FWorldPartitionLevelHelper::MoveExternalActorsToLevel?
For the record - for now, the local fix I’ve made is in UObject::Rename itself:
--- NewName = MakeUniqueObjectName(NewOuter ? NewOuter : GetOuter(), GetClass(), FName(), !!(Flags & REN_ForceGlobalUnique) ? EUniqueObjectNameOptions::GloballyUnique : EUniqueObjectNameOptions::None);
+++ NewName = MakeUniqueObjectName(NewOuter ? NewOuter : GetOuter(), GetClass(), NewOuter ? OldName : FName(), !!(Flags & REN_ForceGlobalUnique) ? EUniqueObjectNameOptions::GloballyUnique : EUniqueObjectNameOptions::None);
The rationale being - if UObject::Rename is called with InName == nullptr (implying we don’t actually need a rename), but rename needs to be done because NewOuter already has a child object with duplicate name, it’s always better to build a new name using use the old name and incrementing numbers, rather class name and incrementing numbers.
Yes, our intention was to put this logic in FWorldPartitionLevelHelper::MoveExternalActorsToLevel at the moment.