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

Register the export info correctly when a script is used as the variable type for Node #90487

Conversation

preslavnpetrov
Copy link
Contributor

@preslavnpetrov preslavnpetrov commented Apr 10, 2024

This PR fixes #73993

It will allow a preloaded const script to be used as the type of an exported variable in GDScript

@preslavnpetrov preslavnpetrov requested a review from a team as a code owner April 10, 2024 12:38
@akien-mga akien-mga added this to the 4.3 milestone Apr 10, 2024
@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch 2 times, most recently from dc699a4 to 4ddb6dc Compare April 10, 2024 15:03
@preslavnpetrov preslavnpetrov requested a review from dalexeev April 10, 2024 23:16
@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch from 4ddb6dc to 6e7aea9 Compare May 4, 2024 17:30
@preslavnpetrov preslavnpetrov requested a review from dalexeev May 4, 2024 17:31
@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch from 6e7aea9 to def8cc0 Compare May 4, 2024 17:41
@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch from def8cc0 to 5eed3b9 Compare May 30, 2024 15:32
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Sorry for delay. Here's my suggestion for making CLASS and SCRIPT cases consistent:

Patch
diff --git a/modules/gdscript/gdscript_parser.cpp b/modules/gdscript/gdscript_parser.cpp
index da599bf57b..771ccf47b7 100644
--- a/modules/gdscript/gdscript_parser.cpp
+++ b/modules/gdscript/gdscript_parser.cpp
@@ -4327,11 +4327,11 @@ bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node
 				if (ClassDB::is_parent_class(export_type.native_type, SNAME("Resource"))) {
 					variable->export_info.type = Variant::OBJECT;
 					variable->export_info.hint = PROPERTY_HINT_RESOURCE_TYPE;
-					variable->export_info.hint_string = export_type.to_string();
+					variable->export_info.hint_string = class_name;
 				} else if (ClassDB::is_parent_class(export_type.native_type, SNAME("Node"))) {
 					variable->export_info.type = Variant::OBJECT;
 					variable->export_info.hint = PROPERTY_HINT_NODE_TYPE;
-					variable->export_info.hint_string = export_type.to_string();
+					variable->export_info.hint_string = class_name;
 				} else {
 					push_error(R"(Export type can only be built-in, a resource, a node, or an enum.)", p_annotation);
 					return false;
@@ -4340,27 +4340,24 @@ bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node
 
 			case GDScriptParser::DataType::SCRIPT: {
 				StringName class_name;
-				StringName native_base;
 				if (export_type.script_type.is_valid()) {
-					class_name = export_type.script_type->get_language()->get_global_class_name(export_type.script_type->get_path());
-					native_base = export_type.script_type->get_instance_base_type();
+					class_name = export_type.script_type->get_global_name();
 				}
 				if (class_name == StringName()) {
 					Ref<Script> script = ResourceLoader::load(export_type.script_path, SNAME("Script"));
 					if (script.is_valid()) {
-						class_name = script->get_language()->get_global_class_name(export_type.script_path);
-						native_base = script->get_instance_base_type();
+						class_name = script->get_global_name();
 					}
 				}
 				if (class_name == StringName()) {
 					push_error(R"(Script export type must be a global class.)", p_annotation);
 					return false;
 				}
-				if (native_base != StringName() && ClassDB::is_parent_class(native_base, SNAME("Resource"))) {
+				if (ClassDB::is_parent_class(export_type.native_type, SNAME("Resource"))) {
 					variable->export_info.type = Variant::OBJECT;
 					variable->export_info.hint = PROPERTY_HINT_RESOURCE_TYPE;
 					variable->export_info.hint_string = class_name;
-				} else if (native_base != StringName() && ClassDB::is_parent_class(native_base, SNAME("Node"))) {
+				} else if (ClassDB::is_parent_class(export_type.native_type, SNAME("Node"))) {
 					variable->export_info.type = Variant::OBJECT;
 					variable->export_info.hint = PROPERTY_HINT_NODE_TYPE;
 					variable->export_info.hint_string = class_name;

@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch from 5eed3b9 to 590763a Compare June 3, 2024 09:50
@preslavnpetrov preslavnpetrov force-pushed the export-script-typed-node-variables branch from 590763a to 653a8b1 Compare June 3, 2024 12:29
@akien-mga akien-mga merged commit ef065d2 into godotengine:master Jun 3, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@shakesoda
Copy link

this merge seems to break spatial gardener (and, looks like, anything that extends with a script path instead of a class name)

@dalexeev
Copy link
Member

dalexeev commented Jun 4, 2024

@shakesoda Please open a new issue if you find a regression. Note that @export requires global classes, as documented (unfortunately, the Inspector does not currently support unnamed and inner classes). The previous behavior with no GDScript error was a bug, there was an inconsistency between the SCRIPT and CLASS datatypes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants