GH-626 Fix handling of script placeholder properties

src/common/variant_utils.cpp

@@ -295,4 +295,10 @@ namespace VariantUtils
         return make_default(p_target_type);
     }
 
+    bool evaluate(Variant::Operator p_operator, const Variant& p_left, const Variant& p_right, Variant& r_value)
+    {
+        bool valid = true;
+        Variant::evaluate(p_operator, p_left, p_right, r_value, valid);
+        return valid;
+    }
 }

src/common/variant_utils.h

@@ -68,6 +68,14 @@ namespace VariantUtils
     /// @param T the cast type
     /// @return the value cast to the specified type <T>
     template<typename T> T cast_to(const Variant& p_value) { return T(p_value); }
+
+    /// Evaluates two variants
+    /// @param p_operator the operation to be performed
+    /// @param p_left the left operand
+    /// @param p_right the right operand
+    /// @param r_value the return value
+    /// @return true if the evaluation was successful, false otherwise
+    bool evaluate(Variant::Operator p_operator, const Variant& p_left, const Variant& p_right, Variant& r_value);
 }
 
 #endif  // ORCHESTRATOR_VARIANT_UTILS_H

src/script/instances/script_instance_placeholder.cpp

@@ -18,8 +18,10 @@
 
 #include "common/dictionary_utils.h"
 #include "common/memory_utils.h"
+#include "common/variant_utils.h"
 #include "script/script.h"
 
+#include <godot_cpp/templates/hash_set.hpp>
 #include <godot_cpp/templates/local_vector.hpp>
 
 static OScriptInstanceInfo init_placeholder_instance_info()
@@ -101,35 +103,33 @@ bool OScriptPlaceHolderInstance::set(const StringName& p_name, const Variant& p_
     if (_script->_is_placeholder_fallback_enabled())
         return false;
 
-    if (_variables.has(p_name))
-    {
-        Variant def_value = _script->get_property_default_value(p_name);
-
-        bool valid = true;
-        Variant result;
-        Variant::evaluate(Variant::OP_EQUAL, def_value, p_value, result, valid);
-        if (valid && bool(result))
-            _variables.erase(p_name);
-        else
-            _variables[p_name] = p_value;
-
-        if (r_err)
-            *r_err = PROP_OK;
+    if (r_err)
+        *r_err = PROP_OK;
 
+    if (_values.has(p_name))
+    {
+        if (_script->_has_property_default_value(p_name))
+        {
+            Variant def_value = _script->get_property_default_value(p_name);
+
+            Variant result;
+            if (VariantUtils::evaluate(Variant::OP_EQUAL, def_value, p_value, result))
+            {
+                _values.erase(p_name);
+                return true;
+            }
+        }
+        _values[p_name] = p_value;
         return true;
     }
     else
     {
-        Variant def_value = _script->get_property_default_value(p_name);
-
-        bool valid = true;
-        Variant result;
-        Variant::evaluate(Variant::OP_NOT_EQUAL, def_value, p_value, result, valid);
-        if (valid && bool(result))
+        if (_script->_has_property_default_value(p_name))
         {
-            _variables[p_name] = p_value;
-            if (r_err)
-                *r_err = PROP_OK;
+            Variant def_value = _script->get_property_default_value(p_name);
+            Variant result;
+            if (VariantUtils::evaluate(Variant::OP_NOT_EQUAL, def_value, p_value, result))
+                _values[p_name] = p_value;
             return true;
         }
     }
@@ -142,24 +142,20 @@ bool OScriptPlaceHolderInstance::set(const StringName& p_name, const Variant& p_
 
 bool OScriptPlaceHolderInstance::get(const StringName& p_name, Variant& r_value, PropertyError* r_err)
 {
-    if (_variables.has(p_name))
-    {
-        r_value = _variables[p_name];
-        if (r_err)
-            *r_err = PROP_OK;
+    if (r_err)
+        *r_err = PROP_OK;
 
+    if (_values.has(p_name))
+    {
+        r_value = _values[p_name];
         return true;
     }
 
     if (!_script->_is_placeholder_fallback_enabled())
     {
-        Variant def_value = _script->get_property_default_value(p_name);
-        if (def_value.get_type() != Variant::NIL)
+        if (_script->_has_property_default_value(p_name))
         {
-            r_value = def_value;
-            if (r_err)
-                *r_err = PROP_OK;
-
+            r_value = _script->get_property_default_value(p_name);
             return true;
         }
     }
@@ -279,4 +275,43 @@ void OScriptPlaceHolderInstance::to_string(GDExtensionBool* r_is_valid, String*
 {
     *r_is_valid = true;
     *r_out = "OrchestratorPlaceHolderScriptInstance[" + _script->get_name() + "]";
-}
+}
+
+void OScriptPlaceHolderInstance::update(const List<PropertyInfo>& p_properties, const HashMap<StringName, Variant>& p_values)
+{
+    HashSet<StringName> new_values;
+    for (const PropertyInfo& E : p_properties)
+    {
+        if (E.usage & (PROPERTY_USAGE_GROUP | PROPERTY_USAGE_SUBGROUP | PROPERTY_USAGE_CATEGORY))
+            continue;
+
+        StringName n = E.name;
+        new_values.insert(n);
+        if (!_values.has(n) || _values[n].get_type() != E.type)
+        {
+            if (p_values.has(n))
+                _values[n] = p_values[n];
+        }
+    }
+
+    _properties = p_properties;
+
+    List<StringName> to_remove;
+    for (KeyValue<StringName, Variant>& E : _values)
+    {
+        if (!new_values.has(E.key))
+            to_remove.push_back(E.key);
+
+        Variant defval = _script->get_property_default_value(E.key);
+        if (defval == E.value)
+            to_remove.push_back(E.key);
+    }
+
+    while (to_remove.size())
+    {
+        _values.erase(to_remove.front()->get());
+        to_remove.pop_front();
+    }
+
+    _owner->notify_property_list_changed();
+}

src/script/instances/script_instance_placeholder.h

@@ -37,7 +37,8 @@ class OScriptPlaceHolderInstance : public OScriptInstanceBase
 {
     Ref<OScript> _script;                       //! The script this instance represents
     Object* _owner;                             //! The owning object of the script
-    HashMap<StringName, Variant> _variables;    //! Script variables
+    HashMap<StringName, Variant> _values;
+    List<PropertyInfo> _properties;
 
 public:
     /// Defines details about the script instance to be passed to Godot
@@ -71,6 +72,8 @@ class OScriptPlaceHolderInstance : public OScriptInstanceBase
     void notification(int32_t p_what, bool p_reversed);
     void to_string(GDExtensionBool* r_is_valid, String* r_out);
     //~ End ScriptInstanceInfo2 Interface
+
+    void update(const List<PropertyInfo>& p_properties, const HashMap<StringName, Variant>& p_values);
 };
 
 #endif  // ORCHESTRATOR_SCRIPT_PLACEHOLDER_INSTANCE_H

src/script/script.cpp

@@ -97,6 +97,7 @@ void* OScript::_placeholder_instance_create(Object* p_object) const
         _placeholders[p_object->get_instance_id()] = psi;
     }
     psi->_script_instance = GDEXTENSION_SCRIPT_INSTANCE_CREATE(&OScriptPlaceHolderInstance::INSTANCE_INFO, psi);
+    _update_exports_placeholder(nullptr, false, psi);
     return psi->_script_instance;
     #else
     return nullptr;
@@ -296,6 +297,13 @@ Variant OScript::_get_property_default_value(const StringName& p_property) const
 }
 
 void OScript::_update_exports()
+{
+    #ifdef TOOLS_ENABLED
+    _update_exports_down(false);
+    #endif
+}
+
+void OScript::_update_placeholders()
 {
 }
 
@@ -325,18 +333,39 @@ String OScript::_get_class_icon_path() const
     return {};
 }
 
-////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-/// Internal API
+void OScript::_update_export_values(HashMap<StringName, Variant>& r_values, List<PropertyInfo>& r_properties) const
+{
+    for (const Ref<OScriptVariable>& variable : get_variables())
+    {
+        PropertyInfo property = variable->get_info();
+        if (variable->is_grouped_by_category())
+            property.name = vformat("%s/%s", variable->get_category(), variable->get_variable_name());
+
+        r_values[property.name] = variable->get_default_value();
+        r_properties.push_back(property);
+    }
+}
 
-bool OScript::_update_exports_placeholder(bool* r_err, bool p_recursive_call, OScriptInstance* p_instance) const
+bool OScript::_update_exports_placeholder(bool* r_err, bool p_recursive_call, OScriptPlaceHolderInstance* p_instance, bool p_base_exports_changed) const
 {
 #ifdef TOOLS_ENABLED
+    HashMap<StringName, Variant> values;
+    List<PropertyInfo> properties;
+    _update_export_values(values, properties);
+
+    for (const KeyValue<uint64_t, OScriptPlaceHolderInstance*>& E : _placeholders)
+        E.value->update(properties, values);
+
     return true;
 #else
     return false;
 #endif
 }
 
-void OScript::_update_placeholders()
+void OScript::_update_exports_down(bool p_base_exports_changed)
 {
+    bool cyclic_error = false;
+    _update_exports_placeholder(&cyclic_error, false, nullptr, p_base_exports_changed);
+    // todo: add inheriters_cache
 }
+

src/script/script.h

@@ -72,14 +72,21 @@ class OScript : public ScriptExtension, public Orchestration
     void _set_signals(const TypedArray<OScriptSignal>& p_signals) { _set_signals_internal(p_signals); }
     //~ End Serialization API
 
+    /// Updates the exported values
+    /// @param r_values the exported variable values
+    /// @param r_properties the exported variable property details
+    void _update_export_values(HashMap<StringName, Variant>& r_values, List<PropertyInfo>& r_properties) const;
+
     /// Update export placeholders
     /// @param r_err the output error
     /// @param p_recursive whether called recursively
     /// @param p_instance the script instance, should never be null
-    bool _update_exports_placeholder(bool* r_err, bool p_recursive, OScriptInstance* p_instance) const;
+    /// @param p_base_exports_changed whether base exports changed
+    bool _update_exports_placeholder(bool* r_err = nullptr, bool p_recursive = false, OScriptPlaceHolderInstance* p_instance = nullptr, bool p_base_exports_changed = false) const;
 
-    /// Updates the placeholders
-    void _update_placeholders() override;
+    /// Updates the exports
+    /// @param p_base_exports_changed whether the base class exports changed
+    void _update_exports_down(bool p_base_exports_changed);
 
 public:
     OScript();
@@ -115,6 +122,7 @@ class OScript : public ScriptExtension, public Orchestration
     bool _has_property_default_value(const StringName& p_property) const override;
     Variant _get_property_default_value(const StringName& p_property) const override;
     void _update_exports() override;
+    void _update_placeholders() override;
     int32_t _get_member_line(const StringName& p_member) const override;
     Dictionary _get_constants() const override;
     TypedArray<StringName> _get_members() const override;