Improve error message when a GDScript instance fails to be constructed #76632

SlashScreen · 2023-04-30T21:51:27Z

I changed this error message to have a clearer cause ,since this line seems to be caused by godot attempting to initialize a script when the constructor takes more than one argument. Having a clearer message here would have saved me a lot of time.

Changes error message from a terse "Error constructing a GDScriptInstance." to "Error constructing a GDScriptInstance: {error}", where the error matches the error messages given when calling a function improperly in normal GDscript.

Bugsquad edit: This closes Inform the users that non-default-constructible nodes are a bad thing godot-proposals#5899.

Calinou · 2023-04-30T22:55:26Z

Thanks for opening a pull request!

Please amend the commit to use a human-readable commit message (git commit --amend). This way, people reading the Git log can easily figure out what's going on 🙂

SlashScreen · 2023-04-30T23:09:19Z

Like this?

Calinou · 2023-04-30T23:36:44Z

Like this?

This is much better already 🙂

To be more consistent with the existing Git history, I recommend using present tense and not finishing the commit message with a period:

Improve error message when a GDScript instance fails to be constructed

SlashScreen · 2023-05-01T00:35:28Z

Done.

dalexeev · 2023-05-01T15:45:05Z

modules/gdscript/gdscript.cpp

@@ -181,7 +181,7 @@ GDScriptInstance *GDScript::_create_instance(const Variant **p_args, int p_argco
 				MutexLock lock(GDScriptLanguage::singleton->mutex);
 				instances.erase(p_owner);
 			}
-			ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance.");
+			ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance. Make sure this script's _init() function can take 0 arguments.");


This is probably not the only possible cause of the error and we should check r_error.error if we want to display more specific errors. For example, this case corresponds to CALL_ERROR_TOO_MANY_ARGUMENTS.

Done. I added printouts for r_error.error.

dalexeev · 2023-05-05T05:46:35Z

modules/gdscript/gdscript.cpp

@@ -181,7 +182,7 @@ GDScriptInstance *GDScript::_create_instance(const Variant **p_args, int p_argco
 				MutexLock lock(GDScriptLanguage::singleton->mutex);
 				instances.erase(p_owner);
 			}
-			ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance.");
+			ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, initializer->get_name(), p_args, p_argcount, r_error) + ". Ensure this script's _init() function can take 0 arguments.");


Suggested change

ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, initializer->get_name(), p_args, p_argcount, r_error) + ". Ensure this script's _init() function can take 0 arguments.");

ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, "_init", p_args, p_argcount, r_error));

I think the clarification is unrelated to other possible errors.

Done. Pardon me, I'm not yet familiar with internal terminology.

This one is not changed.

My bad. It was late last night. I actually fixed it now. I'm a little concerned that removing the clarification may make it so that some people may not understand what it means, but you're right that the clarification is unrelated to all but one of the possible errors in the enum and may lead people astray were they to occur.

modules/gdscript/gdscript.cpp

dalexeev · 2023-05-05T06:13:24Z

This closes godotengine/godot-proposals#5899

I think this PR is just one of the necessary steps to close the issue. Also we need to add a note to the docs and a warning if _init of a Node- or Resource-derived class has required arguments.

anvilfolk · 2023-05-05T15:38:27Z

Just a little more food for thought here! :)

Might there be a way to do something similarly informative without including variant.h, or is this really the only way? It's a pretty big dependency that didn't seem to be there originally, so if we could avoid it it'd keep code a little faster to compile whenever there are changes to variant.h :)

dalexeev · 2023-05-05T15:41:45Z

modules/gdscript/gdscript.cpp

@@ -164,7 +165,7 @@ GDScriptInstance *GDScript::_create_instance(const Variant **p_args, int p_argco
 			MutexLock lock(GDScriptLanguage::singleton->mutex);
 			instances.erase(p_owner);
 		}
-		ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance.");
+		ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, "@implicit_new", nullptr, 0, r_error));


It probably makes sense to move getting the error text before line 163, since it removes the script from the object.

godot/core/variant/variant.cpp

Lines 3678 to 3681 in ce75c46

Ref<Resource> script = p_base->get_script();

if (script.is_valid() && script->get_path().is_resource_file()) {

base_text += "(" + script->get_path().get_file() + ")";

}

godot/core/object/object.cpp

Lines 868 to 872 in e0e93ce

if (p_instance) {

script = p_instance->get_script();

} else {

script = Variant();

}

is this something I should do?

Also, does err_v stop the script? If it does, it's good to keep the memory freeing here before the message, to not cause a memory leak.

Yes, ERR_FAIL_* macros have return. I think you should use a variable:

String error_text = Variant::get_call_error_text(...); instance->owner->set_script_instance(nullptr); ... ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + error_text);

I see, so you're suggesting assigning the variable to grab the message before the script instance is removed, so that we make sure we get the right error?

I think so, in this case the error text will contain the script file name, judging by the snippets that I have given above.

Seems like a good idea. I changed that just now.

SlashScreen · 2023-05-05T16:20:21Z

Just a little more food for thought here! :)

Might there be a way to do something similarly informative without including variant.h, or is this really the only way? It's a pretty big dependency that didn't seem to be there originally, so if we could avoid it it'd keep code a little faster to compile whenever there are changes to variant.h :)

I thought about that, but I would have to copy and paste the get call error text function.

SlashScreen · 2023-05-09T04:21:17Z

@anvilfolk does actually bring up a good point, though; should I instead copy the call message function into gdscript.cpp? Then, I can make the error messages more clear, since the function doesn't have to be for anything other than these errors. And then I can remove the header file, so it doesn't have to recompile. I'd hate to have my one little change to add a whole bunch of compile time.

anvilfolk · 2023-05-09T10:57:06Z

I'd love to have someone else's opinion on this for sure :) maybe it's not a big deal since I don't think Variant.h gets modified that often? If the function isn't huge it might make sense! Let's see what others have to say :)

Edit: I also just realized that it's an include from a .cpp file, which is way less of a big deal than an include from another .h file :)

joao-pedro-braz · 2023-05-09T11:12:42Z

I don't see why we shouldn't just copy the error message over, instead of adding another include to gdscript.cpp.
It's not like that error message is gonna be changed often anyways.

anvilfolk · 2023-05-09T11:16:41Z

I don't see why we shouldn't just copy the error message over, instead of adding another include to gdscript.cpp. It's not like that error message is gonna be changed often anyways.

I went and checked and the function is still like ~30 lines of code. Not a ton, but definitely not the most trivial thing either - it checks for lots of different things. @SlashScreen do you need to check for all of those or do you think that you'd only need a small part of that function? If it really is just checking for one or two conditions, then copy-pasting here makes a ton of sense. Copy pasting 30 lines of code I'd be a little more worried about - especially if the copy pasting requires the includes anyway! :)

adamscott · 2023-05-09T12:53:53Z

Isn't Variant already accessible within "gdscript.cpp" without importing it? I think the include is not needed as "core/variant/variant.h" is already implicitly included by the other includes.

Well, guess it doesn't hurt to import it neither.

dalexeev · 2023-05-09T17:26:23Z

Perhaps this function should be moved from Variant to Callable? This is a core or buildsystem question, so I may be wrong, but this move seems logical to me, since CallError belongs to Callable.
The code looks ok to me, I think the duplication here is undesirable.
However, I'm not entirely sure about the function of this error. GDScriptInstance is an internal implementation detail that is of little interest to users. As I wrote above, we could add a warning if the constructor of a Node- or Resource-derived class has required arguments.

SlashScreen · 2023-05-09T17:40:50Z

@dalexeev

It would make sense to move the error over to callable, at least logically, but I don't know what that would break, and I am not comfortable enough with c++ and the godot codebase to make that move myself, unfortunately.
it's an error I keep running into, and I have seen other people run into it a few times. Having a more helpful error message here would have saved me hours of frustrated debugging, which is why I opened the PR in the first place.

Since we appear to have determined that the include is less of an issue than copy and pasting the function (which I was already hesitant to do anyways, since it was setting off my mental Don't Repeat Yourself alarms), is there anything else that I need to address before the PR can be merged (not to sound too presumptuous)?

anvilfolk · 2023-05-09T17:52:18Z

since it was setting off my mental Don't Repeat Yourself alarms

🌵 🌵 🌵 🌵 DRY alarm! DRY alarm! 🌵 🌵 🌵 🌵

In seriousness, I'd remove the #include since it's already implied elsewhere and is thus redundant :)

The other thing that might make folks a little more excited about this is if you showed the original error vs the new error in your PR description? I'd love to see it myself!

anvilfolk · 2023-05-09T17:56:48Z

Oh, I also just noticed that you have 9 commits. You should squash that into a single commit :)

More information here: https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html :)

SlashScreen · 2023-05-09T18:00:34Z

Okay. I'll rebase it this afternoon.

SlashScreen · 2023-05-12T04:28:08Z

I think I got the squashing to work.

akien-mga · 2023-05-12T09:26:58Z

There was now 11 commits in the PR, so it seems like after squashing, you pulled from your remote again, solved the unnecessarily conflicts, and thus ended up having the changes done multiple times.

So I went ahead and rebased the branch for you. Note that it means your fork's master branch is now different from your local Git clone's. For future PRs, we recommend creating a dedicated branch (e.g. SlashScreen/gdscript-error-instance-fail instead of SlashScreen/master), which makes it less likely that you would mistakenly pull your divergent master branch into it again, and also make it easier for you to maintain multiple branches in parallel for separate PRs.

anvilfolk · 2023-05-12T10:54:18Z

@SlashScreen if you want some more interactive support with this, and are interested in participating more in godot I highly recommend the dev chat at https://chat.godotengine.org/ :)

SlashScreen · 2023-05-12T18:02:02Z

@akien-mga Whoops! Sorry. I'll do that in the future. I'm not super experienced with branch manipulation. Thanks for fixing that up.

@anvilfolk I've spoken in there a few times, and ill be sure to look there if I need help. Although, I think everything is resolved now.

dalexeev · 2023-08-03T08:24:16Z

Array changes are unrelated. Looks like a git rebase mistake.

anvilfolk · 2023-08-03T12:13:07Z

Yeah, there's two commits now - other than that I'd be happy to see this in :)

AThousandShips · 2024-01-09T12:38:06Z

Superseeded by: #86999

Thank you for your contribution nonetheless!

SlashScreen requested a review from a team as a code owner April 30, 2023 21:51

Calinou added enhancement topic:gdscript cherrypick:4.0 labels Apr 30, 2023

Calinou added this to the 4.x milestone Apr 30, 2023

SlashScreen changed the title ~~Changed error message at 184 to be clearer~~ Changed error message when godot fails to construct a script to be less vague. Apr 30, 2023

SlashScreen changed the title ~~Changed error message when godot fails to construct a script to be less vague.~~ Improve error message when a GDScript instance fails to be constructed May 1, 2023

dalexeev reviewed May 1, 2023

View reviewed changes

SlashScreen requested a review from dalexeev May 4, 2023 08:21

dalexeev reviewed May 5, 2023

View reviewed changes

SlashScreen requested a review from dalexeev May 5, 2023 06:23

dalexeev reviewed May 5, 2023

View reviewed changes

SlashScreen requested a review from dalexeev May 8, 2023 23:44

Improve error message when a GDScript instance fails to be constructed

628bf85

Brought over code from other branch. This one should be clean...

6e1da77

SlashScreen requested a review from a team as a code owner August 3, 2023 08:17

YuriSizov removed the cherrypick:4.0 label Dec 4, 2023

AThousandShips mentioned this pull request Dec 30, 2023

Improve error message when trying to instantiate a script with _init(...) #86635

Closed

akien-mga added needs work salvageable labels Jan 9, 2024

AThousandShips mentioned this pull request Jan 9, 2024

Improve error message when a GDScript instance fails to be constructed #86999

Merged

AThousandShips closed this Jan 9, 2024

AThousandShips added archived and removed needs work salvageable labels Jan 9, 2024

AThousandShips removed this from the 4.x milestone Jan 9, 2024

	ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, initializer->get_name(), p_args, p_argcount, r_error) + ". Ensure this script's _init() function can take 0 arguments.");
	ERR_FAIL_V_MSG(nullptr, "Error constructing a GDScriptInstance: " + Variant::get_call_error_text(instance->owner, "_init", p_args, p_argcount, r_error));

	Ref<Resource> script = p_base->get_script();
	if (script.is_valid() && script->get_path().is_resource_file()) {
	base_text += "(" + script->get_path().get_file() + ")";
	}

	if (p_instance) {
	script = p_instance->get_script();
	} else {
	script = Variant();
	}

Improve error message when a GDScript instance fails to be constructed #76632

Improve error message when a GDScript instance fails to be constructed #76632

Conversation

SlashScreen commented Apr 30, 2023 • edited Loading

Calinou commented Apr 30, 2023

SlashScreen commented Apr 30, 2023

Calinou commented Apr 30, 2023 • edited Loading

SlashScreen commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexeev commented May 5, 2023 • edited Loading

anvilfolk commented May 5, 2023

dalexeev May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexeev May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlashScreen commented May 5, 2023

SlashScreen commented May 9, 2023

anvilfolk commented May 9, 2023 • edited Loading

joao-pedro-braz commented May 9, 2023

anvilfolk commented May 9, 2023

adamscott commented May 9, 2023 • edited Loading

dalexeev commented May 9, 2023

SlashScreen commented May 9, 2023 • edited Loading

anvilfolk commented May 9, 2023 • edited Loading

anvilfolk commented May 9, 2023

SlashScreen commented May 9, 2023

SlashScreen commented May 12, 2023

akien-mga commented May 12, 2023 • edited Loading

anvilfolk commented May 12, 2023

SlashScreen commented May 12, 2023

dalexeev commented Aug 3, 2023

anvilfolk commented Aug 3, 2023

AThousandShips commented Jan 9, 2024

SlashScreen commented Apr 30, 2023 •

edited

Loading

Calinou commented Apr 30, 2023 •

edited

Loading

dalexeev commented May 5, 2023 •

edited

Loading

dalexeev May 5, 2023 •

edited

Loading

dalexeev May 5, 2023 •

edited

Loading

anvilfolk commented May 9, 2023 •

edited

Loading

adamscott commented May 9, 2023 •

edited

Loading

SlashScreen commented May 9, 2023 •

edited

Loading

anvilfolk commented May 9, 2023 •

edited

Loading

akien-mga commented May 12, 2023 •

edited

Loading