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

RBMap<StringName, V> doesn't have unique order or it is meaningless #79596

Closed
TokageItLab opened this issue Jul 18, 2023 · 2 comments
Closed
Milestone

Comments

@TokageItLab
Copy link
Member

TokageItLab commented Jul 18, 2023

Godot version

4.1

System information

Any

Issue description

Perhaps because the entity of StringName is a memory address, the order of StringName is not unique when it is used as a key in RBMap.

RBMap<StringName, V> is used in several classes/places:

  • project_settings.cpp
  • gdscript_byte_codegen.h
  • java_class_wrapper.h
  • jni_singleton.h

There are two ways to solve this problem, and we should choose one of them depends on each case:

  • Make RBMap<StringName, V> work
    • Is it correct way to go implicitly converting StringName to String in RBMap?
    • RBMap<StringName, V, StringName::AlphCompare> can be used
  • Replace RBMap<StringName, V> in some classes with HashMap<StringName, V> or RBMap<String, V>

Steps to reproduce

If you run the following code anywhere, you can see that the order changes each time Godot is launched.

RBMap<StringName, int> rbm;
rbm.insert(StringName("a"), 0);
rbm.insert(StringName("b"), 1);
rbm.insert(StringName("c"), 2);
rbm.insert(StringName("d"), 3);
rbm.insert(StringName("e"), 4);
String concat;
for (const KeyValue<StringName, int> &E : rbm) {
	concat += "[" + String(E.key) + ",";
	concat += itos(E.value) + "]";
}
WARN_PRINT(concat);

Minimal reproduction project

RBMap is a C++ class and is not exported to gdscript, so there is no project. Run the above code in any .cpp.

@AThousandShips
Copy link
Member

StringName compares by pointer, as you mentioned the appropriate way to do that is to use StringName::AlphCompare, the reason why StringName compares this way is performance, and the order will be stable, though newer names might end up before older ones depending on allocations

@TokageItLab
Copy link
Member Author

Closed by #79595.

@TokageItLab TokageItLab removed this from the 4.x milestone Oct 15, 2023
@akien-mga akien-mga added this to the 4.2 milestone Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants