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

Always soft-reload scripts #55134

Merged
merged 1 commit into from
May 17, 2022

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 19, 2021

See #26326 (comment) for context.

This PR makes all scripts soft-reload by default. This prevents script variables from being cleared upon save etc. I didn't find any drawbacks of using soft-reloading. The only case where hard-reload is stil used is when running EditorScript.

The "Soft Reload Script" menu option was renamed to "Soft Reload Tool Script" and only works on tool scripts (technically it always only worked on tool scripts, but now there is a proper error message). I left it out, because apparently we have an editor setting for auto-reloading scripts on save, which can be disabled. If we ever remove this setting, soft-reload menu option will become unnecessary and could be removed too.

Also I considered adding menu option for hard-reloading, but apparently it requires that no script instance exists and the option almost always fails when used. There is a reload_tool_script method that could be used, but only GDScript has it.

Resolves #26326
Resolves godotengine/godot-proposals#1659 (fixes the original problem)
Fixes #5969
Closes #6276 (last issue)

2 more related proposals, but not sure if it resolves them:
godotengine/godot-proposals#1012
godotengine/godot-proposals#2365

@mhilbrunner
Copy link
Member

cc @vnen @neikeq This needs your review :)

@neikeq
Copy link
Contributor

neikeq commented Dec 6, 2021

As far as I remember from a discussion with @reduz long ago, both hard reload and soft reload should keep state; with the only difference being hard reload destroys and recreates instances restoring their state, meanwhile soft reload keeps the existing instances.
I may be wrong about that description but if it's indeed true, it would suggest the issues this PR tries to fix are a bug in hard reload rather than the use of the wrong reload mode.
In any case, C# always reloads by recreating instances and restoring their state (what I described as hard reload above), regardless of what flag is passes, so this PR doesn't affect it.
I can't comment on GDScript and other languages. I think @reduz should give this a quick review.

@neikeq neikeq requested a review from reduz December 6, 2021 12:47
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 6, 2021

What is the purpose of destroying script instances? The behavior of hard-reload is weird, seems like you actually need to remove the instances yourself, otherwise the reloading will fail, because the script is in use. Also, state preserving is conditionally disabled for hard-reload, so it looks intended. If this is really a bug, then it's at least 5 years old 🙃

btw when discussed during meeting, reduz's concern was that soft-reload will keep variable list, so if you add a new variable or delete one, it won't work properly. I tested this and the script will detect new variables properly, but they will have null value, which is somewhat expected. Soft-reloading is about keeping values of variables.

@neikeq
Copy link
Contributor

neikeq commented Dec 6, 2021

What is the purpose of destroying script instances? ...

For GDScript, I don't know. For C#, it's the only way.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved in PR review meeting, we're not 100% sure that hard reload is no longer useful (aside from C# needs where it's hardcoded - this PR only impacts the builtin script editor), but it makes sense at least to default to soft reload. We'll see if anyone comes up with a use case for reintroducing hard reload options.

@akien-mga akien-mga merged commit 5b02415 into godotengine:master May 17, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the script_pillow_or_something branch May 17, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants