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 plugin script classes defined even if inactive. #32434

Merged

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Sep 30, 2019

Currently, script classes will always be registered no matter where they are defined in a project. This change correctly filters out scripts from being registered if they are in a plugin's subdirectory and if that plugin is inactive.

@aaronfranke
Copy link
Member

Related: #31401 (comment)

@willnationsdev
Copy link
Contributor Author

This effectively implements that comment @aaronfranke. It's the EditorFileSystem changes that filter plugin-based script classes out of the registration process (which is what everything else, including CreateDialog, bases its script class awareness off of).

@@ -1411,6 +1411,9 @@ String EditorFileSystem::_get_global_script_class(const String &p_type, const St
}

void EditorFileSystem::_scan_script_classes(EditorFileSystemDirectory *p_dir) {
if (p_dir->parent && p_dir->parent->name == "addons" && EditorNode::get_singleton()->is_addon_plugin_enabled(p_dir->name)) {
Copy link
Member

@akien-mga akien-mga Sep 30, 2019

Choose a reason for hiding this comment

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

You said you're filtering out inactive plugins, but doesn't this filter out enabled plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga Whoops. You're right. It was previously two separate if statements (when I tested it), and when I refactored it to one I forgot the exclamation point. Nice catch. Fixed now.

@akien-mga akien-mga added this to the 3.2 milestone Sep 30, 2019
@samdze
Copy link
Contributor

samdze commented Sep 30, 2019

Could be a starting point for https://github.com/godotengine/godot/issues/19178#issuecomment-471099148 ?

@akien-mga akien-mga merged commit 680bcb1 into godotengine:master Sep 30, 2019
@akien-mga
Copy link
Member

Thanks!

@Chaosus
Copy link
Member

Chaosus commented Oct 2, 2019

@willnationsdev This PR prevented VisualShaderNodeCustom's to be loaded from addon subdirs. Can you make a fix PR ?

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 2, 2019

@Chaosus I'm confused. Do they use the script class system? And shouldn't they be blocked from being used anyway unless their relevant addon has been enabled?

Edit: the only change I made was in the _scan_script_classes code. It then sets up data about what script classes exist. If it isn't a Script and it isn't affected by script class existence, then I don't know why it is reliant on that code.

@Chaosus
Copy link
Member

Chaosus commented Oct 2, 2019

They are not connected to plugin system but registered through class_names.

@willnationsdev
Copy link
Contributor Author

@Chaosus So, what would be the best fix for this then? Should I just add an exception to a specific type of resource?

@Chaosus
Copy link
Member

Chaosus commented Oct 2, 2019

I dont know - they are used standart *.gd extension so need to check if they are derived from VisualShaderNodeCustom anyways.

@willnationsdev
Copy link
Contributor Author

@Chaosus Ahh, ok. So it could be any type of script, so long as the base native class is VisualShaderNodeCustom.

@Chaosus
Copy link
Member

Chaosus commented Oct 2, 2019

Yeah

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 2, 2019

@akien-mga I would just revert this commit for now. I'm trying to break down a good "fix" for this, except that you either do or don't want an addon's script classes to be registered by Godot. And if the addon doesn't provide a plugin, then there is no interface in the ProjectSettings by which to mark the addon as active/inactive (because it marks plugins, not addons).

Better to let them all through for now until we update the GUI to be able to toggle script classes contained in addons that don't have plugins.

@aaronfranke
Copy link
Member

aaronfranke commented Oct 2, 2019

Might also make sense to put these kinds of changes on the 4.0 milestone and do them alongside an overhaul to the addon/script class system as discussed in godotengine/godot-proposals#22

@akien-mga
Copy link
Member

Alright, reverted as discussed above.

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.

None yet

5 participants