-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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] Make AnimationNodeBlendTree use OrderedHashMap
insteads Map
#79598
[3.x] Make AnimationNodeBlendTree use OrderedHashMap
insteads Map
#79598
Conversation
f4d1488
to
7a236fa
Compare
Edit: the "ordered" part seems to mean that it preserves insertion order, not comparison, I think I'd suggest |
It is sufficient if the order of the inserted order, or the order of the index list of the map depending on the key is unique. At least from testing #79595 it seems to work well, but does unique comparison results mean that the order of the index list is unique? @AThousandShips |
Pointer comparison means unique order, in one run, and unless the names are freed and reallocated they'll remain ordered, the order doesn't persist between runs though But in cases where the nose is removed and re-added it'll have a new order, if using hash map, and it'll have if the node is removed, and the string name is unallocated because it is freed Using pointer comparison is more performant, so if possible it'd be better I suspect the issue with pointers is only that it doesn't remain consistent between runs, and isn't consistent if the So if you add "ABC" and "BCD" they'll have one order, if you then remove "ABC" and re-add it, after "ABC" is freed, it'll be unknown where it'll end up Sorry if I gave a confusing answer 😅 |
7a236fa
to
bbf1834
Compare
bbf1834
to
798103b
Compare
Looking at the code for Map/RBMap, I see that the comparison by Comparator uniquely determines the index to be stored. When StringName is used as a key of the Map/RBMap:
@AThousandShips Is this correct? |
That sounds correct from my experience and reading of the code My suggestion would be then:
|
231d599
to
417c015
Compare
417c015
to
06a98b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, haven't tested the code but usage is correct
Thanks! |
3.x version of #79595. Supersedes #79531.
An ordered map must be used.
...I am not familiar with OrderedHashMap, does its order depend on the key? If it does not depend, then StringName is sufficient without making to be a String.