Incorrect use of the AssetDatabase (potential editor performance issues)

[CodeSmile-0000011110110111]

I checked the AssetUtility.cs script and found wasteful use of the AssetDatabase - this code threatens sustained editor performance:

[code=CSharp] T instance = ScriptableObject.CreateInstance<T> ();

        onBeforeAdd?.Invoke (instance);

        if (!noUndo) {
            Undo.RecordObject (asset, "Add sub-asset");
        }
        AssetDatabase.AddObjectToAsset (instance, asset);
        EditorUtility.SetDirty (asset);
        AssetDatabase.SaveAssets ();
        AssetDatabase.Refresh ();[/code]

The last three lines should be removed entirely!

Rationale:

1. There is no reason whatsoever saving all (!) assets. There's the asset object in this script, it's already marked as dirty, so please just save that one asset (but only if you have to, see #3). A user or other editor script may not want any of his own assets getting saved 'randomly'.
2. Refreshing the entire ADB is entirely unnecessary! Refresh() is never needed when an asset is created, deleted or modified through the use of AssetDatabase methods. Keep in mind that a Refresh will throw away any "unused" (cached) resources in the editor. On the next access of any asset that's not currently in use, Unity is then forced to reload that resource from disk. This will slow down the editor, more so the more resource-heavy the project is.
3. Dirtying and saving the asset is not necessary when using AddObjectToAsset with an asset Object (saving is only necessary when using the path version of the method). I have a test case here (AddObject_AddWithoutSave_Succeeds - the first method in the file as of this writing) which confirms that saving isn't necessary.

The corrected, more efficient, and editor-user-friendly code would be: [code=CSharp] T instance = ScriptableObject.CreateInstance<T> ();

        onBeforeAdd?.Invoke (instance);

        if (!noUndo) {
            Undo.RecordObject (asset, "Add sub-asset");
        }
        AssetDatabase.AddObjectToAsset (instance, asset);[/code]

[zenvin-dev]

Thank you for your input!

I can definitely appreciate your points, though I had my reasons for writing the code as-is:

• In the past, Unity seemed to randomly not serialize the nested assets properly; the SaveAssets() call seemed to fix that.
• Both the SaveAssets() and Refresh() calls are/were necessary in order for the project window to show the newly created nested objects, at least at the time I wrote the code.
• When a new Setting or Group is created, they are added to the list(s) encapsulated by their parent Group. Marking the asset as dirty is supposed to ensure that those changes are serialized.

I will have a look at the code again.
That being said, I doubt that it would cause notable performance issues as-is, considering it only runs once each time a new Setting or Group is created. Your point about assets being saved "randomly" is the more pressing matter to me here.

[zenvin-dev]

Update after a bit of testing:

• Removing the call to Refresh() causes the project window to not update, until a value in the new nested object is changed.
• Replacing the SaveAssets(), Refresh() and SetDirty() calls with a SaveAssetIfDirty() call causes the project window to not display the new nested object anymore, even after property changes (shows up after recompiling)
• A combination of SetDirty() and SaveAssetIfDirty() makes the new nested object show up, but not update its name until the project window is refreshed manually
  • Attempting to substitute AssetDatabase.ImportAsset() leads to the same behaviour

[CodeSmile-0000011110110111]

Thanks for checking this out!

>> Removing the call to Refresh() causes the project window to not update

I think there should be a UI update (RepaintAll) issued in this case. Refresh probably causes this as a side-effect. At least the way you describe it it seems more likely to be an issue of updating the asset visually rather than the actual serialization not working as expected.

What project window is this in particular, the Inspector for the selected asset or a custom EditorWindow? I'm curious to try to reproduce this myself because I want to be sure whether the AssetDatabase does behave unexpectedly or not, this may be relevant to my own projects. What would be the steps to reproduce this?

Also, what Unity version did you test this on?

[zenvin-dev]

There should be a UI update (RepaintAll) issued

The RepaintAll method - as far as I know - is part of the SceneView specifically and does not exist for other types of editor windows.
I do agree though, that it seems more like a display problem, than something wrong with serialization.

What project window is this in particular

Literally the "Project" window. Unity's file-browser implementation.

Also, what Unity version did you test this on?

Like most other of my UPM repos, I'm developing this on 2020.3.26f1