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.

Sign up for GitHub

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

Jump to bottom

Allow exporting custom Resource variables from GDscript #41264

Closed
wants to merge 1 commit into from
Closed

Allow exporting custom Resource variables from GDscript #41264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/script_language.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,18 @@ StringName ScriptServer::get_global_class_native_base(const String &p_class) {
}
return base;
}

StringName ScriptServer::get_global_class_name(const String &p_path, String *r_base_type, String *r_icon_path) {
for (int i = 0; i < get_language_count(); i++) {
ScriptLanguage *lang = get_language(i);
StringName class_name = lang->get_global_class_name(p_path, r_base_type, r_icon_path);
if (class_name != StringName()) {
return class_name;
}
}
return StringName();
}

void ScriptServer::get_global_class_list(List<StringName> *r_global_classes) {
const StringName *K = NULL;
List<StringName> classes;
Expand Down
1 change: 1 addition & 0 deletions core/script_language.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class ScriptServer {
static String get_global_class_path(const String &p_class);
static StringName get_global_class_base(const String &p_class);
static StringName get_global_class_native_base(const String &p_class);
static StringName get_global_class_name(const String &p_path, String *r_base = NULL, String *r_icon_path = NULL);
static void get_global_class_list(List<StringName> *r_global_classes);
static void save_global_classes();

Expand Down
8 changes: 8 additions & 0 deletions editor/editor_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,14 @@ void EditorData::get_plugin_window_layout(Ref<ConfigFile> p_layout) {
}
}

bool EditorData::class_equals_or_inherits(const String &p_class, const String &p_inherits) {
if (p_class == p_inherits)
return true;
if (ScriptServer::is_global_class(p_class))
return script_class_is_parent(p_class, p_inherits);
return ClassDB::is_parent_class(p_class, p_inherits);
}

bool EditorData::script_class_is_parent(const String &p_class, const String &p_inherits) {
if (!ScriptServer::is_global_class(p_class))
return false;
Expand Down
1 change: 1 addition & 0 deletions editor/editor_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ class EditorData {
void notify_edited_scene_changed();
void notify_resource_saved(const Ref<Resource> &p_resource);

bool class_equals_or_inherits(const String &p_class, const String &p_inherits);
bool script_class_is_parent(const String &p_class, const String &p_inherits);
StringName script_class_get_base(const String &p_class) const;
Object *script_class_instance(const String &p_class);
Expand Down
34 changes: 28 additions & 6 deletions editor/editor_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2125,15 +2125,16 @@ void EditorPropertyResource::_file_selected(const String &p_path) {
if (!property_types.empty()) {
bool any_type_matches = false;
const Vector<String> split_property_types = property_types.split(",");
String res_class = _get_file_script_name_or_default(res);
for (int i = 0; i < split_property_types.size(); ++i) {
if (res->is_class(split_property_types[i])) {
if (EditorNode::get_editor_data().class_equals_or_inherits(res_class, split_property_types[i])) {
any_type_matches = true;
break;
}
}

if (!any_type_matches)
EditorNode::get_singleton()->show_warning(vformat(TTR("The selected resource (%s) does not match any type expected for this property (%s)."), res->get_class(), property_types));
EditorNode::get_singleton()->show_warning(vformat(TTR("The selected resource (%s) does not match any type expected for this property (%s)."), res_class, property_types));
}

emit_changed(get_edited_property(), res);
Expand All @@ -2153,6 +2154,9 @@ void EditorPropertyResource::_menu_option(int p_which) {
}
file->set_mode(EditorFileDialog::MODE_OPEN_FILE);
String type = base_type;
if (ScriptServer::is_global_class(type)) {
type = ScriptServer::get_global_class_native_base(type);
}

List<String> extensions;
for (int i = 0; i < type.get_slice_count(","); i++) {
Expand Down Expand Up @@ -2417,7 +2421,9 @@ void EditorPropertyResource::_update_menu_items() {
ClassDB::get_inheriters_from_class(base.strip_edges(), &inheritors);

for (int j = 0; j < custom_resources.size(); j++) {
inheritors.push_back(custom_resources[j].name);
if (EditorNode::get_editor_data().class_equals_or_inherits(custom_resources[j].name, base)) {
inheritors.push_back(custom_resources[j].name);
}
}

List<StringName>::Element *E = inheritors.front();
Expand Down Expand Up @@ -2701,7 +2707,7 @@ void EditorPropertyResource::update_property() {
assign->set_text(res->get_path().get_file());
assign->set_tooltip(res->get_path());
} else {
assign->set_text(res->get_class());
assign->set_text(_get_file_script_name_or_default(res));
}

if (res->get_path().is_resource_file()) {
Expand Down Expand Up @@ -2843,10 +2849,12 @@ bool EditorPropertyResource::_is_drop_valid(const Dictionary &p_drag_data) const
String ftype = EditorFileSystem::get_singleton()->get_file_type(file);

if (ftype != "") {
RES resource = ResourceLoader::load(file);
ftype = _get_file_script_name_or_default(resource);

for (int i = 0; i < allowed_type.get_slice_count(","); i++) {
String at = allowed_type.get_slice(",", i).strip_edges();
if (ClassDB::is_parent_class(ftype, at)) {
if (EditorNode::get_editor_data().class_equals_or_inherits(ftype, at)) {
return true;
}
}
Expand Down Expand Up @@ -2901,6 +2909,20 @@ void EditorPropertyResource::set_use_sub_inspector(bool p_enable) {
use_sub_inspector = p_enable;
}

String EditorPropertyResource::_get_file_script_name_or_default(const RES &p_resource) const {
Ref<Script> rscript = p_resource->get_script();
if (rscript.is_valid()) {
String rscript_path = rscript->get_path();
int script_index;
EditorFileSystemDirectory *fsdir = EditorFileSystem::get_singleton()->find_file(rscript_path, &script_index);
ERR_FAIL_COND_V(!fsdir, p_resource->get_class());
String file_script_name = fsdir->get_file_script_class_name(script_index);
if (!file_script_name.empty())
return file_script_name;
}
return p_resource->get_class();
}

void EditorPropertyResource::_bind_methods() {

ClassDB::bind_method(D_METHOD("_file_selected"), &EditorPropertyResource::_file_selected);
Expand Down Expand Up @@ -3375,8 +3397,8 @@ bool EditorInspectorDefaultPlugin::parse_property(Object *p_object, Variant::Typ
String type = open_in_new.get_slicec(',', i).strip_edges();
for (int j = 0; j < p_hint_text.get_slice_count(","); j++) {
String inherits = p_hint_text.get_slicec(',', j);
if (ClassDB::is_parent_class(inherits, type)) {

if (!ScriptServer::is_global_class(inherits) && ClassDB::is_parent_class(inherits, type)) {
editor->set_use_sub_inspector(false);
}
}
Expand Down
2 changes: 2 additions & 0 deletions editor/editor_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,8 @@ class EditorPropertyResource : public EditorProperty {
void _open_editor_pressed();
void _fold_other_editors(Object *p_self);

String _get_file_script_name_or_default(const RES &p_resource) const;

bool opened_editor;

protected:
Expand Down
77 changes: 73 additions & 4 deletions modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,9 @@ GDScriptParser::Node *GDScriptParser::_parse_expression(Node *p_parent, bool p_s
constant->value = scr;
expr = constant;
bfn = true;
} else {
_set_error("Cannot load " + identifier + " (" + ScriptServer::get_global_class_path(identifier) + "). Possible error or cyclic reference.");
return NULL;
}
}

Expand Down Expand Up @@ -4638,8 +4641,40 @@ void GDScriptParser::_parse_class(ClassNode *p_class) {
current_export.class_name = native_class->get_name();

} else {
current_export = PropertyInfo();
_set_error("The export hint isn't a resource type.");
Ref<Script> script = Object::cast_to<Script>(constant);
if (script.is_valid()) {
bool is_script_class = false;
for (int i = 0; i < ScriptServer::get_language_count() && !is_script_class; i++) {
ScriptLanguage *lang = ScriptServer::get_language(i);
String base;
String class_name = lang->get_global_class_name(script->get_path(), &base);
if (class_name.length()) {
is_script_class = true;
String to_test = base;
if (ScriptServer::is_global_class(base)) {
to_test = ScriptServer::get_global_class_native_base(base);
}
if (ClassDB::is_parent_class(to_test, "Resource")) {
current_export.type = Variant::OBJECT;
current_export.hint = PROPERTY_HINT_RESOURCE_TYPE;
current_export.usage |= PROPERTY_USAGE_SCRIPT_VARIABLE;

current_export.hint_string = class_name;
current_export.class_name = class_name;
} else {
current_export = PropertyInfo();
_set_error("The exported script class isn't a resource type.");
}
}
}
if (!is_script_class) {
current_export = PropertyInfo();
_set_error("The exported script isn't a script class.");
}
} else {
current_export = PropertyInfo();
_set_error("The export hint isn't a resource type.");
}
}
} else if (constant.get_type() == Variant::DICTIONARY) {
// Enumeration
Expand Down Expand Up @@ -4929,10 +4964,42 @@ void GDScriptParser::_parse_class(ClassNode *p_class) {
member._export.hint_string = member.data_type.native_type;
member._export.class_name = member.data_type.native_type;
} else {
_set_error("Invalid export type. Only built-in and native resource types can be exported.", member.line);
_set_error("Invalid native export type. \"" + member.data_type.native_type + "\" is not a Resource type.", member.line);
return;
}
} else if (member.data_type.kind == DataType::SCRIPT || member.data_type.kind == DataType::GDSCRIPT) {
if (member.data_type.script_type.is_null()) {
_set_error("The member's data type is a script that did not load successfully.", member.line);
return;
}
StringName class_name = ScriptServer::get_global_class_name(member.data_type.script_type->get_path());
if (class_name == StringName()) {
_set_error("Invalid script export type. \"" + class_name + "\" is not a script class.", member.line);
return;
}
if (ClassDB::is_parent_class(member.data_type.native_type, "Resource")) {
member._export.type = Variant::OBJECT;
member._export.hint = PROPERTY_HINT_RESOURCE_TYPE;
member._export.usage |= PROPERTY_USAGE_SCRIPT_VARIABLE;
member._export.hint_string = class_name;
member._export.class_name = class_name;
} else {
_set_error("Invalid script class export type. \"" + class_name + "\" is not a Resource type.", member.line);
return;
}

} else if (member.data_type.kind == DataType::UNRESOLVED && ScriptServer::is_global_class(member.data_type.native_type)) {
String class_name = member.data_type.native_type;
if (ScriptServer::get_global_class_native_base(class_name) == "Resource") {
Comment on lines +4991 to +4993
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some confusion about these lines.

  1. If member.data_type.native_type is a StringName containing the native class type, then why would it ever become a script class name? I'm not sure of what the intent here is. I was looking in the source for where native_type gets assigned a value, but I couldn't seem to find anything in gdscript_parser.h/.cpp, so I'm not sure how a script class name is supposed to end up there. I did see that parse_class_name() directly assigns the parsed identifier to current_class->identifier, so that is likely what you would want to pass to the ScriptServer, unless I'm mistaken.

  2. Based on the above point, I think you should gather the class_name variable's value from something different. As far as I can tell with how it is used, native_type is supposed to be an actual C++ engine class.

  3. I don't think you can simply check if the native type of the class is "Resource". After all, someone might choose to extend a different class other than Resource and make a script class out of that type. So, you should first grab the native type, and then compare it to the ClassDB in full.

     if (ClassDB::is_parent_class(native_type, "Resource")) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. In 3.2 stable the parser sets the native_type to the identifier's string here:
    https://github.com/godotengine/godot/blob/3.2/modules/gdscript/gdscript_parser.cpp#L5723-L5731

I agree it doesn't make much sense semantically, but I just took the information I had available, I didn't change any of the type parsing code.

  1. Totally agree, I will update the PR soon.

member._export.type = Variant::OBJECT;
member._export.hint = PROPERTY_HINT_RESOURCE_TYPE;
member._export.usage |= PROPERTY_USAGE_SCRIPT_VARIABLE;
member._export.hint_string = class_name;
member._export.class_name = class_name;
} else {
_set_error("Invalid script class export type. \"" + class_name + "\" is not a Resource type.", member.line);
return;
}
} else {
_set_error("Invalid export type. Only built-in and native resource types can be exported.", member.line);
return;
Expand Down Expand Up @@ -6255,7 +6322,9 @@ bool GDScriptParser::_is_type_compatible(const DataType &p_container, const Data
switch (p_expression.kind) {
case DataType::NATIVE: {
if (p_container.kind != DataType::NATIVE) {
// Non-native type can't be a superclass of a native type
if (p_container.kind == DataType::GDSCRIPT || p_container.kind == DataType::SCRIPT) {
return ScriptServer::get_global_class_name(p_container.script_type->get_path()) == p_expression.native_type;
}
Comment on lines +6325 to +6327
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Wouldn't this check need to be made the other way around? Here, you are checking to see if the container is a script and the expression is a native class, e.g. var my_node: MyNode = Node.new(). However, the scripted type would have to extend the native type, so you would be assigning a base type to a derived type member. But that would break polymorphism, allowing you to assign incompatible values to variables.

  2. Even if you move it, you still need to get what the base type is and then query the ClassDB for inheritance with is_parent_class() rather than making a direct comparison to the base type. After all, the script could extend a class deriving the member's expected type, e.g.

     var a_control: Control = MyTextureRect.new()

    where MyTextureRect is a script extending TextureRect rather than Control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this code doesn't seem to be right... but the whole type checking in 3.2 GDscript is a big mess. The type checking goes through so many code paths it's hard to keep track of everything.

I did some tests and my changes don't really affect any of the cases you mentioned (I still think my code is incorrect, but it doesn't really make things worse in these cases). Comparing This PR and 3.2.2 stable I get the exact same behavior in this test script:
gdscript_errors

So... I'm not really sure what is the best course of action here. I hope with the cleanup of GDscript 2.0 we get a type checker that is much easier to maintain because the 3.2 one is a bit of a nightmare.

return false;
}
if (p_expression.is_meta_type) {
Expand Down