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.
This commit is contained in:
Ignacio Etcheverry 2018-07-29 22:40:09 +02:00
parent 6601502acd
commit f3c7527225
7 changed files with 253 additions and 57 deletions

View file

@ -450,7 +450,8 @@ void Object::set(const StringName &p_name, const Variant &p_value, bool *r_valid
*r_valid = true; *r_valid = true;
return; return;
#endif #endif
} else { }
//something inside the object... :| //something inside the object... :|
bool success = _setv(p_name, p_value); bool success = _setv(p_name, p_value);
if (success) { if (success) {
@ -458,10 +459,34 @@ void Object::set(const StringName &p_name, const Variant &p_value, bool *r_valid
*r_valid = true; *r_valid = true;
return; return;
} }
setvar(p_name, p_value, r_valid);
{
bool valid;
setvar(p_name, p_value, &valid);
if (valid) {
if (r_valid)
*r_valid = true;
return;
} }
} }
#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 { Variant Object::get(const StringName &p_name, bool *r_valid) const {
Variant ret; Variant ret;
@ -513,8 +538,33 @@ Variant Object::get(const StringName &p_name, bool *r_valid) const {
*r_valid = true; *r_valid = true;
return ret; return ret;
} }
//if nothing else, use getvar //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; script = p_script;
Ref<Script> s(script); Ref<Script> s(script);
if (!s.is_null() && s->can_instance()) { if (!s.is_null()) {
if (s->can_instance()) {
OBJ_DEBUG_LOCK OBJ_DEBUG_LOCK
script_instance = s->instance_create(this); 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"); _change_notify("script");

View file

@ -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 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) { void ScriptInstance::call_multilevel(const StringName &p_method, VARIANT_ARG_DECLARE) {
VARIANT_ARGPTRS; VARIANT_ARGPTRS;
@ -364,6 +375,9 @@ ScriptDebugger::ScriptDebugger() {
bool PlaceHolderScriptInstance::set(const StringName &p_name, const Variant &p_value) { bool PlaceHolderScriptInstance::set(const StringName &p_name, const Variant &p_value) {
if (build_failed)
return false;
if (values.has(p_name)) { if (values.has(p_name)) {
Variant defval; Variant defval;
if (script->get_property_default_value(p_name, defval)) { if (script->get_property_default_value(p_name, defval)) {
@ -392,16 +406,24 @@ bool PlaceHolderScriptInstance::get(const StringName &p_name, Variant &r_ret) co
return true; return true;
} }
if (!build_failed) {
Variant defval; Variant defval;
if (script->get_property_default_value(p_name, defval)) { if (script->get_property_default_value(p_name, defval)) {
r_ret = defval; r_ret = defval;
return true; return true;
} }
}
return false; return false;
} }
void PlaceHolderScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const { void PlaceHolderScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const {
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()) { for (const List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
PropertyInfo pinfo = E->get(); PropertyInfo pinfo = E->get();
if (!values.has(pinfo.name)) { if (!values.has(pinfo.name)) {
@ -410,6 +432,7 @@ void PlaceHolderScriptInstance::get_property_list(List<PropertyInfo> *p_properti
p_properties->push_back(E->get()); p_properties->push_back(E->get());
} }
} }
}
Variant::Type PlaceHolderScriptInstance::get_property_type(const StringName &p_name, bool *r_is_valid) const { Variant::Type PlaceHolderScriptInstance::get_property_type(const StringName &p_name, bool *r_is_valid) const {
@ -426,12 +449,18 @@ Variant::Type PlaceHolderScriptInstance::get_property_type(const StringName &p_n
void PlaceHolderScriptInstance::get_method_list(List<MethodInfo> *p_list) const { void PlaceHolderScriptInstance::get_method_list(List<MethodInfo> *p_list) const {
if (build_failed)
return;
if (script.is_valid()) { if (script.is_valid()) {
script->get_script_method_list(p_list); script->get_script_method_list(p_list);
} }
} }
bool PlaceHolderScriptInstance::has_method(const StringName &p_method) const { bool PlaceHolderScriptInstance::has_method(const StringName &p_method) const {
if (build_failed)
return false;
if (script.is_valid()) { if (script.is_valid()) {
return script->has_method(p_method); 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) { void PlaceHolderScriptInstance::update(const List<PropertyInfo> &p_properties, const Map<StringName, Variant> &p_values) {
build_failed = false;
Set<StringName> new_values; Set<StringName> new_values;
for (const List<PropertyInfo>::Element *E = p_properties.front(); E; E = E->next()) { 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 //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) : PlaceHolderScriptInstance::PlaceHolderScriptInstance(ScriptLanguage *p_language, Ref<Script> p_script, Object *p_owner) :
owner(p_owner), owner(p_owner),
language(p_language), language(p_language),

View file

@ -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 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 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 instance_has(const Object *p_this) const = 0;
virtual bool has_source_code() const = 0; virtual bool has_source_code() const = 0;
@ -176,6 +177,9 @@ public:
virtual bool is_placeholder() const { return false; } 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_rpc_mode(const StringName &p_method) const = 0;
virtual MultiplayerAPI::RPCMode get_rset_mode(const StringName &p_variable) const = 0; virtual MultiplayerAPI::RPCMode get_rset_mode(const StringName &p_variable) const = 0;
@ -319,6 +323,8 @@ class PlaceHolderScriptInstance : public ScriptInstance {
ScriptLanguage *language; ScriptLanguage *language;
Ref<Script> script; Ref<Script> script;
bool build_failed;
public: public:
virtual bool set(const StringName &p_name, const Variant &p_value); virtual bool set(const StringName &p_name, const Variant &p_value);
virtual bool get(const StringName &p_name, Variant &r_ret) const; 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 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 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_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; } virtual MultiplayerAPI::RPCMode get_rset_mode(const StringName &p_variable) const { return MultiplayerAPI::RPC_MODE_DISABLED; }

View file

@ -181,7 +181,11 @@ Variant GDScript::_new(const Variant **p_args, int p_argcount, Variant::CallErro
bool GDScript::can_instance() const { 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 { Ref<Script> GDScript::get_base_script() const {
@ -310,8 +314,26 @@ bool GDScript::get_property_default_value(const StringName &p_property, Variant
ScriptInstance *GDScript::instance_create(Object *p_this) { ScriptInstance *GDScript::instance_create(Object *p_this) {
if (!tool && !ScriptServer::is_scripting_enabled()) { GDScript *top = this;
while (top->_base)
top = top->_base;
if (top->native.is_valid()) {
if (!ClassDB::is_parent_class(p_this->get_class_name(), top->native->get_name())) {
if (ScriptDebugger::get_singleton()) {
GDScriptLanguage::get_singleton()->debug_break_parse(get_path(), 0, "Script inherits from native type '" + String(top->native->get_name()) + "', so it can't be instanced in object of type: '" + p_this->get_class() + "'");
}
ERR_EXPLAIN("Script inherits from native type '" + String(top->native->get_name()) + "', so it can't be instanced in object of type: '" + p_this->get_class() + "'");
ERR_FAIL_V(NULL);
}
}
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 #ifdef TOOLS_ENABLED
//instance a fake script for editing the values //instance a fake script for editing the values
@ -331,24 +353,6 @@ ScriptInstance *GDScript::instance_create(Object *p_this) {
#endif #endif
} }
GDScript *top = this;
while (top->_base)
top = top->_base;
if (top->native.is_valid()) {
if (!ClassDB::is_parent_class(p_this->get_class_name(), top->native->get_name())) {
if (ScriptDebugger::get_singleton()) {
GDScriptLanguage::get_singleton()->debug_break_parse(get_path(), 0, "Script inherits from native type '" + String(top->native->get_name()) + "', so it can't be instanced in object of type: '" + p_this->get_class() + "'");
}
ERR_EXPLAIN("Script inherits from native type '" + String(top->native->get_name()) + "', so it can't be instanced in object of type: '" + p_this->get_class() + "'");
ERR_FAIL_V(NULL);
}
}
Variant::CallError unchecked_error;
return _create_instance(NULL, 0, p_this, Object::cast_to<Reference>(p_this), unchecked_error);
}
bool GDScript::instance_has(const Object *p_this) const { bool GDScript::instance_has(const Object *p_this) const {
#ifndef NO_THREADS #ifndef NO_THREADS
@ -480,6 +484,10 @@ bool GDScript::_update_exports() {
for (int i = 0; i < c->_signals.size(); i++) { for (int i = 0; i < c->_signals.size(); i++) {
_signals[c->_signals[i].name] = c->_signals[i].arguments; _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 { } else {
//print_line("unchanged is "+get_path()); //print_line("unchanged is "+get_path());
@ -501,7 +509,7 @@ bool GDScript::_update_exports() {
_update_exports_values(values, propnames); _update_exports_values(values, propnames);
for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) { for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
E->get()->set_build_failed(false);
E->get()->update(propnames, values); E->get()->update(propnames, values);
} }
} }

View file

@ -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 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 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 instance_has(const Object *p_this) const;
virtual bool has_source_code() const; virtual bool has_source_code() const;

View file

@ -736,6 +736,9 @@ void CSharpLanguage::reload_assemblies_if_needed(bool p_soft_reload) {
obj->get_script_instance()->get_property_state(state); obj->get_script_instance()->get_property_state(state);
map[obj->get_instance_id()] = state; map[obj->get_instance_id()] = state;
obj->set_script(RefPtr()); 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; return;
}
for (Map<Ref<CSharpScript>, Map<ObjectID, List<Pair<StringName, Variant> > > >::Element *E = to_reload.front(); E; E = E->next()) { 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; 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()) { 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); 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() { bool CSharpScript::_update_exports() {
#ifdef TOOLS_ENABLED #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; return false;
}
bool changed = false; bool changed = false;
@ -1577,6 +1608,7 @@ bool CSharpScript::_update_exports() {
_update_exports_values(values, propnames); _update_exports_values(values, propnames);
for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) { for (Set<PlaceHolderScriptInstance *>::Element *E = placeholders.front(); E; E = E->next()) {
E->get()->set_build_failed(false);
E->get()->update(propnames, values); 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)); MonoObject *attr = p_member->get_attribute(CACHED_CLASS(ExportAttribute));
PropertyHint hint; PropertyHint hint = PROPERTY_HINT_NONE;
String hint_string; String hint_string;
if (variant_type == Variant::NIL) { if (variant_type == Variant::NIL) {
@ -1873,7 +1905,11 @@ bool CSharpScript::can_instance() const {
} }
#endif #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 { 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) { ScriptInstance *CSharpScript::instance_create(Object *p_this) {
if (!tool && !ScriptServer::is_scripting_enabled()) { #ifdef DEBUG_ENABLED
#ifdef TOOLS_ENABLED CRASH_COND(!valid);
PlaceHolderScriptInstance *si = memnew(PlaceHolderScriptInstance(CSharpLanguage::get_singleton(), Ref<Script>(this), p_this));
placeholders.insert(si);
_update_exports();
return si;
#else
return NULL;
#endif #endif
}
if (!script_class) { if (!script_class) {
if (GDMono::get_singleton()->get_project_assembly() == NULL) { 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); 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 { bool CSharpScript::instance_has(const Object *p_this) const {
#ifndef NO_THREADS #ifndef NO_THREADS
@ -2077,9 +2118,11 @@ Error CSharpScript::reload(bool p_keep_state) {
if (script_class) { if (script_class) {
#ifdef DEBUG_ENABLED #ifdef DEBUG_ENABLED
if (OS::get_singleton()->is_stdout_verbose()) {
OS::get_singleton()->print(String("Found class " + script_class->get_namespace() + "." + OS::get_singleton()->print(String("Found class " + script_class->get_namespace() + "." +
script_class->get_name() + " for script " + get_path() + "\n") script_class->get_name() + " for script " + get_path() + "\n")
.utf8()); .utf8());
}
#endif #endif
tool = script_class->has_attribute(CACHED_CLASS(ToolAttribute)); tool = script_class->has_attribute(CACHED_CLASS(ToolAttribute));

View file

@ -139,6 +139,7 @@ public:
virtual bool can_instance() const; virtual bool can_instance() const;
virtual StringName get_instance_base_type() const; virtual StringName get_instance_base_type() const;
virtual ScriptInstance *instance_create(Object *p_this); 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 instance_has(const Object *p_this) const;
virtual bool has_source_code() const; virtual bool has_source_code() const;