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

Fix EditorFileSystemDirectory::get_file_deps() may return wrong result #83081

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Oct 10, 2023

Limit the max split to 8 to get complete deps.

deps caches the return value of ResourceLoader::get_dependencies(), which is also separated by "::" (eg uid://dnl1wt40gvcuu::::res://icon.svg).
"::" is quite popular as a splitter.

Reference: Code for storing information

String s = p_dir->files[i]->file + "::" + type + "::" + itos(p_dir->files[i]->uid) + "::" + itos(p_dir->files[i]->modified_time) + "::" + itos(p_dir->files[i]->import_modified_time) + "::" + itos(p_dir->files[i]->import_valid) + "::" + p_dir->files[i]->import_group_file + "::" + p_dir->files[i]->script_class_name + "<>" + p_dir->files[i]->script_class_extends + "<>" + p_dir->files[i]->script_class_icon_path;
s += "::";
for (int j = 0; j < p_dir->files[i]->deps.size(); j++) {
if (j > 0) {
s += "<>";
}
s += p_dir->files[i]->deps[j];
}
p_file->store_line(s);

…ults

Limit the maxsplit to `8` to get complete `deps`.

`deps` caches the return value of `ResourceLoader::get_dependencies()`,
which is also separated by "::".
"::" is quite popular as a splitter.
@KoBeWi
Copy link
Member

KoBeWi commented Oct 24, 2023

I'm wondering about the code below:

String deps = split[8].strip_edges();
if (deps.length()) {
Vector<String> dp = deps.split("<>");
for (int i = 0; i < dp.size(); i++) {
String path = dp[i];
fc.deps.push_back(path);
}
}

It uses the last split and splits it using <> separator. Which is weird, because the dependencies are written with :: 🤔Also the :: has caused an error in the past (#78242).
Not sure what does this code do, it seems it wasn't working correctly for quite some time.

In any case, while your change is correct, I think the splitting code I linked is wrong. It should either use ::, or better, the deps should be written using <>. Though changing how they are serialized might break something, idk. Probably not that important, given it wasn't correct (unless I'm missing something).

@Rindbee
Copy link
Contributor Author

Rindbee commented Oct 24, 2023

fi->deps = _get_dependencies(path);

ResourceLoader::get_dependencies(p_path, &deps);
Vector<String> ret;
for (const String &E : deps) {
ret.push_back(E);
}

Returns the dependencies for the resource at the given [param path].
[b]Note:[/b] The dependencies are returned with slices separated by [code]::[/code]. You can use [method String.get_slice] to get their components.
[codeblock]
for dep in ResourceLoader.get_dependencies(path):
print(dep.get_slice("::", 0)) # Prints UID.
print(dep.get_slice("::", 2)) # Prints path.
[/codeblock]

if (next_tag.fields.has("uid")) {
// If uid exists, return uid in text format, not the path.
String uidt = next_tag.fields["uid"];
ResourceUID::ID uid = ResourceUID::get_singleton()->text_to_id(uidt);
if (uid != ResourceUID::INVALID_ID) {
fallback_path = path; // Used by Dependency Editor, in case uid path fails.
path = ResourceUID::get_singleton()->id_to_text(uid);
using_uid = true;
}
}
if (!using_uid && !path.contains("://") && path.is_relative_path()) {
// Path is relative to file being loaded, so convert to a resource path.
path = ProjectSettings::get_singleton()->localize_path(local_path.get_base_dir().path_join(path));
}
if (p_add_types) {
path += "::" + type;
}
if (!fallback_path.is_empty()) {
if (!p_add_types) {
path += "::"; // Ensure that path comes third, even if there is no type.
}
path += "::" + fallback_path;
}

The format of a single dep is UID::TYPE::FILEPATH (TYPE usually does not exist). And if it is a script, the format is FILEPATH (personally, I feel that for non-scripts, it would be better if its format is FILEPATH::TYPE::UID, and I'm curious what dep.get_slice("::", 1) is.)

Eg. the dependencies of c.tscn include res://a.tscn, res://b.tscn, res://c.gd, res://Label2.gd.

These single dep:

uid://b0rtjjhymwmhd::::res://a.tscn
uid://cdw0p6dbf3p36::::res://b.tscn
res://c.gd
res://Label2.gd

The deps will be recorded as:

uid://b0rtjjhymwmhd::::res://a.tscn<>uid://cdw0p6dbf3p36::::res://b.tscn<>res://c.gd<>res://Label2.gd

A correct and complete record should be:

c.tscn::PackedScene::4055944345688729969::1698188374::0::1::::<><>::uid://b0rtjjhymwmhd::::res://a.tscn<>uid://cdw0p6dbf3p36::::res://b.tscn<>res://c.gd<>res://Label2.gd

split would be the opposite operation.

@akien-mga akien-mga merged commit 432c75d into godotengine:master Nov 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the fix-wrong-split branch November 9, 2023 10:51
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.

None yet

4 participants