Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] SpriteFrames: Sort animations alphabetically #62977

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jul 13, 2022

Previously they would be serialized in the order they come with in Map. So when adding a new animation, it would be added at the end of the Map in the current process, and thus saved at the last element in the array in the .tscn. But when loading the scene in a new process, the Map could be reordered as it doesn't depend on the serialized array.

This is only used for (de)serialization from scene/res files so I think the performance impact of having to sort the StringNames is minimal.

The unwanted reordering doesn't seem to happen in master but I'll make a PR anyway to force alphabetical ordering, and remove the obsolete frames property.

3.x version of #62978.

@akien-mga akien-mga added bug topic:editor topic:animation cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Jul 13, 2022
@akien-mga akien-mga added this to the 3.x milestone Jul 13, 2022
@akien-mga akien-mga requested a review from a team as a code owner July 13, 2022 13:53
@akien-mga akien-mga changed the title SpriteFrames: Sort animations alphabetically [3.x] SpriteFrames: Sort animations alphabetically Jul 13, 2022
@akien-mga akien-mga requested a review from kleonc July 13, 2022 14:06
@akien-mga akien-mga modified the milestones: 3.x, 3.6 Jul 13, 2022
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense and all looks good.

Since both for editor and for serializing alphabetical ordering is desired I wonder whether the internal storage of SpriteFrames could be just changed from map to ordered map. If the lookup times wouldn't be worse then it should be fine to do so? 🤔 And after such change we could remove all these sortings accross the editor/serialization. But in any case that's for potential another PR.

@akien-mga
Copy link
Member Author

Yeah I thought about that too but I noticed that e.g. in AnimationPlayer it's also using a lot of Map<StringName, SomeCustomStruct>.

Might be worth discussing on chat and see if some general rules can be figured out to establish when it's worth it to used OrderedHashMap directly, or when it's preferred to keep it unordered and just sort where needed. In master we could maybe get some kind of way to get an iterator that goes through StringName keys ordered to simplify such code.

@akien-mga akien-mga modified the milestones: 3.6, 3.5 Jul 14, 2022
@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jul 14, 2022
@akien-mga akien-mga merged commit 2642fd9 into godotengine:3.x Jul 14, 2022
@akien-mga akien-mga deleted the 3.x-spriteframes-sort-anims-alphabetically branch July 14, 2022 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants