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

Make AnimationNodeBlendTree use RBMap instead HashMap #79595

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jul 18, 2023

Fixes #42788. Alternative PR of #79530.

node_connections is stored in ordered array, but the order of the array depends on the HashMap nodes, so every time the hash is changed, the .tres is changed.

This PR ensures uniqueness in the order of nodes by using an RBMap for nodes.

However, there seems to be a problem with inconsistent insertion locations when StringName is used as the key for RBMap (see also #79596). Therefore, RBMap<String, Node> should be used instead of RBMap<StringName, Node>.

Although not related to this PR, if you are using RBMap<StringName, V> in other classes, you may need to replace it with HashMap<StringName, V> or RBMap<String, V>.

@TokageItLab TokageItLab added this to the 4.2 milestone Jul 18, 2023
@TokageItLab TokageItLab requested a review from a team as a code owner July 18, 2023 01:09
@TokageItLab TokageItLab changed the title Make AnimationNodeBlendTree use RBMap insteads HashMap Make AnimationNodeBlendTree use RBMap insteads HashMap Jul 18, 2023
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Discussed deterministic sort and using an ordered map off Github.

@AThousandShips
Copy link
Member

AThousandShips commented Jul 18, 2023

Can't you just use RBMap<StringName, Node, StringName::AlphCompare>?

@TokageItLab
Copy link
Member Author

TokageItLab commented Jul 18, 2023

Can't you just use RBMap<StringName, Node, StringName::AlphCompare>?

!!

You are right, I didn't care that RBMap has a Comparator. It is working perfectly.

So this is now a one-line change. Thanks!

@TokageItLab TokageItLab force-pushed the rbmap-animblendtree branch from 348e5c7 to 29d63f4 Compare July 18, 2023 06:45
@AThousandShips
Copy link
Member

That could probably have been suggested in the original PR going forward

@TokageItLab TokageItLab force-pushed the rbmap-animblendtree branch from 29d63f4 to a3cdacd Compare July 18, 2023 08:58
@YuriSizov YuriSizov merged commit 6a30f64 into godotengine:master Jul 21, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov changed the title Make AnimationNodeBlendTree use RBMap insteads HashMap Make AnimationNodeBlendTree use RBMap instead HashMap Jul 28, 2023
@TokageItLab TokageItLab deleted the rbmap-animblendtree branch February 14, 2024 05:38
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.

Nondeterministic .tscn data from AnimationBlendTreeNode
4 participants