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

Fix properties being lost when reloading placeholder GDScript instance #24877

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Fix properties being lost when reloading placeholder GDScript instance #24877

merged 1 commit into from
Jan 10, 2019

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Jan 10, 2019

Fixes #24280
Supersedes #24707

During reloading in GDScriptLanguage::reload_all_scripts a placeholder instance that must remain so is replaced with a new placeholder instance. The state is then restored by calling ScriptInstance::set for each property. This does not work if the script is missing the properties due to build/parse failing.
The fix for such cases is to call placeholder_set_fallback instead of set on the script instance.
(Another solution could be to call Object::set instead, which already deals with this problem. However, I'm not sure if that could cause any regression or if it would add too much overhead.)

The reason the same bug was not happening with C# is because there we don't replace placeholder instances with new placeholder instances during reloading, we just keep the existing one. I'm not familiar enough with the GDScript module to understand why it does not do the same.

I took this chance to move the build_failed flag from PlaceHolderScriptInstance to Script. That improves the code a lot. I also renamed it to placeholder_fallback_enabled which is a much better name (build_failed could lead to misunderstandings).

@neikeq
Fix properties being lost when reloading placeholder GDScript instance
ea85ff0 
During reloading in `GDScriptLanguage::reload_all_scripts` a placeholder instance that must remain so is replaced with a new placeholder instance. The state is then restored by calling `ScriptInstance::set` for each property. This does not work if the script is missing the properties due to build/parse failing.
The fix for such cases is to call `placeholder_set_fallback` instead of `set` on the script instance.

I took this chance to move the `build_failed` flag from `PlaceHolderScriptInstance` to `Script`. That improves the code a lot. I also renamed it to `placeholder_fallback_enabled` which is a much better name (`build_failed` could lead to misunderstandings).
@neikeq neikeq added this to the 3.1 milestone Jan 10, 2019
@neikeq neikeq mentioned this pull request Jan 10, 2019
Closed
@N0hbdy
Copy link
Contributor

N0hbdy commented Jan 10, 2019

FYI - you removed all reviewers? Not sure if you meant to do that?

@akien-mga akien-mga requested review from reduz and vnen January 10, 2019 11:11
@reduz
Copy link
Member

reduz commented Jan 10, 2019

I really think build_failed is more explicit and to the point as a name.

@QbieShay
Copy link
Contributor

Tested on my reproduction case - it works! Thank you a lot @neikeq !

@realkotob
Copy link
Contributor

realkotob commented Jan 10, 2019 •
edited
Loading

I think we could keep the name to build_failed for this PR and rename it later if needed -- this fix is urgently needed.

@neikeq
Copy link
Contributor Author

neikeq commented Jan 10, 2019

The name build_failed is bad because this doesn't necessarily mean a build failed. All this says is the script is in a state in which it cannot provide information about the exported properties, so the placeholder must rely on a fallback instead. Hence the name placeholder_fallback_enabled. It may not be the best name, but I can't think of anything better.

For example, in the case of GDScript, what is the difference between valid and build_failed? They seem to be the same thing, but they are not.

@akien-mga akien-mga merged commit 6582968 into godotengine:master Jan 10, 2019
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Jan 19, 2019
Closed
@MidZik MidZik mentioned this pull request Feb 13, 2019
Closed
@neikeq neikeq mentioned this pull request Jul 3, 2019
Merged
@neikeq neikeq deleted the issue-24280 branch May 14, 2021 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reduz reduz Awaiting requested review from reduz

@vnen vnen Awaiting requested review from vnen

Assignees
No one assigned
Labels
bug enhancement topic:core topic:gdscript
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.

Loss of export variable when a script does not compile and then compiles again
6 participants
@neikeq @N0hbdy @reduz @QbieShay @realkotob @akien-mga