EsotericSoftware / spine-runtimes

2D skeletal animation runtimes for Spine.
http://esotericsoftware.com/
Other
4.37k stars 2.9k forks source link

[unity] Prefab instance always lists MeshFilter as override #1273

Open HaraldCsaszar opened 5 years ago

HaraldCsaszar commented 5 years ago

As reported on the Forum: http://esotericsoftware.com/forum/Unity-Nested-prefab-overrides-11476 http://esotericsoftware.com/forum/when-will-always-dirty-bug-be-fixed-16125

Problem seems to be in the double-buffered mesh that is assigned and changes again after being applied.

Resolution: Setting hide flags on the mesh did not yield the desired result in the prefab. A workaround could be to always assign the first mesh uport save or clear the mesh every time before saving it.

ledvinam commented 5 years ago

Possible solution could be to create mesh.asset which will be updated whenever we reexport spine projects but it won't set prefabs as changed.

HaraldCsaszar commented 4 years ago

Creation of an additional _Mesh.asset file is not a viable solution here, we require every instance to show at least the first frame of the starting animation (not to speak of later showing an animation in Scene view). Having one mesh asset per instance is not acceptable for obvious reasons.

Setting HideFlags at MeshFilter might be promising. Unfortunately, at a quick test it caused indefinite reloading when selecting an already created prefab. Further investigations will be necessary.

kvfreedom commented 4 years ago

Unity 2018.4.17: Selecting a prefab asset with a SkeletonRenderSeparator added in the project panel will load repeatedly.

HaraldCsaszar commented 4 years ago

Thanks @kvfreedom for reporting, I have created a separate issue ticket for this issue here: https://github.com/EsotericSoftware/spine-runtimes/issues/1626

ledvinam commented 3 years ago

I will be happy to test this once it has been resolved. Please let me know.

ledvinam commented 3 years ago

Harald, right clicking on the Sorting Layer label and click on Revert does not work.

HaraldCsaszar commented 2 years ago

While setting Fix PrefabMeshOverride to true was working in Unity 2020.3, on Unity 2021.3 enabling PrefabMeshOverride seems to have no effect and still lists the prefab as having overrides.

HaraldCsaszar commented 2 years ago

The previous fixes for this issue shall be re-evaluated and improved if possible, since the following forum threads show very high build time overhead due to cleanup of removing the procedural meshes: http://esotericsoftware.com/forum/BuildPlayerWindow-OnGUI-mouseUp-during-build-WebGL-17603 http://esotericsoftware.com/forum/PreprocessSpinePrefabMeshes-takes-forever-to-finish-17927

vhristov commented 2 years ago

While setting Fix PrefabMeshOverride to true was working in Unity 2020.3, on Unity 2021.3 enabling PrefabMeshOverride seems to have no effect and still lists the prefab as having overrides.

This seems to be related to the code here: https://github.com/EsotericSoftware/spine-runtimes/blob/4.1/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs#L103

Making this change:

--- a/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs
+++ b/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs
@@ -100,7 +100,7 @@ namespace Spine.Unity {
                         var objectOverrides = UnityEditor.PrefabUtility.GetObjectOverrides(instanceRoot);
                         foreach (UnityEditor.SceneManagement.ObjectOverride objectOverride in objectOverrides) {
                             if (objectOverride.instanceObject == meshFilter) {
-                                objectOverride.Revert();
+                                objectOverride.Revert(UnityEditor.InteractionMode.AutomatedAction);
                                 break;
                             }
                         }

seems to resolve the always dirty prefab in my case on 2022.1.15. Do you remember why it was changed from automated to user mode (the default for revert)?

HaraldCsaszar commented 2 years ago

Thanks very much for sharing your insights @vhristov! The argument UnityEditor.InteractionMode.AutomatedAction was previously removed since it caused compile errors in earlier Unity versions where only the default override Revert() without parameters was supported. Since my tests did not show any difference, I removed the argument instead of creating a branch.

Thanks for sharing that on 2022.1 the additional argument resolves the issue (and works as in earlier versions). Strangely both with- and without this argument, on Unity 2021.3 it always lists the mesh as override. Behaviour seems highly version dependent in this regard it seems. I will perform some more tests tomorrow and let update the code accordingly with version-dependent code branches.

Thanks again for all your efforts!

vhristov commented 2 years ago

Actually I think I was a bit fast in my conclusion. With the above change the prefab is not getting dirty on every update, but the mesh itself still gets marked as overrided.

HaraldCsaszar commented 2 years ago

Thanks for the followup info. Unfortunately this issue has its hidden pitfalls.

HaraldCsaszar commented 2 years ago

With the above change the prefab is not getting dirty on every update, but the mesh itself still gets marked as overrided. @vhristov May I ask what update previously led to your prefab being marked as dirty or not? Both with and without the InteractionMode.AutomatedAction argument I found the behaviour to be identical. Or could you please describe what exactly you mean by getting dirty, how in the Inspector or scene you see any difference with- and without the argument added?

vhristov commented 2 years ago

I get this star in the hierarchy as soon as I open a prefab variant with skeletonrenderer in it: image This is with disabled auto save: image

Then, even if there were no modifications in the prefab, when I close the prefab it asks for it to be saved: image

I noticed this when I updated unity from 2020.3 to 2022.1 I spend a few hours on 2021.3 but I had more than a few issues there and don't know if this was presented there. Unfortunately I don't have a commit on 2021.3 so I might need some more time to move back there and check how it behaves.

With the above change this is no longer seen, but the mesh is still marked as override even if I manually force it to revert: image

vhristov commented 2 years ago

While digging in unity's localization package for something completely unrelated I stumbled upon the following class DrivenPropertyManager. From the first glance it looks exactly what is needed here for this. Unfortunately it is internal (and as expected of this completely undocumented). According to this forum post, one day shall become official API.

I can think of a few dirty hacks how it can be accessed (ex. reflections), but I am not sure it is a good idea to do so. I am posting it here at least as a future reference when it becomes publicly available.

HaraldCsaszar commented 2 years ago

Thanks very much for all your hard work!

Using DrivenPropertyManager I could at least achieve a state where modification can be performed after saving/serialization, leaving the prefab clean and not marked as Override even after the scene's mesh changed.

At least some tests to add a call to DrivenPropertyManager.RegisterProperty before meshFilter.sharedMesh = currentMesh; in SkeletonRenderer.cs led to these changes in behaviour, serializing null as MeshFilter's mesh:

// TEST CODE START
meshFilter.sharedMesh = null;
// code below calls DrivenPropertyManager.RegisterProperty(Object driver, Object target, string propertyPath);
UnityEngine.Object driver = this;
SkeletonAnimation skeletonAnimation = this.GetComponent<SkeletonAnimation>();
if (skeletonAnimation != null)
    driver = skeletonAnimation; // note driver needs to be of the subclass to have any effect.
UnityEngine.Object target = meshFilter;
string propertyPath = "m_Mesh";
Type DrivenPropertyManagerType = Type.GetType("UnityEngine.DrivenPropertyManager,UnityEngine");
DrivenPropertyManagerType.InvokeMember("RegisterProperty", BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public, null, null, new object[3] {
    driver, target, propertyPath
});
// TEST CODE END

if (meshFilter)
    meshFilter.sharedMesh = currentMesh;            

The problem now is that depending on whether OnPostprocessPrefab is calling SetupSpinePrefabMesh or not (even with or without a call to DrivenPropertyManager.RegisterProperty before assigning the prefab asset mesh), the preview thumbnail image is again not rendered for the prefab. Perhaps this new (or better said re-occurring) problem can be fixed in another way, now that we have one more tool at our disposal.

In case you find out anything relevant, please don't hesitate to let us know. Thanks again for all your contributions!

vhristov commented 2 years ago

Was that the only change you got there? Because adding this in my project was a total disaster - Unity was crashing here and there when trying to edit prefabs, to the extent that caused the project to get corrupted and needed to clean out the library and temp folder for it to recover. I'll try to spend more time today or tomorrow trying to find the reasons for the crash. I would suspect there is something extra that needs to be done. The most noticeable difference I see from other users of the API (unity timeline and unity localization) is that they both use a singleton instance as a driver instead of a component.

How did you got to "m_Mesh"? I am unable to find anything like that in my serialized assets. I was expecting the property name to be "sharedMesh" (I am more of curious here)

In addition to that I accidentally managed to created prefab thumbnail without saving the mesh to the asset, but it was producing a lot of warnings:

SendMessage cannot be called during Awake, CheckConsistency, or OnValidate (SpineSkeleton: OnMeshFilterChanged)
UnityEngine.StackTraceUtility:ExtractStackTrace ()
Spine.Unity.SkeletonRenderer:LateUpdateMesh () (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs:588)
Spine.Unity.SkeletonRenderer:LateUpdate () (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs:486)
Spine.Unity.SkeletonAnimation:LateUpdate () (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonAnimation.cs:274)
Spine.Unity.SkeletonRenderer:Initialize (bool,bool) (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs:448)
Spine.Unity.SkeletonAnimation:Initialize (bool,bool) (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonAnimation.cs:187)
Spine.Unity.SkeletonRenderer:OnValidate () (at C:/projects/test/spine-runtimes/spine-unity/Assets/Spine/Runtime/spine-unity/Components/SkeletonRenderer.cs:354)
UnityEditor.PrefabImporterEditor:OnDestroy ()

The code:

void OnValidate()
{
    Initialize(true);
    if (!Application.isPlaying)
        LateUpdate();
}

It does not seems to work reliably (I guess that's caused by the reason for the warning, that the mesh renderer might not be yet ready during the on validate call). If we find a way to cheat the above problem, and with the combination with Revert/DrivenPropertyManager it might get possible to completely drop the prefab postprocess & build pre/post process for the prefabs, what do you think?

Update: Actually it seems to work consistently even though the warning. It is my component not properly settings the skins for the some of the prefabs (I still don't understand why only OnValidate is called)

HaraldCsaszar commented 2 years ago

Was that the only change you got there? Because adding this in my project was a total disaster

Oh dear, sorry to hear that! Strangely I had no problems initially when testing variants of this code.

How did you got to "m_Mesh"? I am unable to find anything like that in my serialized assets. I was expecting the property name to be "sharedMesh" (I am more of curious here)

While I found out the member name some time ago, I think back then I got it from a serialized prefab. When looking at a prefab file where the mesh is saved I see the following members serialized:

MeshFilter:
  m_ObjectHideFlags: 0
  m_CorrespondingSourceObject: {fileID: 0}
  m_PrefabInstance: {fileID: 0}
  m_PrefabAsset: {fileID: 0}
  m_GameObject: {fileID: 1384885617275220092}
  m_Mesh: {fileID: 0}

I assume that you didn't see m_Mesh in your prefabs since it was set to 0 and will thus not be serialized (equal to the default). Anyway, Unity always seems to use m_CamelCase for member vars as a standard, mesh and sharedMesh are just the public accessor properties that map to them.

Another way to see the private member var names is by letting reflection print all private members of the type.

Another would be using decompiler tools like ilSpy to browse the UnityEngine.dll. That's what I often used in the past when searching for usage of certain types and for hidden utility methods.

In addition to that I accidentally managed to created prefab thumbnail without saving the mesh to the asset, but it was producing a lot of warnings Update: Actually it seems to work consistently even though the warning. It is my component not properly settings the skins for the some of the prefabs (I still don't understand why only OnValidate is called)

Thats great news. While it would be great to not perform the actual task in OnValidate, perhaps the "cleanup" task that is producing the warning could be enqueued elsewhere and processed shortly thereafter. What comes to my mind is to e.g. enqueue a cleanup call in e.g. EditorApplication.update. On Canvas components it would e.g. work if OnValidate is only setting some needsUpdate flag, and then handle the actual update in Rebuild. While I assume that Rebuild is not called at the prefab, perhaps some other method is.

Anyway thanks so much for all your efforts!

vhristov commented 2 years ago

Oh dear, sorry to hear that! Strangely I had no problems initially when testing variants of this code.

I'll give it another try I suspect it might be somehow related with the prefab save and/or I might have something else dirty (TBH I don't remember now)

Another would be using decompiler tools like ilSpy to browse the UnityEngine.dll. That's what I often used in the past when searching for usage of certain types and for hidden utility methods.

Thanks for the info. It was useful. And for your information (if you are not yet aware of) https://github.com/Unity-Technologies/UnityCsReference is a good source of info about what is going under the hoods, too.

Thats great news. While it would be great to not perform the actual task in OnValidate, perhaps the "cleanup" task that is producing the warning could be enqueued elsewhere and processed shortly thereafter. What comes to my mind is to e.g. enqueue a cleanup call in e.g. EditorApplication.update. On Canvas components it would e.g. work if OnValidate is only setting some needsUpdate flag, and then handle the actual update in Rebuild. While I assume that Rebuild is not called at the prefab, perhaps some other method is.

Didn't turned out as that great of a news actually... Every modification of the mesh (not only the meshfilter) seems to result in a SendMessage from MeshFilter to Mesh (I suspect). I tried also delaying the work, but it seems like the object is already destroy on the next frame. I suspect unity is doing something like this:

The weird part is that I have not found any other lifecycle method being called, that's why it needed Initialize(true), otherwise the mesh never gets created (but somehow, "valid" was true, have not investigated why).

I haven't tested anything with canvas, as I don't see any thumbnails being generated for canvas elements (I believe I have seen a bug report somewhere for unity, but cannot find anything currently).

HaraldCsaszar commented 2 years ago

And for your information (if you are not yet aware of) https://github.com/Unity-Technologies/UnityCsReference is a good source of info about what is going under the hoods, too.

Ah, sorry, I forgot to mention this. Yes, thanks, I'm also using the UnityCsReference sources, for obvious reasons :).

Every modification of the mesh (not only the meshfilter) seems to result in a SendMessage from MeshFilter to Mesh (I suspect).

Hm, upon looking up any online references for Mesh and OnValidate I came across this simple script which does surprisingly not issue any warning messages: https://github.com/Firnox/Polyhedra/blob/main/Episode3-ViewInEditMode/Assets/Scripts/RegularPolygon.cs It seems as if not assigning the Mesh again at the MeshFilter avoids the warning. So if this proves to be the case, avoiding reassignment of the mesh in the Editor (and avoiding alternating double-buffered meshes) might perhaps resolve the problem.

I tried also delaying the work, but it seems like the object is already destroy on the next frame. Did this occur in a registered callback at EditorApplication.update or in a different lifecycle method?

vhristov commented 2 years ago

It seems as if not assigning the Mesh again at the MeshFilter avoids the warning. So if this proves to be the case, avoiding reassignment of the mesh in the Editor (and avoiding alternating double-buffered meshes) might perhaps resolve the problem.

I believe I might have already tried this. One of the tests I did was the following:

  1. Created a dummy mesh (i believe i used some cube I had in the project from another asset)
  2. Never touched meshFilter.sharedMesh, nor meshFilter.mesh (commented out most occurrence)
  3. Forced mesh to get regenerated on the currentMesh with meshFilter.sharedMesh, The warning was still presented, but moved the root cause from sharedMesh = ... to some of the lines inside FillVertexData. I suspect the code was running somehow properly because the mesh asset I used, got replaced with the spine mesh.

Anyway this scripts seems to be a pretty nice small example that can be a nice simplified playground for the meshes and prefabs (compared to the spine codebase, which can always surprise us with something we didn't expect due to its size and complexity). But I believe there might be an issue with it. According to my understanding (from how I read the unity docs) meshFilter.mesh shall be manually cleaned (IIRC DestroyImmediately) otherwise it will not be garbage collected.

Another side effect I have noticed when trying to exploit the OnValidate method was that everytime I start the editor it was regenerating the thumbnails (when I open a project folder with them), while using the saved asset it didn't do so. I suspect It might create a huge overhead in case there are a lot of prefabs in a single folder while running the editor.

Did this occur in a registered callback at EditorApplication.update or in a different lifecycle method?

The code I tried was looking something like this:

public void OnValidate()
{
    EditorApplication.delayCall+= () => // I am not 100% sure if I tried update here
    {
        Initialize(true);
    if (!Application.isPlaying)
        LateUpdate();
    }
}

The error was something like:

You are trying to use (some component/gameobject) but it was already destroyed

I do not remember the exact message to quote it, but I believe you understand what I mean. The source was some of the unity functions inside Initialize

Btw, I just noticed I have made a typo:

Every modification of the mesh (not only the meshfilter) seems to result in a SendMessage from MeshFilter to Mesh (I suspect).

This shall read "SendMessage from MeshFilter to MeshRenderer".

HaraldCsaszar commented 2 years ago

Was that the only change you got there? Because adding this in my project was a total disaster

I just had another check regarding the code modifications and noticed that I changed the lines here both from meshFilter.hideFlags = HideFlags.DontSaveInEditor to both meshFilter.hideFlags = HideFlags.DontSaveInBuild. Perhaps your initially reported crashes were coming from having the MeshFilter set to meshFilter.hideFlags = HideFlags.DontSaveInEditor and then calling RegisterProperty with this MeshFilter as target. At least that would explain the crash when first registering an element, then it gets destroyed but the DrivenPropertyManager still tries to access it or its member m_Mesh.

vhristov commented 2 years ago

That did the trick for me. No more crashes. The strange part is I am still getting thumbnails... Interesting could that be also environment related? I am using 2022.1.15f with 2d URP version 13.1.8 I can see there have been an issue in URP 13.1.1 to 7 but it seems pretty generic to be causing something like what you described.

Here is the exact commit and git tree I used for the test: https://github.com/vhristov/spine-runtimes/commit/0d76f129327266db25eadb0db6c5f8e0b4f10bb1 (based on the last spine-runtimes with unity commit in it)

HaraldCsaszar commented 2 years ago

Hm, unfortunately after some more testing and reduction of code that should in the best case no longer be necessary (the fixPrefabOverride code parts), now I can reproduce crashes. Some hideFlags are also set before calling RegisterProperty to be sure there are no leftover states, perhaps that is causing / contributing to crashes now.

Reproduction steps are not the exact same every time, but doing one more "reproduction iteration" reproduced a crash reliably:

0) Prerequisite: Having a scene with Spineboy-Pro as a SkeletonAnimation GameObject, already assigned as prefab before starting Unity. 1) Start Unity with the scene opening. 2) Apply the prefab from the Inspector (nothing changed, but lists it as override the first time) 3) Change the animation, apply the prefab changes again. 4) Enter the prefab (edit mode, enter the prefab scene) 5) Change anim inside prefab scene via Inspector (e.g. to portal animation), then save and exit prefab scene again. 6) Save scene (now it's saving the outside scene) 7) [optional?] Apply prefab overrides. Save scene again 8) Re-open the same scene. 9) Apply prefab from the Inspector (sometimes 2x) 10) Change anim, apply prefab again. -> leads to crash.

Sometimes I had to go through this whole loop a second time, but after 1-2 such iterations it crashed reliably. I will investigate the problem, just wanted to share some steps to reproduce any crashes more easily.

vhristov commented 2 years ago

I am again totally unable to reproduce the crash.... And it was WAY easier for me.. It was simple operations like Save the prefab, or RightClick -> Reimport. But it was still pretty random (lets say one out of 5 attempts). But again the last time I was testing on this commit: 6da33e5c883615eb51fe5caa88271e344314e707 now I am on the top, maybe it is related to some of the other changes, too?

I noticed something extra while trying to reproduce the crash though .... Always settings meshFilter.hideFlags = HideFlags.None; results in the following warning:

Importer(PrefabImporter) generated inconsistent result for asset(guid:33f116d6d0a4b3249a1d1db830105767) "Assets/atestprefab.prefab"

This is on base prefabs only, there is no such issue on prefab variants... I am not sure if this is because of something I have done wrong at the moment, or is it because of the DrivenPropertyManager... Do you see anything like that ?

HaraldCsaszar commented 2 years ago

I noticed something extra while trying to reproduce the crash though .... Always settings meshFilter.hideFlags = HideFlags.None; results in the following warning:

I never received any warning when saving or modifying the prefabs, neither on Unity 2020.3 nor 2021.3 where I just tested this. But upon selecting reimport on one of the prefab's parent directories, I could get the warning on one nested prefab you reported, but not on all prefabs in this directory.

I could reproduce a crash/hang with (roughly) the above steps in both Unity versions, just with slightly different behaviour: on 2021.3 it was stuck at Application.UpdateScene Waiting for Unity's code to finish executing while on 2020.3 it crashed without waiting for anything. Also when reimporting the nested prefab, I got an immediate crash on Unity 2021.3 as well.

HaraldCsaszar commented 2 years ago

Adding an UnregisterProperty counterpart in OnDestroy seems to resolve the crash issue (which would make sense). While it's somewhat strange that Unity's own code does not call UnregisterProperty unless just before registering the property, it makes total sense of course.

I added the following code in OnDestroy:

#if UNITY_EDITOR
    if (meshFilter && !Application.isPlaying /*&& UnityEditor.PrefabUtility.IsPartOfAnyPrefab(meshFilter)*/) {
        // code below calls DrivenPropertyManager.RegisterProperty(Object driver, Object target, string propertyPath);
        UnityEngine.Object driver = this;
        SkeletonAnimation skeletonAnimation = this.GetComponent<SkeletonAnimation>();
        if (skeletonAnimation != null)
            driver = skeletonAnimation; // note driver needs to be of the subclass to have any effect.
        UnityEngine.Object target = meshFilter;
        string propertyPath = "m_Mesh";
        Type DrivenPropertyManagerType = Type.GetType("UnityEngine.DrivenPropertyManager,UnityEngine");
        DrivenPropertyManagerType.InvokeMember("UnregisterProperty", BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public, null, null, new object[3] {
                        driver, target, propertyPath
                    });
    }
#endif

The main issue I just noticed with the whole RegisterProperty solution is that while the override is clear ("nothing to apply") after modifying the skeleton's mesh e.g. via changing the animation and applying it once, it is again in a "something changed" apply overrides state when reloading the same scene. That is with and without unregistering. I'm afraid unless this issue can be resolved by RegisterProperty, it's rather useless if it solves the main problem just until the next time the scene is reloaded.. But perhaps I've missed something and it could still be changed to make that work as well. If you know a solution to this, please don't hesitate to let us know of course. :)

vhristov commented 2 years ago

Adding an UnregisterProperty counterpart in OnDestroy seems to resolve the crash issue (which would make sense). While it's somewhat strange that Unity's own code does not call UnregisterProperty unless just before registering the property, it makes total sense of course.

As I mentioned before, unity's code seems to register the driver as a singleton object instead of the dynamic component itself. Do you think this might be somehow related to when they call UnregisterProperty?

The main issue I just noticed with the whole RegisterProperty solution is that while the override is clear ("nothing to apply") after modifying the skeleton's mesh e.g. via changing the animation and applying it once, it is again in a "something changed" apply overrides state when reloading the same scene. That is with and without unregistering.

Do I understand you correctly: this is in scene, not a prefab? I.e. it is a prefab instance in scene?

If I misunderstood this part I would suspect it is somehow related to the prefab mesh save. I mean I suspect the mesh gets saved while the property is not being driven. In this case, maybe it might be necessary to do a single revert of the MeshFilter right before the DrivenPropertyManager is setup?

If I understood it correctly, is it possible some modification to slip before the driver is initialized (I cannot imagine exactly how this could happen, but still...). Lets forget for a moment for the unregister part (as you said it doesn't change anything). I would image the DrivenPropertyManager marks the property as driver only after the code for it gets called. Before that moment, when a new instance is created, it is not yet setup. Is it possible somewhere before that some clear, setting to null or something is similar is happening? Could we still benefit from having the revert I mentioned in the previous case?

changing the animation and applying it once,

BTW, what does this mean exactly? Until I click Reload button changing the animation does not seems to be performed. Is this what you mean, or is there something else?

Artemko1 commented 5 months ago

Unity 2022.3.20, Spine 4.1-2023-10-02. The problem did appear to me, making overrides of meshFilter on prefab instances on scene.

I think the solution is not to try to revert meshFilter field changes, but to not create a mesh when it is not needed. If we set up a prefab, then the correct mesh will already exist in the prefab instance on the scene, right? So we don't need to create new mesh in that case. What i did is added if (UnityEditor.PrefabUtility.IsPartOfPrefabInstance(meshFilter)) { return; } near the top of SkeletonRenderer.LateUpdateMesh(), after other return statements.

I think this is a good decision and it seems to be working fine. You also need to add a check for overrides on the instance on the components that can affect the mesh generation (like SkeletonAnimation component). And if there are such overrides, still override meshFilter property with new mesh. This should cover all possible cases, but personally I don't need it.

What do you think @HaraldCsaszar?

HaraldCsaszar commented 5 months ago

@Artemko1 Thanks very much for sharing your findings. Unfortunately I'm not sure I can fully follow: if you early-out return in LateUpdateMesh, the skeleton mesh won't be updated at all, thus never showing selected skins or animations, which then don't show up in the prefab's preview mesh or the scene view (outside of play mode). Or did I misunderstand you there, and you meant something else? Or did you work around these issues in a different way?

Artemko1 commented 5 months ago

I can definitely see the mesh on scene outside of playmode and in the prefab preview mode. That is because the mesh is already generated during prefab editing. However, when out of playmode, animations are not playing for me when i select an animationName on the SkeletonAnimation component, only first animation frame is applied. Should they play fully? During playmode everything works correctly. As for skins, i haven't worked with it, but they should work too, i will explain below.

I didn't dig too deep into Spine, but what 'SkeletonAnimation' does is simply creating skeleton mesh from 'SkeletonDataAsset', right? So when we are editing a prefab, the mesh generetion should do it's thing, creating a skeleton mesh. When we are exiting prefab mode and saving, that mesh is serialized into prefab. If we put our prefab on a scene, that saved mesh will show up. We don't need to create a new mesh by now. We just built a prefab and put it on scene. There is a IsPartOfPrefabInstance() method to check if it's an instance.

If we would override some properties on our prefab instance on scene (changing skeletonDataAsset, skin, animation and maybe some other properties), only then the mesh should be rebuilt, making an override of SkeletonMesh. There are some methods in PrefabUtility class to check if component has overrides.

Artemko1 commented 5 months ago

In case you haven't got notification, @HaraldCsaszar

HaraldCsaszar commented 5 months ago

@Artemko1 Thanks for your writeup, you've got some valid points there. We will have a look whether we can perform mesh updates in the Editor only when any of the mesh-relevant properties were actually modified.