From f3c7527225c73812a7de10dfc273ba0e82e8d7ec Mon Sep 17 00:00:00 2001
From: Ignacio Etcheverry <ignalfonsore@gmail.com>
Date: Sun, 29 Jul 2018 22:40:09 +0200
Subject: [PATCH] Fix case where exported properties value is lost

Fixes exported property modified values lost when creating a placeholder script instance with a failed script compilation

- Object set/get will call PlaceHolderScriptInstance's new fallback set/get methods as a last resort. This way, placeholder script instances can keep the values for storage or until the script is compiled successfuly.
- Script::can_instance() will only return true if a real script instance can be created. Otherwise, in the case of placeholder script instances, it will return false.
- Object::set_script(script) is now in charge of requesting the creation of placeholder script instances. It's no longer Script::instance_create(owner)'s duty.
- PlaceHolderScriptInstance has a new method set_build_failed(bool) to determine whether it should call into its script methods or not.
- Fixed a few problems during reloading of C# scripts.
---
 core/object.cpp                | 73 ++++++++++++++++++++++----
 core/script_language.cpp       | 94 ++++++++++++++++++++++++++++++----
 core/script_language.h         | 12 +++++
 modules/gdscript/gdscript.cpp  | 54 ++++++++++---------
 modules/gdscript/gdscript.h    |  1 +
 modules/mono/csharp_script.cpp | 75 +++++++++++++++++++++------
 modules/mono/csharp_script.h   |  1 +
 7 files changed, 253 insertions(+), 57 deletions(-)

diff --git a/core/object.cpp b/core/object.cpp
index 8c9d3557f82..fd56374c667 100644
--- a/core/object.cpp
+++ b/core/object.cpp
@@ -450,16 +450,41 @@ void Object::set(const StringName &p_name, const Variant &p_value, bool *r_valid
 			*r_valid = true;
 		return;
 #endif
-	} else {
-		//something inside the object... :|
-		bool success = _setv(p_name, p_value);
-		if (success) {
+	}
+
+	//something inside the object... :|
+	bool success = _setv(p_name, p_value);
+	if (success) {
+		if (r_valid)
+			*r_valid = true;
+		return;
+	}
+
+	{
+		bool valid;
+		setvar(p_name, p_value, &valid);
+		if (valid) {
 			if (r_valid)
 				*r_valid = true;
 			return;
 		}
-		setvar(p_name, p_value, r_valid);
 	}
+
+#ifdef TOOLS_ENABLED
+	if (script_instance) {
+		bool valid;
+		script_instance->property_set_fallback(p_name, p_value, &valid);
+		if (valid) {
+			if (r_valid)
+				*r_valid = true;
+			return;
+		}
+	}
+#endif
+
+	if (r_valid)
+		*r_valid = false;
+	return;
 }
 
 Variant Object::get(const StringName &p_name, bool *r_valid) const {
@@ -513,8 +538,33 @@ Variant Object::get(const StringName &p_name, bool *r_valid) const {
 				*r_valid = true;
 			return ret;
 		}
+
 		//if nothing else, use getvar
-		return getvar(p_name, r_valid);
+		{
+			bool valid;
+			ret = getvar(p_name, &valid);
+			if (valid) {
+				if (r_valid)
+					*r_valid = true;
+				return ret;
+			}
+		}
+
+#ifdef TOOLS_ENABLED
+		if (script_instance) {
+			bool valid;
+			ret = script_instance->property_get_fallback(p_name, &valid);
+			if (valid) {
+				if (r_valid)
+					*r_valid = true;
+				return ret;
+			}
+		}
+#endif
+
+		if (r_valid)
+			*r_valid = false;
+		return Variant();
 	}
 }
 
@@ -979,9 +1029,14 @@ void Object::set_script(const RefPtr &p_script) {
 	script = p_script;
 	Ref<Script> s(script);
 
-	if (!s.is_null() && s->can_instance()) {
-		OBJ_DEBUG_LOCK
-		script_instance = s->instance_create(this);
+	if (!s.is_null()) {
+		if (s->can_instance()) {
+			OBJ_DEBUG_LOCK
+			script_instance = s->instance_create(this);
+		} else if (Engine::get_singleton()->is_editor_hint()) {
+			OBJ_DEBUG_LOCK
+			script_instance = s->placeholder_instance_create(this);
+		}
 	}
 
 	_change_notify("script");
diff --git a/core/script_language.cpp b/core/script_language.cpp
index 37ba3cfc62b..e146fb773c3 100644
--- a/core/script_language.cpp
+++ b/core/script_language.cpp
@@ -255,6 +255,17 @@ void ScriptInstance::call_multilevel_reversed(const StringName &p_method, const
 	call(p_method, p_args, p_argcount, ce); // script may not support multilevel calls
 }
 
+void ScriptInstance::property_set_fallback(const StringName &, const Variant &, bool *r_valid) {
+	if (r_valid)
+		*r_valid = false;
+}
+
+Variant ScriptInstance::property_get_fallback(const StringName &, bool *r_valid) {
+	if (r_valid)
+		*r_valid = false;
+	return Variant();
+}
+
 void ScriptInstance::call_multilevel(const StringName &p_method, VARIANT_ARG_DECLARE) {
 
 	VARIANT_ARGPTRS;
@@ -364,6 +375,9 @@ ScriptDebugger::ScriptDebugger() {
 
 bool PlaceHolderScriptInstance::set(const StringName &p_name, const Variant &p_value) {
 
+	if (build_failed)
+		return false;
+
 	if (values.has(p_name)) {
 		Variant defval;
 		if (script->get_property_default_value(p_name, defval)) {
@@ -392,22 +406,31 @@ bool PlaceHolderScriptInstance::get(const StringName &p_name, Variant &r_ret) co
 		return true;
 	}
 
-	Variant defval;
-	if (script->get_property_default_value(p_name, defval)) {
-		r_ret = defval;
-		return true;
+	if (!build_failed) {
+		Variant defval;
+		if (script->get_property_default_value(p_name, defval)) {
+			r_ret = defval;
+			return true;
+		}
 	}
+
 	return false;
 }
 
 void PlaceHolderScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const {
 
-	for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
-		PropertyInfo pinfo = E->get();
-		if (!values.has(pinfo.name)) {
-			pinfo.usage |= PROPERTY_USAGE_SCRIPT_DEFAULT_VALUE;
+	if (build_failed) {
+		for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
+			p_properties->push_back(E->get());
+		}
+	} else {
+		for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
+			PropertyInfo pinfo = E->get();
+			if (!values.has(pinfo.name)) {
+				pinfo.usage |= PROPERTY_USAGE_SCRIPT_DEFAULT_VALUE;
+			}
+			p_properties->push_back(E->get());
 		}
-		p_properties->push_back(E->get());
 	}
 }
 
@@ -426,12 +449,18 @@ Variant::Type PlaceHolderScriptInstance::get_property_type(const StringName &p_n
 
 void PlaceHolderScriptInstance::get_method_list(List<MethodInfo> *p_list) const {
 
+	if (build_failed)
+		return;
+
 	if (script.is_valid()) {
 		script->get_script_method_list(p_list);
 	}
 }
 bool PlaceHolderScriptInstance::has_method(const StringName &p_method) const {
 
+	if (build_failed)
+		return false;
+
 	if (script.is_valid()) {
 		return script->has_method(p_method);
 	}
@@ -440,6 +469,8 @@ 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()) {
 
@@ -483,6 +514,51 @@ void PlaceHolderScriptInstance::update(const List<PropertyInfo> &p_properties, c
 	//change notify
 }
 
+void PlaceHolderScriptInstance::property_set_fallback(const StringName &p_name, const Variant &p_value, bool *r_valid) {
+
+	if (build_failed) {
+		Map<StringName, Variant>::Element *E = values.find(p_name);
+
+		if (E) {
+			E->value() = p_value;
+		} else {
+			values.insert(p_name, p_value);
+		}
+
+		bool found = false;
+		for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
+			if (E->get().name == p_name) {
+				found = true;
+				break;
+			}
+		}
+		if (!found) {
+			properties.push_back(PropertyInfo(p_value.get_type(), p_name, PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NOEDITOR | PROPERTY_USAGE_SCRIPT_VARIABLE));
+		}
+	}
+
+	if (r_valid)
+		*r_valid = false; // Cannot change the value in either case
+}
+
+Variant PlaceHolderScriptInstance::property_get_fallback(const StringName &p_name, bool *r_valid) {
+
+	if (build_failed) {
+		const Map<StringName, Variant>::Element *E = values.find(p_name);
+
+		if (E) {
+			if (r_valid)
+				*r_valid = true;
+			return E->value();
+		}
+	}
+
+	if (r_valid)
+		*r_valid = false;
+
+	return Variant();
+}
+
 PlaceHolderScriptInstance::PlaceHolderScriptInstance(ScriptLanguage *p_language, Ref<Script> p_script, Object *p_owner) :
 		owner(p_owner),
 		language(p_language),
diff --git a/core/script_language.h b/core/script_language.h
index 4e81b9b6263..c24f237e36e 100644
--- a/core/script_language.h
+++ b/core/script_language.h
@@ -115,6 +115,7 @@ public:
 
 	virtual StringName get_instance_base_type() const = 0; // this may not work in all scripts, will return empty if so
 	virtual ScriptInstance *instance_create(Object *p_this) = 0;
+	virtual PlaceHolderScriptInstance *placeholder_instance_create(Object *p_this) { return NULL; }
 	virtual bool instance_has(const Object *p_this) const = 0;
 
 	virtual bool has_source_code() const = 0;
@@ -176,6 +177,9 @@ public:
 
 	virtual bool is_placeholder() const { return false; }
 
+	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 MultiplayerAPI::RPCMode get_rpc_mode(const StringName &p_method) const = 0;
 	virtual MultiplayerAPI::RPCMode get_rset_mode(const StringName &p_variable) const = 0;
 
@@ -319,6 +323,8 @@ 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;
@@ -344,8 +350,14 @@ 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 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; }
 
diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp
index cff3be76ae6..802b60b4ff4 100644
--- a/modules/gdscript/gdscript.cpp
+++ b/modules/gdscript/gdscript.cpp
@@ -181,7 +181,11 @@ Variant GDScript::_new(const Variant **p_args, int p_argcount, Variant::CallErro
 
 bool GDScript::can_instance() const {
 
-	return valid || (!tool && !ScriptServer::is_scripting_enabled());
+#ifdef TOOLS_ENABLED
+	return valid && (tool || ScriptServer::is_scripting_enabled());
+#else
+	return valid;
+#endif
 }
 
 Ref<Script> GDScript::get_base_script() const {
@@ -310,27 +314,6 @@ bool GDScript::get_property_default_value(const StringName &p_property, Variant
 
 ScriptInstance *GDScript::instance_create(Object *p_this) {
 
-	if (!tool && !ScriptServer::is_scripting_enabled()) {
-
-#ifdef TOOLS_ENABLED
-
-		//instance a fake script for editing the values
-		//plist.invert();
-
-		/*print_line("CREATING PLACEHOLDER");
-		for(List<PropertyInfo>::Element *E=plist.front();E;E=E->next()) {
-			print_line(E->get().name);
-		}*/
-		PlaceHolderScriptInstance *si = memnew(PlaceHolderScriptInstance(GDScriptLanguage::get_singleton(), Ref<Script>(this), p_this));
-		placeholders.insert(si);
-		//_update_placeholder(si);
-		_update_exports();
-		return si;
-#else
-		return NULL;
-#endif
-	}
-
 	GDScript *top = this;
 	while (top->_base)
 		top = top->_base;
@@ -349,6 +332,27 @@ ScriptInstance *GDScript::instance_create(Object *p_this) {
 	Variant::CallError unchecked_error;
 	return _create_instance(NULL, 0, p_this, Object::cast_to<Reference>(p_this), unchecked_error);
 }
+
+PlaceHolderScriptInstance *GDScript::placeholder_instance_create(Object *p_this) {
+#ifdef TOOLS_ENABLED
+
+	//instance a fake script for editing the values
+	//plist.invert();
+
+	/*print_line("CREATING PLACEHOLDER");
+		for(List<PropertyInfo>::Element *E=plist.front();E;E=E->next()) {
+			print_line(E->get().name);
+		}*/
+	PlaceHolderScriptInstance *si = memnew(PlaceHolderScriptInstance(GDScriptLanguage::get_singleton(), Ref<Script>(this), p_this));
+	placeholders.insert(si);
+	//_update_placeholder(si);
+	_update_exports();
+	return si;
+#else
+	return NULL;
+#endif
+}
+
 bool GDScript::instance_has(const Object *p_this) const {
 
 #ifndef NO_THREADS
@@ -480,6 +484,10 @@ bool GDScript::_update_exports() {
 			for (int i = 0; i < c->_signals.size(); i++) {
 				_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);
+			}
 		}
 	} else {
 		//print_line("unchanged is "+get_path());
@@ -501,7 +509,7 @@ 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);
 		}
 	}
diff --git a/modules/gdscript/gdscript.h b/modules/gdscript/gdscript.h
index 79ac9ed4136..516a8b4899c 100644
--- a/modules/gdscript/gdscript.h
+++ b/modules/gdscript/gdscript.h
@@ -171,6 +171,7 @@ public:
 
 	virtual StringName get_instance_base_type() const; // this may not work in all scripts, will return empty if so
 	virtual ScriptInstance *instance_create(Object *p_this);
+	virtual PlaceHolderScriptInstance *placeholder_instance_create(Object *p_this);
 	virtual bool instance_has(const Object *p_this) const;
 
 	virtual bool has_source_code() const;
diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 7d7028a7a63..cd1a8266edb 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -736,6 +736,9 @@ void CSharpLanguage::reload_assemblies_if_needed(bool p_soft_reload) {
 					obj->get_script_instance()->get_property_state(state);
 					map[obj->get_instance_id()] = state;
 					obj->set_script(RefPtr());
+				} else {
+					// no instance found. Let's remove it so we don't loop forever
+					E->get()->placeholders.erase(E->get()->placeholders.front()->get());
 				}
 			}
 
@@ -747,8 +750,24 @@ void CSharpLanguage::reload_assemblies_if_needed(bool p_soft_reload) {
 		}
 	}
 
-	if (gdmono->reload_scripts_domain() != OK)
+	if (gdmono->reload_scripts_domain() != OK) {
+		// Failed to reload the scripts domain
+		// Make sure to add the scripts back to their owners before returning
+		for (Map<Ref<CSharpScript>, Map<ObjectID, List<Pair<StringName, Variant> > > >::Element *E = to_reload.front(); E; E = E->next()) {
+			Ref<CSharpScript> scr = E->key();
+			for (Map<ObjectID, List<Pair<StringName, Variant> > >::Element *F = E->get().front(); F; F = F->next()) {
+				Object *obj = ObjectDB::get_instance(F->key());
+				if (!obj)
+					continue;
+				obj->set_script(scr.get_ref_ptr());
+				// 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();
+				}
+			}
+		}
 		return;
+	}
 
 	for (Map<Ref<CSharpScript>, Map<ObjectID, List<Pair<StringName, Variant> > > >::Element *E = to_reload.front(); E; E = E->next()) {
 
@@ -778,6 +797,14 @@ void CSharpLanguage::reload_assemblies_if_needed(bool p_soft_reload) {
 				continue;
 			}
 
+			if (scr->valid && scr->is_tool() && obj->get_script_instance()->is_placeholder()) {
+				// Script instance was a placeholder, but now the script was built successfully and is a tool script.
+				// We have to replace the placeholder with an actual C# script instance.
+				scr->placeholders.erase(static_cast<PlaceHolderScriptInstance *>(obj->get_script_instance()));
+				ScriptInstance *script_instance = scr->instance_create(obj);
+				obj->set_script_instance(script_instance); // Not necessary as it's already done in instance_create, but just in case...
+			}
+
 			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);
 			}
@@ -1474,8 +1501,12 @@ void CSharpScript::_update_exports_values(Map<StringName, Variant> &values, List
 bool CSharpScript::_update_exports() {
 
 #ifdef TOOLS_ENABLED
-	if (!valid)
+	if (!valid) {
+		for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
+			E->get()->set_build_failed(true);
+		}
 		return false;
+	}
 
 	bool changed = false;
 
@@ -1577,6 +1608,7 @@ 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);
 		}
 	}
@@ -1687,7 +1719,7 @@ bool CSharpScript::_get_member_export(GDMonoClass *p_class, GDMonoClassMember *p
 
 		MonoObject *attr = p_member->get_attribute(CACHED_CLASS(ExportAttribute));
 
-		PropertyHint hint;
+		PropertyHint hint = PROPERTY_HINT_NONE;
 		String hint_string;
 
 		if (variant_type == Variant::NIL) {
@@ -1873,7 +1905,11 @@ bool CSharpScript::can_instance() const {
 	}
 #endif
 
-	return valid || (!tool && !ScriptServer::is_scripting_enabled());
+#ifdef TOOLS_ENABLED
+	return valid && (tool || ScriptServer::is_scripting_enabled());
+#else
+	return valid;
+#endif
 }
 
 StringName CSharpScript::get_instance_base_type() const {
@@ -1971,16 +2007,9 @@ Variant CSharpScript::_new(const Variant **p_args, int p_argcount, Variant::Call
 
 ScriptInstance *CSharpScript::instance_create(Object *p_this) {
 
-	if (!tool && !ScriptServer::is_scripting_enabled()) {
-#ifdef TOOLS_ENABLED
-		PlaceHolderScriptInstance *si = memnew(PlaceHolderScriptInstance(CSharpLanguage::get_singleton(), Ref<Script>(this), p_this));
-		placeholders.insert(si);
-		_update_exports();
-		return si;
-#else
-		return NULL;
+#ifdef DEBUG_ENABLED
+	CRASH_COND(!valid);
 #endif
-	}
 
 	if (!script_class) {
 		if (GDMono::get_singleton()->get_project_assembly() == NULL) {
@@ -2011,6 +2040,18 @@ ScriptInstance *CSharpScript::instance_create(Object *p_this) {
 	return _create_instance(NULL, 0, p_this, Object::cast_to<Reference>(p_this), unchecked_error);
 }
 
+PlaceHolderScriptInstance *CSharpScript::placeholder_instance_create(Object *p_this) {
+
+#ifdef TOOLS_ENABLED
+	PlaceHolderScriptInstance *si = memnew(PlaceHolderScriptInstance(CSharpLanguage::get_singleton(), Ref<Script>(this), p_this));
+	placeholders.insert(si);
+	_update_exports();
+	return si;
+#else
+	return NULL;
+#endif
+}
+
 bool CSharpScript::instance_has(const Object *p_this) const {
 
 #ifndef NO_THREADS
@@ -2077,9 +2118,11 @@ Error CSharpScript::reload(bool p_keep_state) {
 
 		if (script_class) {
 #ifdef DEBUG_ENABLED
-			OS::get_singleton()->print(String("Found class " + script_class->get_namespace() + "." +
-											  script_class->get_name() + " for script " + get_path() + "\n")
-											   .utf8());
+			if (OS::get_singleton()->is_stdout_verbose()) {
+				OS::get_singleton()->print(String("Found class " + script_class->get_namespace() + "." +
+												  script_class->get_name() + " for script " + get_path() + "\n")
+												   .utf8());
+			}
 #endif
 
 			tool = script_class->has_attribute(CACHED_CLASS(ToolAttribute));
diff --git a/modules/mono/csharp_script.h b/modules/mono/csharp_script.h
index 7f9732c2972..daa2b5bdf19 100644
--- a/modules/mono/csharp_script.h
+++ b/modules/mono/csharp_script.h
@@ -139,6 +139,7 @@ public:
 	virtual bool can_instance() const;
 	virtual StringName get_instance_base_type() const;
 	virtual ScriptInstance *instance_create(Object *p_this);
+	virtual PlaceHolderScriptInstance *placeholder_instance_create(Object *p_this);
 	virtual bool instance_has(const Object *p_this) const;
 
 	virtual bool has_source_code() const;