Issue with OnConstruction

Hi! I’ve been using OnConstruction() in my C++ code to set up a material instance. When I place an instance of that actor in a level, and then save the level, the Material is not saved properly, it has a value of NULL. I’m talking about the properties in OverrideMaterials.

I think the Editor rightfully refuses to serialize a pointer to a dynamic material instance. But this works fine in a Blueprint Construction script. I did a bit of a deep dive in the ExecuteConstruction() method and figured that properties that were modified by the UCS are saved by DetermineUCSModifiedProperties(), and I guess that’s what the editor uses to serialize the actor when saving, instead of the changed properties. So what happens is the value before the UCS is run is saved.

But since OnConstruction runs after all that, I have to manually run DetermineUCSModifiedProperties() in my OnConstruction to fix the issue. Is this a known bug? Or do I need to unwind my MaterialInstance manually in some other method to ensure correct serialization?

Thanks!

I think I tracked the change to commit 9cc24ed24849729. In 4.7, before I upgraded to 4.9, this was not an issue. I’m still reading the code to see if there is a better alternative to directly calling DetermineUCSModifiedProperties().

Created a pull request for a possible fix. Unsure of side effects, but works for me.
https://github.com/EpicGames/UnrealEngine/pull/1572

I took a look at the pull request and I guess I need to understand better exactly what you’re doing and what you were seeing to evaluate whether the change makes sense.

A bit (well really a lot) of background:

OnConstruction runs every time the construction script is run for an Actor, much like the UCS except in C++. So when an Actor is placed in the world, each time it is moved, each time the level is re-loaded, etc.

For BP defined Actors we wanted to support customizing properties at the instance level and since each time the construction script is run an Actor is re-instanced we needed a mechanism to copy the properties from the old instance to the new instance. This is done with the component instance data cache via delta serialization (we compare the differences between the instance and the archetype). However, if the UCS modifies a property there ends up being a conflict with which setting of the property should “win” the UCS or the instance customization.

We made the decision that the UCS should always win for a number of reasons which mostly came down to it was easier to implement, explain, and the results are the most consistent of the alternatives we discussed even if it does mean that some desirable use cases aren’t available.

So, the UCS Modified Properties exist for two purposes. First is a nicety to allow the UI to lock out changes to those properties to avoid (more) confusion when someone tries to override it at the instance level and then it just pops back to what the UCS wants it to be.

The more important purpose is to act as a filter mechanism that lets us know which properties should be stored in the component instance data cache and reapplied after the re-instancing. Any properties that are in the UCS Modified Property list (the one that gets serialized) represents changes that were made the last time the UCS ran and so regardless of whether the instance differs from the archetype it is based on we know that it was not changed as part of an instance override, so we don’t serialize it. Then after the UCS runs we use the filter list a second time as we apply the instance override properties so that if a new property has become set by the UCS as part of this compilation we throw away the instance override.

Ok, so with all that background what I need to understand, and maybe posting your OnConstruction code would help clarify this, is why running DetermineModifiedUCSProperties would help. Essentially what that would be doing is preventing any properties on the Actor instance from being persisted across reconstruction of the Actor.

Thanks Marc,

So this makes more sense than what I thought the code did at first. I thought that one of the main reasons for the property list simply was to store what was generated (or touched) by the UCS in order to avoid serializing it as it is regenerated anyway. An optimization, with the nice side-effect of avoiding the bug I was seeing, when triggered by the UCS instead of OnConstruction. I missed the instance issue.

Your last sentence sums up very well why the change fixes my issue, but ain’t good generally. This is my code in simplified form:

void AMBModule::OnConstruction(const FTransform& Transform)
{
    UStaticMeshComponent* Component = Cast<UPrimitiveComponent>(GetRootComponent());
    UMaterialInstanceDynamic* Mat = Component->CreateDynamicMaterialInstance(MaterialIndex);
    Mat->SetScalarParameterValue(InstanceValueName, 12.f);
}

Since OverrideMaterials get assigned a transient material by that code, and the property was not touched by my UCS, but IS overriden in the BP’s detail panel, then it is tagged Instanced. So the transient pointer is what gets serialized, and is reset to NULL somewhere along the way. My fix worked because it marked OverrideMaterials as not instanced?

Now normally when calling Component->CreateDynamicMaterialInstance() AFTER I saved my level, the code would read the NULL from OverrideMaterials, and then fetch the StaticMesh’s material (see UStaticMeshComponent::GetMaterial) and Instance that one, and all is good. Probably why it works for most people. But in my case, since I also did Instance to another material, OnConstruction instantiates the material from the StaticMesh, instead of the one configured in the blueprint’s details.

So now I’m thinking the right fix would be to stop storing dynamically instanced materials in OverrideMaterials (UMeshComponent::SetMaterial), have another array for those, and make sure it never gets serialized. What do you think?