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

Add is_built_in() method to Resource #50352

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 10, 2021

A built-in Resource can be recognized by its path, which may be:

  • empty
  • containing ::
  • starting with local://? idk, I've seen it in few places

There were such checks all over the codebase, but they were inconsistent, which led to some bugs, e.g. #45950 or partially #49086 (_is_built_in_script() is not checking for empty paths, so the issue isn't fully fixed).

To fix inconsistencies I added is_built_in() to resources. I replaced equivalent conditions with this method.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Let's merge, but maybe warrants a rebase first to be sure it still builds fine.

@KoBeWi KoBeWi requested a review from a team as a code owner November 4, 2021 11:46
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 4, 2021

Rebased. I also replaced a few more conditions (that likely popped out since the PR was made).

I seen a few partial conditions, which only check for "::", I didn't touch them as I wasn't sure.
There is also String.is_resource_file(), which I didn't replace too.

@akien-mga
Copy link
Member

I seen a few partial conditions, which only check for "::", I didn't touch them as I wasn't sure.

Would probably be worth reviewing in a follow-up PR, I guess the intent was to check if it's a builtin resource. What else would use ::?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 4, 2021

The ones left out are:

godot/core/string/ustring.cpp

Lines 4338 to 4340 in 78931aa

bool String::is_resource_file() const {
return begins_with("res://") && find("::") == -1;
}

This is used in lots of places. Some of them (res.get_path().is_resource_file()) could be replaced by is_built_in().

if (n.find("::") != -1) {
path = n.get_slice("::", 0);
type = n.get_slice("::", 1);
} else {
path = n;
type = "Resource";
}

Doesn't look related to file paths.

for (int i = 0; i < files[p_idx]->deps.size(); i++) {
String dep = files[p_idx]->deps[i];
int sep_idx = dep.find("::"); //may contain type information, unwanted
if (sep_idx != -1) {
dep = dep.substr(0, sep_idx);
}

No idea here.

godot/editor/editor_node.cpp

Lines 1200 to 1208 in 78931aa

String path = p_resource->get_path();
int srpos = path.find("::");
if (srpos != -1) {
String base = path.substr(0, srpos);
if (!get_edited_scene() || get_edited_scene()->get_scene_file_path() != base) {
show_warning(TTR("This resource can't be saved because it does not belong to the edited scene. Make it unique first."));
return;
}
}

It uses the index of "::", but the performance probably doesn't matter here. Could be replaced.

godot/editor/editor_node.cpp

Lines 2159 to 2168 in 78931aa

int subr_idx = current_res->get_path().find("::");
if (subr_idx != -1) {
String base_path = current_res->get_path().substr(0, subr_idx);
if (FileAccess::exists(base_path + ".import")) {
editable_warning = TTR("This resource belongs to a scene that was imported, so it's not editable.\nPlease read the documentation relevant to importing scenes to better understand this workflow.");
} else {
if ((!get_edited_scene() || get_edited_scene()->get_scene_file_path() != base_path) && ResourceLoader::get_resource_type(base_path) == "PackedScene") {
editable_warning = TTR("This resource belongs to a scene that was instantiated or inherited.\nChanges to it won't be kept when saving the current scene.");
}
}

Same as above.

int sep_pos = r->get_path().find("::");
if (sep_pos >= 0) {
extra_path = base_path.substr(sep_pos, base_path.length());
base_path = base_path.substr(0, sep_pos);
}

Not sure here.

String path = var;
if (path.find("::") != -1) {
// built-in resource
String base_path = path.get_slice("::", 0);
RES dependency = ResourceLoader::load(base_path);
if (dependency.is_valid()) {
remote_dependencies.insert(dependency);
}
}

Could be replaced probably.

} else if (path.find("::") != -1) {
// built-in script

It checks !path.is_resource_file() later, so it confused me a bit.

@akien-mga akien-mga merged commit c4f29b0 into godotengine:master Nov 4, 2021
@akien-mga
Copy link
Member

Thanks!

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.

2 participants