Fix properties being lost when reloading placeholder GDScript instance

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).

core/script_language.cpp

@@ -376,7 +376,7 @@ ScriptDebugger::ScriptDebugger() {
 
 bool PlaceHolderScriptInstance::set(const StringName &p_name, const Variant &p_value) {
 
-	if (build_failed)
+	if (script->is_placeholder_fallback_enabled())
 		return false;
 
 	if (values.has(p_name)) {
@@ -407,7 +407,7 @@ bool PlaceHolderScriptInstance::get(const StringName &p_name, Variant &r_ret) co
 		return true;
 	}
 
-	if (!build_failed) {
+	if (!script->is_placeholder_fallback_enabled()) {
 		Variant defval;
 		if (script->get_property_default_value(p_name, defval)) {
 			r_ret = defval;
@@ -420,7 +420,7 @@ bool PlaceHolderScriptInstance::get(const StringName &p_name, Variant &r_ret) co
 
 void PlaceHolderScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const {
 
-	if (build_failed) {
+	if (script->is_placeholder_fallback_enabled()) {
 		for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
 			p_properties->push_back(E->get());
 		}
@@ -450,7 +450,7 @@ Variant::Type PlaceHolderScriptInstance::get_property_type(const StringName &p_n
 
 void PlaceHolderScriptInstance::get_method_list(List<MethodInfo> *p_list) const {
 
-	if (build_failed)
+	if (script->is_placeholder_fallback_enabled())
 		return;
 
 	if (script.is_valid()) {
@@ -459,7 +459,7 @@ void PlaceHolderScriptInstance::get_method_list(List<MethodInfo> *p_list) const
 }
 bool PlaceHolderScriptInstance::has_method(const StringName &p_method) const {
 
-	if (build_failed)
+	if (script->is_placeholder_fallback_enabled())
 		return false;
 
 	if (script.is_valid()) {
@@ -470,8 +470,6 @@ bool PlaceHolderScriptInstance::has_method(const StringName &p_method) const {
 
 void PlaceHolderScriptInstance::update(const List<PropertyInfo> &p_properties, const Map<StringName, Variant> &p_values) {
 
-	build_failed = false;
-
 	Set<StringName> new_values;
 	for (const List<PropertyInfo>::Element *E = p_properties.front(); E; E = E->next()) {
 
@@ -517,7 +515,7 @@ void PlaceHolderScriptInstance::update(const List<PropertyInfo> &p_properties, c
 
 void PlaceHolderScriptInstance::property_set_fallback(const StringName &p_name, const Variant &p_value, bool *r_valid) {
 
-	if (build_failed) {
+	if (script->is_placeholder_fallback_enabled()) {
 		Map<StringName, Variant>::Element *E = values.find(p_name);
 
 		if (E) {
@@ -544,7 +542,7 @@ void PlaceHolderScriptInstance::property_set_fallback(const StringName &p_name,
 
 Variant PlaceHolderScriptInstance::property_get_fallback(const StringName &p_name, bool *r_valid) {
 
-	if (build_failed) {
+	if (script->is_placeholder_fallback_enabled()) {
 		const Map<StringName, Variant>::Element *E = values.find(p_name);
 
 		if (E) {
@@ -563,8 +561,7 @@ Variant PlaceHolderScriptInstance::property_get_fallback(const StringName &p_nam
 PlaceHolderScriptInstance::PlaceHolderScriptInstance(ScriptLanguage *p_language, Ref<Script> p_script, Object *p_owner) :
 		owner(p_owner),
 		language(p_language),
-		script(p_script),
-		build_failed(false) {
+		script(p_script) {
 }
 
 PlaceHolderScriptInstance::~PlaceHolderScriptInstance() {

core/script_language.h

@@ -146,6 +146,8 @@ public:
 	virtual void get_constants(Map<StringName, Variant> *p_constants) {}
 	virtual void get_members(Set<StringName> *p_constants) {}
 
+	virtual bool is_placeholder_fallback_enabled() const { return false; }
+
 	Script() {}
 };
 
@@ -334,8 +336,6 @@ class PlaceHolderScriptInstance : public ScriptInstance {
 	ScriptLanguage *language;
 	Ref<Script> script;
 
-	bool build_failed;
-
 public:
 	virtual bool set(const StringName &p_name, const Variant &p_value);
 	virtual bool get(const StringName &p_name, Variant &r_ret) const;
@@ -361,13 +361,10 @@ public:
 
 	void update(const List<PropertyInfo> &p_properties, const Map<StringName, Variant> &p_values); //likely changed in editor
 
-	void set_build_failed(bool p_build_failed) { build_failed = p_build_failed; }
-	bool get_build_failed() const { return build_failed; }
-
 	virtual bool is_placeholder() const { return true; }
 
-	virtual void property_set_fallback(const StringName &p_name, const Variant &p_value, bool *r_valid);
-	virtual Variant property_get_fallback(const StringName &p_name, bool *r_valid);
+	virtual void property_set_fallback(const StringName &p_name, const Variant &p_value, bool *r_valid = NULL);
+	virtual Variant property_get_fallback(const StringName &p_name, bool *r_valid = NULL);
 
 	virtual MultiplayerAPI::RPCMode get_rpc_mode(const StringName &p_method) const { return MultiplayerAPI::RPC_MODE_DISABLED; }
 	virtual MultiplayerAPI::RPCMode get_rset_mode(const StringName &p_variable) const { return MultiplayerAPI::RPC_MODE_DISABLED; }

modules/gdscript/gdscript.cpp

@@ -475,20 +475,15 @@ bool GDScript::_update_exports() {
 				_signals[c->_signals[i].name] = c->_signals[i].arguments;
 			}
 		} else {
-			for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
-				E->get()->set_build_failed(true);
-			}
-			return false;
-		}
-	} else {
-		if (!valid) {
-			for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
-				E->get()->set_build_failed(true);
-			}
+			placeholder_fallback_enabled = true;
 			return false;
 		}
+	} else if (!valid || placeholder_fallback_enabled) {
+		return false;
 	}
 
+	placeholder_fallback_enabled = false;
+
 	if (base_cache.is_valid()) {
 		if (base_cache->_update_exports()) {
 			changed = true;
@@ -503,7 +498,6 @@ bool GDScript::_update_exports() {
 		_update_exports_values(values, propnames);
 
 		for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
-			E->get()->set_build_failed(false);
 			E->get()->update(propnames, values);
 		}
 	}
@@ -907,6 +901,7 @@ GDScript::GDScript() :
 	tool = false;
 #ifdef TOOLS_ENABLED
 	source_changed_cache = false;
+	placeholder_fallback_enabled = false;
 #endif
 
 #ifdef DEBUG_ENABLED
@@ -1675,6 +1670,8 @@ void GDScriptLanguage::reload_tool_script(const Ref<Script> &p_script, bool p_so
 		//restore state if saved
 		for (Map<ObjectID, List<Pair<StringName, Variant> > >::Element *F = E->get().front(); F; F = F->next()) {
 
+			List<Pair<StringName, Variant> > &saved_state = F->get();
+
 			Object *obj = ObjectDB::get_instance(F->key());
 			if (!obj)
 				continue;
@@ -1684,16 +1681,26 @@ void GDScriptLanguage::reload_tool_script(const Ref<Script> &p_script, bool p_so
 				obj->set_script(RefPtr());
 			}
 			obj->set_script(scr.get_ref_ptr());
-			if (!obj->get_script_instance()) {
+
+			ScriptInstance *script_instance = obj->get_script_instance();
+
+			if (!script_instance) {
 				//failed, save reload state for next time if not saved
 				if (!scr->pending_reload_state.has(obj->get_instance_id())) {
-					scr->pending_reload_state[obj->get_instance_id()] = F->get();
+					scr->pending_reload_state[obj->get_instance_id()] = saved_state;
 				}
 				continue;
 			}
 
-			for (List<Pair<StringName, Variant> >::Element *G = F->get().front(); G; G = G->next()) {
-				obj->get_script_instance()->set(G->get().first, G->get().second);
+			if (script_instance->is_placeholder() && scr->is_placeholder_fallback_enabled()) {
+				PlaceHolderScriptInstance *placeholder = static_cast<PlaceHolderScriptInstance *>(script_instance);
+				for (List<Pair<StringName, Variant> >::Element *G = saved_state.front(); G; G = G->next()) {
+					placeholder->property_set_fallback(G->get().first, G->get().second);
+				}
+			} else {
+				for (List<Pair<StringName, Variant> >::Element *G = saved_state.front(); G; G = G->next()) {
+					script_instance->set(G->get().first, G->get().second);
+				}
 			}
 
 			scr->pending_reload_state.erase(obj->get_instance_id()); //as it reloaded, remove pending state

modules/gdscript/gdscript.h

@@ -97,6 +97,7 @@ class GDScript : public Script {
 	Ref<GDScript> base_cache;
 	Set<ObjectID> inheriters_cache;
 	bool source_changed_cache;
+	bool placeholder_fallback_enabled;
 	void _update_exports_values(Map<StringName, Variant> &values, List<PropertyInfo> &propnames);
 
 #endif
@@ -209,6 +210,10 @@ public:
 	virtual void get_constants(Map<StringName, Variant> *p_constants);
 	virtual void get_members(Set<StringName> *p_members);
 
+#ifdef TOOLS_ENABLED
+	virtual bool is_placeholder_fallback_enabled() const { return placeholder_fallback_enabled; }
+#endif
+
 	GDScript();
 	~GDScript();
 };

modules/mono/csharp_script.cpp

@@ -783,7 +783,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 
 				// Even though build didn't fail, this tells the placeholder to keep properties and
 				// it allows using property_set_fallback for restoring the state without a valid script.
-				placeholder->set_build_failed(true);
+				scr->placeholder_fallback_enabled = true;
 
 				// Restore Variant properties state, it will be kept by the placeholder until the next script reloading
 				for (List<Pair<StringName, Variant> >::Element *G = scr->pending_reload_state[obj_id].properties.front(); G; G = G->next()) {
@@ -1830,12 +1830,10 @@ void CSharpScript::_update_exports_values(Map<StringName, Variant> &values, List
 bool CSharpScript::_update_exports() {
 
 #ifdef TOOLS_ENABLED
-	if (!valid) {
-		for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
-			E->get()->set_build_failed(true);
-		}
+	placeholder_fallback_enabled = true; // until proven otherwise
+
+	if (!valid)
 		return false;
-	}
 
 	bool changed = false;
 
@@ -1944,6 +1942,8 @@ bool CSharpScript::_update_exports() {
 		tmp_object = NULL;
 	}
 
+	placeholder_fallback_enabled = false;
+
 	if (placeholders.size()) {
 		// Update placeholders if any
 		Map<StringName, Variant> values;
@@ -1951,7 +1951,6 @@ bool CSharpScript::_update_exports() {
 		_update_exports_values(values, propnames);
 
 		for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
-			E->get()->set_build_failed(false);
 			E->get()->update(propnames, values);
 		}
 	}
@@ -2666,6 +2665,7 @@ CSharpScript::CSharpScript() :
 
 #ifdef TOOLS_ENABLED
 	source_changed_cache = false;
+	placeholder_fallback_enabled = false;
 	exports_invalidated = true;
 #endif
 

modules/mono/csharp_script.h

@@ -115,6 +115,7 @@ class CSharpScript : public Script {
 	Map<StringName, Variant> exported_members_defval_cache; // member_default_values_cache
 	Set<PlaceHolderScriptInstance *> placeholders;
 	bool source_changed_cache;
+	bool placeholder_fallback_enabled;
 	bool exports_invalidated;
 	void _update_exports_values(Map<StringName, Variant> &values, List<PropertyInfo> &propnames);
 	virtual void _placeholder_erased(PlaceHolderScriptInstance *p_placeholder);
@@ -180,6 +181,10 @@ public:
 
 	virtual int get_member_line(const StringName &p_member) const;
 
+#ifdef TOOLS_ENABLED
+	virtual bool is_placeholder_fallback_enabled() const { return placeholder_fallback_enabled; }
+#endif
+
 	Error load_source_code(const String &p_path);
 
 	StringName get_script_name() const;