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 an error when dragging nodes into built-in scripts because script does not inherit Node #81299

Conversation

jsjtxietian
Copy link
Contributor

Fixes #81294
Reload built_in script after create to allow drag nodes into it. Calling reload can compile the script and give it the inheritance info, thus can pass the check in drop_data_fw.

@akien-mga
Copy link
Member

Reload built_in script after create to allow drag nodes into it. Calling reload can compile the script and give it the inheritance info, thus can pass the check in drop_data_fw.

This probably warrants a comment in the code, as it's not obvious why a script that has just been created would need to be reloaded. I also wonder if it's the right location for it or if it should be done after receiving script_created?

@akien-mga akien-mga requested a review from KoBeWi September 4, 2023 13:48
@jsjtxietian
Copy link
Contributor Author

I also wonder if it's the right location for it or if it should be done after receiving script_created

As far as I can see, for no built_in scripts, it will get reloaded and compiled in Error err = ResourceSaver::save(scr, lpath, ResourceSaver::FLAG_CHANGE_PATH), rightly after it is created. But there might be other reasons why we can't do the same for built_in scripts that I do not know.

@jsjtxietian jsjtxietian force-pushed the Reload-built_in-script-after-create-to-allow-drag-nodes-into-it branch from fbe01d6 to 80d3a2a Compare September 5, 2023 04:40
@jsjtxietian jsjtxietian force-pushed the Reload-built_in-script-after-create-to-allow-drag-nodes-into-it branch from 80d3a2a to 1d4201e Compare September 5, 2023 08:34
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The comment might need rewording (not sure about template; also instead of specifically referring to dropping nodes I think it should say that it makes type recognizable or something), but I can confirm this change fixes the issue.

I checked when external script are reloaded and it happens on save as you noted, but only when auto_reload_and_parse_scripts_on_save editor setting is enabled. Otherwise the script will be reloaded when generating preview 🙃 (which also happens right after saving)

So I think this is a correct place to reload the script. Although it's weird that it isn't done automatically somewhere (in script editor maybe).

@jsjtxietian
Copy link
Contributor Author

The comment might need rewording.

I will update it soon.

@jsjtxietian jsjtxietian force-pushed the Reload-built_in-script-after-create-to-allow-drag-nodes-into-it branch from 1d4201e to d32348c Compare September 6, 2023 02:04
@YuriSizov YuriSizov merged commit d2cc689 into godotengine:master Sep 6, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@jsjtxietian jsjtxietian deleted the Reload-built_in-script-after-create-to-allow-drag-nodes-into-it branch September 6, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants