Potential Fix for crashes during foliage painting

Development World Creation
, ,
1

We have been seeing some difficult to reproduce corruption in AInstancedFoliageActors which results in our maps crashing on load. After some investigation we think we’ve tracked down the path which causes the foliage actors to contain invalid data:

There seems to be a bug in the logic in AISMPartitionActor::RemoveISMInstances. This code specifically

		if (ComponentData.ClientInstances.IsValidIndex(Handle.Index))
		{
			FISMClientData& ClientData = ComponentData.ClientInstances[Handle.Index];
			for (int32 InstanceIndex = ClientData.Instances.Num() - 1; InstanceIndex >= 0; --InstanceIndex)
			{
				RemoveISMInstancesInternal(ComponentData, ClientData, InstanceIndex);
			}
			ClientData.Instances.Empty(); // Free the memory
		}


		bool bIsEmpty = DestroyComponentIfEmpty(Descriptors[DescriptorIndex], ComponentData);
		// If Component is empty and its the last descriptor in the list we can remove it without breaking indices
		if (bIsEmpty && DescriptorIndex == Descriptors.Num() - 1)
		{
			Descriptors.RemoveAt(DescriptorIndex);
			DescriptorComponents.RemoveAt(DescriptorIndex);
		}

ComponentData.ClientInstances can contain empty entries since it’s resized to the highwater mark of FISMClientHandles so IsValidIndex doesn’t actually mean that that component actually has data from the client which is being removed. This can result in DestroyComponentIfEmpty being called too soon which invalidates the indices of the other clients

Changing the code like this seems to avoid this case and solve the issue since it avoids calling DestroyComponentIfEmpty on descriptors which are from other clients so the indicies stay valid for the lifetime of the client.

		if (ComponentData.ClientInstances.IsValidIndex(Handle.Index) && !ComponentData.ClientInstances[Handle.Index].Instances.IsEmpty())
		{
			FISMClientData& ClientData = ComponentData.ClientInstances[Handle.Index];
			for (int32 InstanceIndex = ClientData.Instances.Num() - 1; InstanceIndex >= 0; --InstanceIndex)
			{
				RemoveISMInstancesInternal(ComponentData, ClientData, InstanceIndex);
			}
			ClientData.Instances.Empty(); // Free the memory

			bool bIsEmpty = DestroyComponentIfEmpty(Descriptors[DescriptorIndex], ComponentData);
			// If Component is empty and its the last descriptor in the list we can remove it without breaking indices
			if (bIsEmpty && DescriptorIndex == Descriptors.Num() - 1)
			{
				Descriptors.RemoveAt(DescriptorIndex);
				DescriptorComponents.RemoveAt(DescriptorIndex);
			}
		}

There is also another crash with this foliage path due to logic in FFoliageISMActor::Reapply. Calling Initialize if Info->Instances is empty results in AISMPartitionActor Descriptors being created with null Components since the components are added when the first instance is created, which causes other code to crash. The class already calls Initialize() if required in FFoliageISMActor::PreAddInstances so this can be avoided by early returning from Reapply if there are no instances

void FFoliageISMActor::Reapply(const UFoliageType* FoliageType)
{
//...existing code
    		Uninitialize();
	}
  
      	if (Info->Instances.IsEmpty()) //New Check
      	{
      		return;
      	}
      	Initialize(FoliageType);
      	check(IsInitialized());
      	
      	BeginUpdate();
      	for (const FFoliageInstance& Instance : Info->Instances)
      	{
      		AddInstance(Instance);
      	}
      	EndUpdate();
      }

Hopefully this is helpful if Epic wasn’t already aware of these issues.

Thanks,

Lucas

[Attachment Removed]

2

Steps to Reproduce
Paint multiple foliage types using FFoliageISMActor that share common meshes so that they end up in overlapping AISMPartitionActor descriptors. Eventually after adding and removing instances the internal state of the AISMPartitionActor becomes corrupted and it crashes the editor accessing invalid array items next time the map is opened.

[Attachment Removed]

3

Hi Lucas,

Thanks for the detailed report and the repro steps, really appreciated. Your analysis looks correct and these do appear to be real issues. We’ll look into this.

Thanks,

Andrzej

[Attachment Removed]