talecrafter / AnimationImporter

Aseprite Animation Importer for Unity
613 stars 72 forks source link

Reimporting animations sets references to sprites to 0, breaks reference when moving computers #58

Closed GieziJo closed 1 year ago

GieziJo commented 1 year ago

As referenced in this post in unity, when reimporting a file, the metadata reference gets broken, and sets the Ids to 0.

Example: image

They should all have IDs.

Unity version 2022.1.20f

tuntorius commented 1 year ago

It gets broken on the same computer as well. If you add 2 or more new sprites to a sheet at the same time, they all get ids of 0 and can't be distinguished from one another. This is because unity made TextureImporter.spriteSheet property obsolete. Not deprecated, but obsolete - it stopped working at all for Unity 2021.2+. For that version upwards, you need to use UnityEditor.U2D.Sprites.ISpriteEditorDataProvider interface instead: https://docs.unity3d.com/2022.2/Documentation/ScriptReference/TextureImporter-spritesheet.html

GieziJo commented 1 year ago

Thanks for that.

Agreed, I also had issues with the same computer as well.

I'm not sure it;s the root of the problem, it's only obsolete for versions > 2022.1, and I'm using that one, but still getting the error.

Maybe it's not obsolete but not actually working correctly?

I also saw that if you re-import the file ids get completely screwed up.

My local fix, which kinda worked, but means re-linking all images in the animation is to delete file table id in the meta file, when it gets regenerated it then works.

Honestly I'm a bit lost with this one, not sure how to fix it, would be super happy to get help.

tuntorius commented 1 year ago

The page I linked clearly says it's obsolete. It also says "Support for accessing sprite meta data through spritesheet has been removed." It's obsolete since 2021.2.0a7, not 2022. The older docs are not updated.

This issue shows which is the first version with this change: https://issuetracker.unity3d.com/issues/identifier-uniqueness-violation-warning-go-loses-sprite-when-using-textureimporter-dot-spritesheet-to-split-an-image-into-sprites It has links explaining the new APIs.

I can only give general pointers on how to fix it. The spritesheet property must be replaced with the new API. It looks like it's used here: https://github.com/talecrafter/AnimationImporter/blob/517f4667d21a94680cec8118cf63a15053bff711/Assets/AnimationImporter/Editor/AnimationImporter.cs#L472

Here are some examples of how to use the new API: https://docs.unity3d.com/Packages/com.unity.2d.sprite@1.0/manual/DataProvider.html

First, you need to obtain ISpriteEditorDataProvider instance, which the first example shows how. Then, the spritesheet metadata has to be updated. The example "How to change Sprite data" shows that. This line: dataProvider.SetSpriteRects(spriteRects); is the equivalent of the old and broken: importer.spritesheet = ... They both receive an array of sprite rects that describe the spritesheet. However, the old one uses UnityEditor.SpriteMetadata, the new one UnityEditor.SpriteRect. They are different classes, so not interchangeable, but somewhat similar. They both have pivot, border, rect. However, the new one has spriteID!!! This is the ID that becomes 0. It's missing from the old one.

Here it becomes complicated and I can't help anymore. Now it's the responsibility of the plugin to manipulate that ID. For existing frames, it must retrieve the ID from the spritesheet and keep it the same. For new frames it must generate a new ID using GUID.Generate(); This means the code behind animationSheet.GetSpriteSheet() must be updated to use the new SpriteRect classes and respect the IDs. It looks like a lot of work.

I'm not using this plugin. I use another, which is just a sprite packer. It had the exact same problem. The change there was easier and I fixed it. It took me several days though.

GieziJo commented 1 year ago

The older docs are not updated.

Ah okay, that's why I got confused, thanks for the clarification.

Thanks a lot for the details on how you made it all work for this other package.

I am not the main developer of the tool, I just helped lately with a couple of new features, so I'm not sure I would be able to do these changes, but I can try.

I don't know if @talecrafter is still interested in keeping this tool updated? I know a lot of people are actively using it and would probably be glad if this gets fixed, but I understand if this is not a priority anymore.

Thanks a lot for your inputs!

talecrafter commented 1 year ago

I will take a look at the problem.

Sorry for not maintaining this tool lately. I was really busy and overwhelmed on my own project, and still am.

I'm using 2021.3 currently and the sprite import seems to work fine still without compilation error. (Not 100% sure I'm still at the exact same code as this Github version.) But occasionally I get a warning about invalid IDs on the sprites, so I guess that's related.

talecrafter commented 1 year ago

Does anyone have an idea why the UnityEditor.U2D.Sprites namespace is not available while the com.talecrafter.animationimporter Assembly definition exists? I'm using Unity 2021.3 and once I delete that assembly definition, the new API becomes available.

tuntorius commented 1 year ago

You need to add a reference to Unity.2D.Sprite.Editor to your assembly definition.

talecrafter commented 1 year ago

Thank you, @tuntorius. That was indeed the issue.

@GieziJo: I implemented a simple version of reusing the Sprite IDs on a new beta branch. For the Unity package, you would use this url: https://github.com/talecrafter/AnimationImporter.git?path=/Assets#beta Can you check if this version fixes your issues?

GieziJo commented 1 year ago

@talecrafter Thanks a lot!

Will check this this weekend and let you know.

GieziJo commented 1 year ago

I think this worked!

I had an issue with the importer, where if animations have been imported incorrectly, the spriteID is set to 00000000000000000800000000000000 and is the same for all sprites.

I added a catch and regenerate the ID if it is detected (here):

try
{
    dataProvider.SetSpriteRects(newSpriteRects);
}
catch (Exception _)
{
    for (int i = 0; i < newSpriteRects.Length; i++)
    {
        if(newSpriteRects[i].spriteID == new GUID("00000000000000000800000000000000"))
            newSpriteRects[i].spriteID = GUID.Generate();
    }
    dataProvider.SetSpriteRects(newSpriteRects);
}

I think this is not necessary if a sprite had not been imported with a version where the importer is obsolete, but it was in my case.

Are we happy with including this or is there a better option for that?

Thanks a lot for the updated package!

talecrafter commented 1 year ago

I saw this specific fault ID in a handful of my assets, too. Refactored the check for that ID to be part of IsValidGUID in ImportedAnimationSheet. As far as I can see there's no need to retry changing the GUIDs on Exception as the IDs in newSpriteRects should be fine and if there's an error, it's due to other problems.

talecrafter commented 1 year ago

I merged this on the main branch and deleted the beta branch (for now).

https://github.com/talecrafter/AnimationImporter/releases/tag/1.5

Closing this now.

GieziJo commented 1 year ago

That is a much cleaner solution, awesome!

Thanks a lot!