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

Fix gdscript analyzer error when instantiating EditorPlugins. #74025

Closed
wants to merge 1 commit into from

Conversation

baptr
Copy link
Contributor

@baptr baptr commented Feb 27, 2023

Editor code is not instantiable outside of the editor (

if (ti->api == API_EDITOR && !Engine::get_singleton()->is_editor_hint()) {
return false;
). This is fine for editor plugins and the like, but the GDScript analyzer balks at it, causing F5 runs to fail: #73525.

Instead, we really just want to know if the type is abstract - so add a new ClassDB method to check that and nothing else.

Closes #73525

Editor code is not instantiable outside of the editor
(https://github.com/godotengine/godot/blob/1d14c054a12dacdc193b589e4afb0ef319ee2aae/core/object/class_db.cpp#L369).
This is fine for editor plugins and the like, but the GDScript analyzer
balks at it, causing F5 runs to fail: godotengine#73525.

Instead, we really just want to know if the type is abstract - so add
a new ClassDB method to check that and nothing else.
}
return false;
}
return (ti->creation_func == nullptr && (!ti->gdextension || ti->gdextension->create_instance == nullptr));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also test ti->disabled like in can_instantiate()?

Copy link
Member

Choose a reason for hiding this comment

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

Probably, if a class is disabled, it does not cease to be abstract. That is, whether the class is enabled is not important for is_abstract(), but it is important for can_instantiate().

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@YuriSizov YuriSizov requested a review from a team June 26, 2023 12:54
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Oct 30, 2023
@MikeSchulze
Copy link

MikeSchulze commented Jul 3, 2024

I build Godot rebased on master and run the example project

EditorSpinSlider.zip

The parse error Parser Error: Native class "EditorSpinSlider" cannot be constructed as it is abstract. is gone but the components still not usable in a testing scene as described in #93854.

It produces errors
image

For my unit testing, this PR unblocks running my tests
image

So the parse error is fixed by this PR 👍

@MikeSchulze
Copy link

@vnen @YuriSizov this issue is resolved and fixes the parsing error.
If there are any steps I need to do to bring this pr ready to approve and merged into 4.3.x ?

@dalexeev dalexeev requested a review from a team July 4, 2024 07:47
Comment on lines +237 to 239
static bool is_abstract(const StringName &p_class);
static bool can_instantiate(const StringName &p_class);
static bool is_virtual(const StringName &p_class);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static bool is_abstract(const StringName &p_class);
static bool can_instantiate(const StringName &p_class);
static bool is_virtual(const StringName &p_class);
static bool can_instantiate(const StringName &p_class);
static bool is_abstract(const StringName &p_class);
static bool is_virtual(const StringName &p_class);

It's a nitpick, but I'd rather have is_abstract() and is_virtual() next to each other. This also matches the order in .cpp.

}
return false;
}
return (ti->creation_func == nullptr && (!ti->gdextension || ti->gdextension->create_instance == nullptr));
Copy link
Member

Choose a reason for hiding this comment

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

Probably, if a class is disabled, it does not cease to be abstract. That is, whether the class is enabled is not important for is_abstract(), but it is important for can_instantiate().

}
return false;
}
return (ti->creation_func == nullptr && (!ti->gdextension || ti->gdextension->create_instance == nullptr));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return (ti->creation_func == nullptr && (!ti->gdextension || ti->gdextension->create_instance == nullptr));
return ti->creation_func == nullptr && (!ti->gdextension || ti->gdextension->create_instance == nullptr);

if (!ScriptServer::is_global_class(p_class)) {
ERR_FAIL_V_MSG(false, "Cannot get class '" + String(p_class) + "'.");
}
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return false;
String path = ScriptServer::get_global_class_path(p_class);
Ref<Script> scr = ResourceLoader::load(path);
return scr.is_valid() && scr->is_valid() && scr->is_abstract();

Not sure about this, just copied from ClassDB::is_virtual().

@MikeSchulze
Copy link

@dalexeev i can't push to the original forked repo, should i create a new pr?

@AThousandShips
Copy link
Member

@baptr are you able/interested in continuing working on this?

@akien-mga
Copy link
Member

Superseded by #93942. Thanks for the contribution!

@akien-mga akien-mga closed this Jul 11, 2024
@akien-mga akien-mga removed this from the 4.3 milestone Jul 11, 2024
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.

EditorExportPlugin raises error everytime you run the game with F5 because of "abstract native class"
7 participants