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

Remap script path when registering class. #41025

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Aug 4, 2020

Was causing class_name-defined scripts to not being loaded in exported games due to the remap from *.gd to *.gdc/*.gde.

Note: I could have used ResourceLoader::exists directly, but it seems to come with a more complex logic (thus slower).
But I'd like an opinion on this from someone more knowledgable on the topic (since e.g. ResourceLoader would allow us to specify the exptected type). @vnen @Xrayez ?

Fixes #40991

Was causing `class_name`-defined scripts to not being loaded in exported
games due to the remap from `*.gd` to `*.gdc`/`*.gde`.
@Faless Faless added bug topic:gdscript regression cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Aug 4, 2020
@Faless Faless added this to the 4.0 milestone Aug 4, 2020
@Xrayez
Copy link
Contributor

Xrayez commented Aug 4, 2020

Was causing class_name-defined scripts to not being loaded in exported games due to the remap from *.gd to *.gdc/*.gde.

I think this is due to #40781 (CC @Rubonnek). I'm surprised about myself why I didn't think about this before because I've done similar path remapping workarounds/hacks: #33266 (basically the same issue but from the other end). 😃

Note: I could have used ResourceLoader::exists directly, but it seems to come with a more complex logic (thus slower).

According to #39797, yeah it indeed may be slow, and may impact #39841, but it may be different for exists specifically.

If your solution works, I think it's ok then, especially since the task is to fix the regression first, then we should re-evaluate the way scripts are remapped, because it's very easy to break something with the current mechanism.

@@ -163,7 +164,7 @@ void ScriptServer::init_languages() {

for (int i = 0; i < script_classes.size(); i++) {
Dictionary c = script_classes[i];
if (!c.has("class") || !c.has("language") || !c.has("path") || !FileAccess::exists(c["path"]) || !c.has("base")) {
if (!c.has("class") || !c.has("language") || !c.has("path") || !FileAccess::exists(ResourceLoader::path_remap(c["path"])) || !c.has("base")) {
Copy link
Contributor

@Xrayez Xrayez Aug 4, 2020

Choose a reason for hiding this comment

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

Would FileAccess::exists work in exported project anyways? May be it's better to omit the check in tools=no builds altogether?

EDIT: works, but I'm curious why? FileAccess works on pck files? 👀

EDIT2: yes:

bool FileAccess::exists(const String &p_name) {
if (PackedData::get_singleton() && PackedData::get_singleton()->has_path(p_name)) {
return true;
}

@vnen
Copy link
Member

vnen commented Aug 4, 2020

TBH I'm not familiar with this code and the release target is broken on master so I have not tested the export. But yes, if you need to check if the file exists you need to use the remapped path.

@akien-mga akien-mga merged commit d3b5c09 into godotengine:master Aug 11, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.3.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 11, 2020
@akien-mga
Copy link
Member

Reverted with #41501 as per #40584 (comment).
The revert will also be cherry-picked for 3.2.

Xrayez referenced this pull request Nov 13, 2020
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.

Custom class (class_name) not working with 3.2.3 RC3, but it works in RC2
4 participants