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.

Sign up for GitHub

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

Optimize classnames enumeration #101489

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Optimize classnames enumeration #101489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aca5cfc
Rationalize parsing of file system cache
RandomShaper Jan 8, 2025
318af42
Include more attributes in the global class names cache
RandomShaper Jan 8, 2025
a20934c
Optimize enumeration of global classes in create dialog and autocomplete
RandomShaper Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/config/project_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,10 +1263,10 @@ void ProjectSettings::refresh_global_class_list() {
Array script_classes = get_global_class_list();
for (int i = 0; i < script_classes.size(); i++) {
Dictionary c = script_classes[i];
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base")) {
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base") || !c.has("is_abstract") || !c.has("is_tool")) {
continue;
}
ScriptServer::add_global_class(c["class"], c["base"], c["language"], c["path"]);
ScriptServer::add_global_class(c["class"], c["base"], c["language"], c["path"], c["is_abstract"], c["is_tool"]);
}
}

Expand Down
33 changes: 25 additions & 8 deletions core/object/script_language.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,10 @@ void ScriptServer::init_languages() {

for (const Variant &script_class : script_classes) {
Dictionary c = script_class;
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base")) {
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base") || !c.has("is_abstract") || !c.has("is_tool")) {
continue;
}
add_global_class(c["class"], c["base"], c["language"], c["path"]);
add_global_class(c["class"], c["base"], c["language"], c["path"], c["is_abstract"], c["is_tool"]);
}
ProjectSettings::get_singleton()->clear("_global_script_classes");
}
Expand All @@ -281,10 +281,10 @@ void ScriptServer::init_languages() {
Array script_classes = ProjectSettings::get_singleton()->get_global_class_list();
for (const Variant &script_class : script_classes) {
Dictionary c = script_class;
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base")) {
if (!c.has("class") || !c.has("language") || !c.has("path") || !c.has("base") || !c.has("is_abstract") || !c.has("is_tool")) {
continue;
}
add_global_class(c["class"], c["base"], c["language"], c["path"]);
add_global_class(c["class"], c["base"], c["language"], c["path"], c["is_abstract"], c["is_tool"]);
}
}

Expand Down Expand Up @@ -390,7 +390,7 @@ void ScriptServer::global_classes_clear() {
inheriters_cache.clear();
}

void ScriptServer::add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path) {
void ScriptServer::add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path, bool p_is_abstract, bool p_is_tool) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a particular suggestion for improvement, but I can't help wonder if there won't be other types of qualifiers we need to keep track on in the future, leading to further boolean parameters. It's probably fine, though the GDExtension compatibility might need some care.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I've added just two, I've kept them as separate args here and as separate fields in the classnames cache. For future extensions, I'd say moving to some kind of bitset or similar would be advisable. I'll check the GDExtension compat.

ERR_FAIL_COND_MSG(p_class == p_base || (global_classes.has(p_base) && get_global_class_native_base(p_base) == p_class), "Cyclic inheritance in script class.");
GlobalScriptClass *existing = global_classes.getptr(p_class);
if (existing) {
Expand All @@ -399,6 +399,8 @@ void ScriptServer::add_global_class(const StringName &p_class, const StringName
existing->base = p_base;
existing->path = p_path;
existing->language = p_language;
existing->is_abstract = p_is_abstract;
existing->is_tool = p_is_tool;
inheriters_cache_dirty = true;
}
} else {
Expand All @@ -407,6 +409,8 @@ void ScriptServer::add_global_class(const StringName &p_class, const StringName
g.language = p_language;
g.path = p_path;
g.base = p_base;
g.is_abstract = p_is_abstract;
g.is_tool = p_is_tool;
global_classes[p_class] = g;
inheriters_cache_dirty = true;
}
Expand Down Expand Up @@ -480,6 +484,16 @@ StringName ScriptServer::get_global_class_native_base(const String &p_class) {
return base;
}

bool ScriptServer::is_global_class_abstract(const String &p_class) {
ERR_FAIL_COND_V(!global_classes.has(p_class), false);
return global_classes[p_class].is_abstract;
Comment on lines +488 to +489
Copy link
Member

@KoBeWi KoBeWi Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
ERR_FAIL_COND_V(!global_classes.has(p_class), false);
return global_classes[p_class].is_abstract;
GlobalScriptClass *global_class = global_classes.getptr(p_class);
ERR_FAIL_NULL(global_class, false);
return global_class->is_abstract;

Same below.

Copy link
Member

Choose a reason for hiding this comment

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

Unresolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I forgot.

However, on this new inspection, I realize why I did it like that: for consistency, to match how all the other ScriptServer::get_global_class_*() getters are made. A change like the one you're suggesting would be ideal for a subsequent PR that addresses them all at once.

}

bool ScriptServer::is_global_class_tool(const String &p_class) {
ERR_FAIL_COND_V(!global_classes.has(p_class), false);
return global_classes[p_class].is_tool;
}

void ScriptServer::get_global_class_list(List<StringName> *r_global_classes) {
List<StringName> classes;
for (const KeyValue<StringName, GlobalScriptClass> &E : global_classes) {
Expand Down Expand Up @@ -507,12 +521,15 @@ void ScriptServer::save_global_classes() {
get_global_class_list(&gc);
Array gcarr;
for (const StringName &E : gc) {
const GlobalScriptClass &global_class = global_classes[E];
Dictionary d;
d["class"] = E;
d["language"] = global_classes[E].language;
d["path"] = global_classes[E].path;
d["base"] = global_classes[E].base;
d["language"] = global_class.language;
d["path"] = global_class.path;
d["base"] = global_class.base;
d["icon"] = class_icons.get(E, "");
d["is_abstract"] = global_class.is_abstract;
d["is_tool"] = global_class.is_tool;
gcarr.push_back(d);
}
ProjectSettings::get_singleton()->store_global_class_list(gcarr);
Expand Down
8 changes: 6 additions & 2 deletions core/object/script_language.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class ScriptServer {
StringName language;
String path;
StringName base;
bool is_abstract = false;
bool is_tool = false;
};

static HashMap<StringName, GlobalScriptClass> global_classes;
Expand All @@ -86,14 +88,16 @@ class ScriptServer {
static void thread_exit();

static void global_classes_clear();
static void add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path);
static void add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path, bool p_is_abstract, bool p_is_tool);
static void remove_global_class(const StringName &p_class);
static void remove_global_class_by_path(const String &p_path);
static bool is_global_class(const StringName &p_class);
static StringName get_global_class_language(const StringName &p_class);
static String get_global_class_path(const String &p_class);
static StringName get_global_class_base(const String &p_class);
static StringName get_global_class_native_base(const String &p_class);
static bool is_global_class_abstract(const String &p_class);
static bool is_global_class_tool(const String &p_class);
static void get_global_class_list(List<StringName> *r_global_classes);
static void get_inheriters_list(const StringName &p_base_type, List<StringName> *r_classes);
static void save_global_classes();
Expand Down Expand Up @@ -443,7 +447,7 @@ class ScriptLanguage : public Object {
virtual void frame();

virtual bool handles_global_class_type(const String &p_type) const { return false; }
virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr) const { return String(); }
virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr, bool *r_is_abstract = nullptr, bool *r_is_tool = nullptr) const { return String(); }

virtual ~ScriptLanguage() {}
};
Expand Down
8 changes: 7 additions & 1 deletion core/object/script_language_extension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ class ScriptLanguageExtension : public ScriptLanguage {

GDVIRTUAL1RC_REQUIRED(Dictionary, _get_global_class_name, const String &)

virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr) const override {
virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr, bool *r_is_abstract = nullptr, bool *r_is_tool = nullptr) const override {
Copy link
Member

Choose a reason for hiding this comment

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

These should likely be added to the GDVIRTUAL_CALL, otherwise GDExtension scripting languages won't be able to pass this information.

@dsnopek just added a system for virtual methods compatibility bindings which should work here: #100674.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm always—inexplicably—fuzzy about this GDExtension compatibility topic, like I used to be with GDNative... So, let's see if I'm getting this straight. This is the only related binding I'm being able to find: GDVIRTUAL_BIND(_get_global_class_name, "path");

Given it only cares about the first parameter, looks like we are lucky that second parameter onwards don't have an impact. Is that right?

Dictionary ret;
GDVIRTUAL_CALL(_get_global_class_name, p_path, ret);
if (!ret.has("name")) {
Expand All @@ -684,6 +684,12 @@ class ScriptLanguageExtension : public ScriptLanguage {
if (r_icon_path != nullptr && ret.has("icon_path")) {
*r_icon_path = ret["icon_path"];
}
if (r_is_abstract != nullptr && ret.has("is_abstract")) {
*r_is_abstract = ret["is_abstract"];
}
if (r_is_tool != nullptr && ret.has("is_tool")) {
*r_is_tool = ret["is_tool"];
}
return ret["name"];
}
};
Expand Down
2 changes: 1 addition & 1 deletion editor/connections_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1446,7 +1446,7 @@ void ConnectionsDock::update_tree() {
doc_class_name = String();
}

class_icon = editor_data.get_script_icon(script_base);
class_icon = editor_data.get_script_icon(script_base->get_path());
if (class_icon.is_null() && has_theme_icon(native_base, EditorStringName(EditorIcons))) {
class_icon = get_editor_theme_icon(native_base);
}
Expand Down
25 changes: 10 additions & 15 deletions editor/create_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,14 +257,9 @@ void CreateDialog::_add_type(const StringName &p_type, TypeCategory p_type_categ
inherits = ClassDB::get_parent_class(p_type);
inherited_type = TypeCategory::CPP_TYPE;
} else {
if (p_type_category == TypeCategory::PATH_TYPE || ScriptServer::is_global_class(p_type)) {
Ref<Script> scr;
if (p_type_category == TypeCategory::PATH_TYPE) {
ERR_FAIL_COND(!ResourceLoader::exists(p_type, "Script"));
scr = ResourceLoader::load(p_type, "Script");
} else {
scr = EditorNode::get_editor_data().script_class_load_script(p_type);
}
if (p_type_category == TypeCategory::PATH_TYPE) {
ERR_FAIL_COND(!ResourceLoader::exists(p_type, "Script"));
Ref<Script> scr = ResourceLoader::load(p_type, "Script");
ERR_FAIL_COND(scr.is_null());

Ref<Script> base = scr->get_base_script();
Expand All @@ -286,6 +281,10 @@ void CreateDialog::_add_type(const StringName &p_type, TypeCategory p_type_categ
inherited_type = TypeCategory::PATH_TYPE;
}
}
} else if (ScriptServer::is_global_class(p_type)) {
inherits = ScriptServer::get_global_class_base(p_type);
bool is_native_class = ClassDB::class_exists(inherits);
inherited_type = is_native_class ? TypeCategory::CPP_TYPE : TypeCategory::OTHER_TYPE;
} else {
inherits = custom_type_parents[p_type];
if (ClassDB::class_exists(inherits)) {
Expand Down Expand Up @@ -315,18 +314,14 @@ void CreateDialog::_configure_search_option_item(TreeItem *r_item, const StringN
r_item->set_metadata(0, p_type);
r_item->set_text(0, p_type);

String script_path = ScriptServer::get_global_class_path(p_type);
Ref<Script> scr = ResourceLoader::load(script_path, "Script");

ERR_FAIL_COND(scr.is_null());
is_abstract = scr->is_abstract();
is_abstract = ScriptServer::is_global_class_abstract(p_type);

String tooltip = TTR("Script path: %s");
bool is_tool = scr->is_tool();
bool is_tool = ScriptServer::is_global_class_tool(p_type);
if (is_tool) {
tooltip = TTR("The script will run in the editor.") + "\n" + tooltip;
}
r_item->add_button(0, get_editor_theme_icon(SNAME("Script")), 1, false, vformat(tooltip, script_path));
r_item->add_button(0, get_editor_theme_icon(SNAME("Script")), 1, false, vformat(tooltip, ScriptServer::get_global_class_path(p_type)));
if (is_tool) {
int button_index = r_item->get_button_count(0) - 1;
r_item->set_button_color(0, button_index, get_theme_color(SNAME("accent_color"), EditorStringName(Editor)));
Expand Down
84 changes: 41 additions & 43 deletions editor/editor_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,20 +983,6 @@ bool EditorData::script_class_is_parent(const String &p_class, const String &p_i
return true;
}

StringName EditorData::script_class_get_base(const String &p_class) const {
Ref<Script> script = script_class_load_script(p_class);
if (script.is_null()) {
return StringName();
}

Ref<Script> base_script = script->get_base_script();
if (base_script.is_null()) {
return ScriptServer::get_global_class_base(p_class);
}

return script->get_language()->get_global_class_name(base_script->get_path());
}

Variant EditorData::script_class_instance(const String &p_class) {
if (ScriptServer::is_global_class(p_class)) {
Ref<Script> script = script_class_load_script(p_class);
Expand Down Expand Up @@ -1026,22 +1012,25 @@ void EditorData::script_class_set_icon_path(const String &p_class, const String
_script_class_icon_paths[p_class] = p_icon_path;
}

String EditorData::script_class_get_icon_path(const String &p_class) const {
if (!ScriptServer::is_global_class(p_class)) {
return String();
}

String EditorData::script_class_get_icon_path(const String &p_class, bool *r_valid) const {
String current = p_class;
String ret = _script_class_icon_paths[current];
while (ret.is_empty()) {
current = script_class_get_base(current);
while (true) {
if (!ScriptServer::is_global_class(current)) {
// If the classnames chain has a native class ancestor, we're done with success.
if (r_valid) {
*r_valid = ClassDB::class_exists(current);
}
return String();
}
ret = _script_class_icon_paths.has(current) ? _script_class_icon_paths[current] : String();
HashMap<StringName, String>::ConstIterator E = _script_class_icon_paths.find(current);
if ((bool)E) {
if (r_valid) {
Copy link
Contributor

@PhairZ PhairZ Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This sets r_valid to true even if path is empty.

Copy link
Member

@akien-mga akien-mga Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

How so? If the path is not found, find would return an Iterator for a nullptr element, and casting it to bool should be false, so it wouldn't enter this branch.

godot/core/templates/hash_map.h

Line 542 in 394508d

_FORCE_INLINE_ Iterator find(const TKey &p_key) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but for plugins find returns an address with no value, which causes plugin icons not to show. For more details check #102997 and my PR #103143

I think My wording was slightly off, Sorry about that.

*r_valid = true;
}
return E->value;
}
current = ScriptServer::get_global_class_base(current);
}

return ret;
}

StringName EditorData::script_class_get_name(const String &p_path) const {
Expand Down Expand Up @@ -1126,58 +1115,67 @@ Ref<Texture2D> EditorData::_load_script_icon(const String &p_path) const {
return nullptr;
}

Ref<Texture2D> EditorData::get_script_icon(const Ref<Script> &p_script) {
Ref<Texture2D> EditorData::get_script_icon(const String &p_script_path) {
// Take from the local cache, if available.
if (_script_icon_cache.has(p_script)) {
if (_script_icon_cache.has(p_script_path)) {
// Can be an empty value if we can't resolve any icon for this script.
// An empty value is still cached to avoid unnecessary attempts at resolving it again.
return _script_icon_cache[p_script];
return _script_icon_cache[p_script_path];
}

// Fast path in case the whole hierarchy is made of global classes.
StringName class_name = script_class_get_name(p_script_path);
{
if (class_name != StringName()) {
bool icon_valid = false;
String icon_path = script_class_get_icon_path(class_name, &icon_valid);
if (icon_valid) {
Ref<Texture2D> icon = _load_script_icon(icon_path);
_script_icon_cache[p_script_path] = icon;
return icon;
}
}
}

Ref<Script> base_scr = p_script;
Ref<Script> base_scr = ResourceLoader::load(p_script_path, "Script");
while (base_scr.is_valid()) {
// Check for scripted classes.
String icon_path;
StringName class_name = script_class_get_name(base_scr->get_path());
if (base_scr->is_built_in() || class_name == StringName()) {
StringName base_class_name = script_class_get_name(base_scr->get_path());
if (base_scr->is_built_in() || base_class_name == StringName()) {
icon_path = base_scr->get_class_icon_path();
} else {
icon_path = script_class_get_icon_path(class_name);
icon_path = script_class_get_icon_path(base_class_name);
}

Ref<Texture2D> icon = _load_script_icon(icon_path);
if (icon.is_valid()) {
_script_icon_cache[p_script] = icon;
_script_icon_cache[p_script_path] = icon;
return icon;
}

// Check for legacy custom classes defined by plugins.
// TODO: Should probably be deprecated in 4.x
const EditorData::CustomType *ctype = get_custom_type_by_path(base_scr->get_path());
if (ctype && ctype->icon.is_valid()) {
_script_icon_cache[p_script] = ctype->icon;
_script_icon_cache[p_script_path] = ctype->icon;
return ctype->icon;
}

// Move to the base class.
base_scr = base_scr->get_base_script();
}

// No custom icon was found in the inheritance chain, so check the base
// class of the script instead.
String base_type;
p_script->get_language()->get_global_class_name(p_script->get_path(), &base_type);

// Check if the base type is an extension-defined type.
Ref<Texture2D> ext_icon = extension_class_get_icon(base_type);
Ref<Texture2D> ext_icon = extension_class_get_icon(class_name);
if (ext_icon.is_valid()) {
_script_icon_cache[p_script] = ext_icon;
_script_icon_cache[p_script_path] = ext_icon;
return ext_icon;
}

// If no icon found, cache it as null.
_script_icon_cache[p_script] = Ref<Texture>();
return nullptr;
_script_icon_cache[p_script_path] = Ref<Texture2D>();
return Ref<Texture2D>();
}

void EditorData::clear_script_icon_cache() {
Expand Down
7 changes: 3 additions & 4 deletions editor/editor_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class EditorData {

HashMap<StringName, String> _script_class_icon_paths;
HashMap<String, StringName> _script_class_file_to_path;
HashMap<Ref<Script>, Ref<Texture>> _script_icon_cache;
HashMap<String, Ref<Texture>> _script_icon_cache;

Ref<Texture2D> _load_script_icon(const String &p_path) const;

Expand Down Expand Up @@ -241,23 +241,22 @@ class EditorData {
void notify_scene_saved(const String &p_path);

bool script_class_is_parent(const String &p_class, const String &p_inherits);
StringName script_class_get_base(const String &p_class) const;
Variant script_class_instance(const String &p_class);

Ref<Script> script_class_load_script(const String &p_class) const;

StringName script_class_get_name(const String &p_path) const;
void script_class_set_name(const String &p_path, const StringName &p_class);

String script_class_get_icon_path(const String &p_class) const;
String script_class_get_icon_path(const String &p_class, bool *r_valid = nullptr) const;
void script_class_set_icon_path(const String &p_class, const String &p_icon_path);
void script_class_clear_icon_paths() { _script_class_icon_paths.clear(); }
void script_class_save_icon_paths();
void script_class_load_icon_paths();

Ref<Texture2D> extension_class_get_icon(const String &p_class) const;

Ref<Texture2D> get_script_icon(const Ref<Script> &p_script);
Ref<Texture2D> get_script_icon(const String &p_script_path);
void clear_script_icon_cache();

EditorData();
Expand Down
Loading