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

GDScript: Add error message when a GDScript resource fails to load. #78540

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented Jun 21, 2023

Currently, GDScripts who are only loaded through ResourceLoader::load(), like Autoloads, do not have a pathway to announce there is an error in their code. This contributes to significant confusion when errors occur in projects when autoloads are involved. At least partially closes #78230.

@anvilfolk anvilfolk requested a review from a team as a code owner June 21, 2023 21:11
@anvilfolk anvilfolk marked this pull request as draft June 21, 2023 21:11
@@ -2698,6 +2698,10 @@ Ref<Resource> ResourceFormatLoaderGDScript::load(const String &p_path, const Str
Error err;
Ref<GDScript> scr = GDScriptCache::get_full_script(p_path, err, "", p_cache_mode == CACHE_MODE_IGNORE);

if (err) {
ERR_PRINT(vformat(R"(Failed to load script at path "%s". Error code %d.)", p_path, err));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to check whether ERR_PRINT_ED also writes an error to the console, or even whether, when autoloads get loaded, these errors would be visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it is possible to distinguish what period the error is, see #76954 (comment):

Suggested change
ERR_PRINT(vformat(R"(Failed to load script at path "%s". Error code %d.)", p_path, err));
if (scr.is_valid()){
// Errors during compilation.
}else{
// Errors during loading.
}

Copy link
Member

Choose a reason for hiding this comment

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

Great suggestion, @Rindbee

@Chaosus Chaosus added this to the 4.2 milestone Jun 22, 2023
@adamscott adamscott added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 22, 2023
@adamscott
Copy link
Member

Added the cherrypick:4.1 label as this would help 4.1 users with errors in their code, not a new feature per se.

@OmarShehata
Copy link
Contributor

Recently encountered this, and wanted to open a PR to fix but found this. @anvilfolk are you still working on this? Looks like it's ready, just needs the suggested patch applied?

Happy to help test or contribute if any help is needed here

@anvilfolk
Copy link
Contributor Author

Recently encountered this, and wanted to open a PR to fix but found this. @anvilfolk are you still working on this? Looks like it's ready, just needs the suggested patch applied?

Happy to help test or contribute if any help is needed here

Sorry, life's been a bit of a mess. I'm hoping to have a little more time in the coming weeks and want to work on this again, yes :)

@Rindbee so if !scr->is_valid(), then the error is usually that the file was not found? Whereas if scr->is_valid(), then the file was found, but there may be some parsing/analyzing/compilation error? Does that sound right?

@Rindbee
Copy link
Contributor

Rindbee commented Jul 31, 2023

@Rindbee so if !scr->is_valid(), then the error is usually that the file was not found?

This usually means an error occurred in GDScript::load_source_code().

r_error = script->load_source_code(p_path);
if (r_error) {
return Ref<GDScript>(); // Returns null and does not cache when the script fails to load.
}

Other than that, it's a valid script.

This is for the first load, subsequent loads are a bit different.

r_error = script->load_source_code(p_path);
if (r_error) {
return script;
}

@anvilfolk anvilfolk marked this pull request as ready for review July 31, 2023 15:19
@anvilfolk anvilfolk force-pushed the gdresloaderr branch 2 times, most recently from 36ca717 to 5a5bccb Compare July 31, 2023 15:45
@anvilfolk
Copy link
Contributor Author

anvilfolk commented Jul 31, 2023

A couple of notes:

  • I'm using ERR_PRINT_ED in an attempt to generate a visible error in the editor, but unfortunately when autoloads are loaded, the editor isn't quite open yet so the error does not appear.
  • ERR_PRINT_ED does still print an error to the console, so when you load your project in the editor, the OS console will have the error log.
  • When you run your game from the editor, the errors will show up in the editor console.
  • This approach results in "duplicate" errors, e.g. Godot will still complain about the auloload not inheriting from Node, but will now also print another error stating why (parse error, for example).
  • Will only print errors if we are not passing the error back to the caller - that means it's ResourceFormatLoaderGDScript::load's responsibility to report the error. This is actually not possible because loading tasks do use return errors but silently ignore them.

I don't think we can get around the "duplicated" errors, since ResourceLoader::load() specifically does not want to fail due to GDScript parsing errors, and will return OK if the parsing fails.

@anvilfolk anvilfolk marked this pull request as draft July 31, 2023 16:03
@anvilfolk anvilfolk force-pushed the gdresloaderr branch 2 times, most recently from 4f41d95 to a9b3b09 Compare July 31, 2023 17:06
@anvilfolk anvilfolk marked this pull request as ready for review July 31, 2023 17:16
Currently, GDScripts who are only loaded through `ResourceLoader::load()`,
like Autoloads, do not have a pathway to announce there is an error in their
code. This contributes to significant confusion in error projects when
autoloads are involved. At least partially closes godotengine#78230.
@akien-mga akien-mga merged commit 5e1671a into godotengine:master Aug 3, 2023
@akien-mga
Copy link
Member

Thanks!

@anvilfolk anvilfolk deleted the gdresloaderr branch August 3, 2023 13:34
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

Autoload scripts compile error are not reported (except confusing "Script does not inherit from Node")
8 participants