-
-
Notifications
You must be signed in to change notification settings - Fork 21k
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.
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
Implement Resource State Inheritance #86779
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -437,6 +437,98 @@ String Resource::get_id_for_path(const String &p_path) const { | |
} | ||
#endif | ||
|
||
bool Resource::_property_can_revert(const StringName &p_name) const { | ||
if (!inherits_state.is_valid()) { | ||
return false; | ||
} | ||
bool value_valid; | ||
Variant value = get(p_name, &value_valid); | ||
if (!value_valid) { | ||
return false; | ||
} | ||
Variant inherits_value = inherits_state->get(p_name, &value_valid); | ||
if (!value_valid) { | ||
return false; | ||
} | ||
return bool(Variant::evaluate(Variant::OP_NOT_EQUAL, value, inherits_value)); | ||
} | ||
|
||
bool Resource::_property_get_revert(const StringName &p_name, Variant &r_property) const { | ||
if (!inherits_state.is_valid()) { | ||
return false; | ||
} | ||
|
||
bool value_valid; | ||
Variant inherits_value = inherits_state->get(p_name, &value_valid); | ||
if (value_valid) { | ||
r_property = inherits_value; | ||
} | ||
return value_valid; | ||
} | ||
|
||
void Resource::_validate_property(PropertyInfo &p_property) const { | ||
if (p_property.name == "resource_inherits") { | ||
p_property.hint_string = get_class(); | ||
} | ||
} | ||
|
||
bool Resource::setup_inherits_state(const Ref<Resource> &p_resource) { | ||
bool ret; | ||
if (GDVIRTUAL_CALL(_setup_inherits_state, p_resource, ret)) { | ||
return ret; | ||
} | ||
|
||
ERR_FAIL_COND_V_MSG(p_resource.is_valid() && !is_class(p_resource->get_class_name()), false, "Resources must be of (or class inherit) the same class when setting up state inheritance"); | ||
|
||
if (p_resource.is_valid()) { | ||
copy_from(p_resource); | ||
} | ||
return true; | ||
} | ||
|
||
void Resource::set_inherits_state(const Ref<Resource> &p_resource) { | ||
if (setup_inherits_state(p_resource)) { | ||
inherits_state = p_resource; | ||
} | ||
} | ||
|
||
Ref<Resource> Resource::get_inherits_state() const { | ||
return inherits_state; | ||
} | ||
|
||
bool Resource::is_inherited_state_property_value_saved(const StringName &p_name, const Variant &p_value) const { | ||
bool ret; | ||
if (GDVIRTUAL_CALL(_is_inherited_state_property_value_saved, p_name, p_value, ret)) { | ||
return ret; | ||
} | ||
|
||
bool default_value_valid; | ||
Variant default_value = inherits_state->get(p_name, &default_value_valid); | ||
if (!default_value_valid) { | ||
return true; // Not a default value, so save. | ||
} | ||
|
||
if (default_value.get_type() != Variant::NIL && bool(Variant::evaluate(Variant::OP_EQUAL, p_value, default_value))) { | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
bool Resource::is_property_value_saved(const StringName &p_property, const Variant &p_value) const { | ||
if (!inherits_state.is_valid()) { | ||
Variant default_value = ClassDB::class_get_default_property_value(get_class(), p_property); | ||
|
||
if (default_value.get_type() != Variant::NIL && bool(Variant::evaluate(Variant::OP_EQUAL, p_value, default_value))) { | ||
return false; | ||
} | ||
|
||
return true; | ||
} else { | ||
return is_inherited_state_property_value_saved(p_property, p_value); | ||
} | ||
} | ||
|
||
void Resource::_bind_methods() { | ||
ClassDB::bind_method(D_METHOD("set_path", "path"), &Resource::_set_path); | ||
ClassDB::bind_method(D_METHOD("take_over_path", "path"), &Resource::_take_over_path); | ||
|
@@ -448,6 +540,9 @@ void Resource::_bind_methods() { | |
ClassDB::bind_method(D_METHOD("is_local_to_scene"), &Resource::is_local_to_scene); | ||
ClassDB::bind_method(D_METHOD("get_local_scene"), &Resource::get_local_scene); | ||
ClassDB::bind_method(D_METHOD("setup_local_to_scene"), &Resource::setup_local_to_scene); | ||
ClassDB::bind_method(D_METHOD("set_inherits_state", "base"), &Resource::set_inherits_state); | ||
ClassDB::bind_method(D_METHOD("get_inherits_state"), &Resource::get_inherits_state); | ||
ClassDB::bind_method(D_METHOD("is_property_value_saved", "property", "value"), &Resource::is_property_value_saved); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be the benefit of exposing This may also be easily confused with |
||
|
||
ClassDB::bind_method(D_METHOD("emit_changed"), &Resource::emit_changed); | ||
|
||
|
@@ -459,12 +554,15 @@ void Resource::_bind_methods() { | |
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "resource_local_to_scene"), "set_local_to_scene", "is_local_to_scene"); | ||
ADD_PROPERTY(PropertyInfo(Variant::STRING, "resource_path", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_EDITOR), "set_path", "get_path"); | ||
ADD_PROPERTY(PropertyInfo(Variant::STRING, "resource_name"), "set_name", "get_name"); | ||
ADD_PROPERTY(PropertyInfo(Variant::OBJECT, "resource_inherits_state", PROPERTY_HINT_RESOURCE_TYPE, "Resource"), "set_inherits_state", "get_inherits_state"); | ||
|
||
MethodInfo get_rid_bind("_get_rid"); | ||
get_rid_bind.return_val.type = Variant::RID; | ||
|
||
::ClassDB::add_virtual_method(get_class_static(), get_rid_bind, true, Vector<String>(), true); | ||
GDVIRTUAL_BIND(_setup_local_to_scene); | ||
GDVIRTUAL_BIND(_setup_inherits_state, "resource"); | ||
GDVIRTUAL_BIND(_is_inherited_state_property_value_saved, "property", "value"); | ||
} | ||
|
||
Resource::Resource() : | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2189,9 +2189,7 @@ Error ResourceFormatSaverBinaryInstance::save(const String &p_path, const Ref<Re | |
} | ||
} | ||
|
||
Variant default_value = ClassDB::class_get_default_property_value(E->get_class(), F.name); | ||
|
||
if (default_value.get_type() != Variant::NIL && bool(Variant::evaluate(Variant::OP_EQUAL, p.value, default_value))) { | ||
if (!E->is_property_value_saved(F.name, p.value)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have this now, shouldn't we also replace the same sort of check done in gdextension here with this? |
||
continue; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -20,6 +20,20 @@ | |||||||||||
Override this method to return a custom [RID] when [method get_rid] is called. | ||||||||||||
</description> | ||||||||||||
</method> | ||||||||||||
<method name="_is_inherited_state_property_value_saved" qualifiers="virtual const"> | ||||||||||||
<return type="bool" /> | ||||||||||||
<param index="0" name="property" type="StringName" /> | ||||||||||||
<param index="1" name="value" type="Variant" /> | ||||||||||||
<description> | ||||||||||||
</description> | ||||||||||||
</method> | ||||||||||||
<method name="_setup_inherits_state" qualifiers="virtual"> | ||||||||||||
<return type="bool" /> | ||||||||||||
<param index="0" name="resource" type="Resource" /> | ||||||||||||
<description> | ||||||||||||
This function is called when resource state inheritance is being set up. The new base resource is notified via [b]resource[/b] while the existing one can still be obtained via the [member resource_inherits_state] property. | ||||||||||||
</description> | ||||||||||||
</method> | ||||||||||||
<method name="_setup_local_to_scene" qualifiers="virtual"> | ||||||||||||
<return type="void" /> | ||||||||||||
<description> | ||||||||||||
|
@@ -71,6 +85,13 @@ | |||||||||||
Returns the [RID] of this resource (or an empty RID). Many resources (such as [Texture2D], [Mesh], and so on) are high-level abstractions of resources stored in a specialized server ([DisplayServer], [RenderingServer], etc.), so this function will return the original [RID]. | ||||||||||||
</description> | ||||||||||||
</method> | ||||||||||||
<method name="is_property_value_saved" qualifiers="const"> | ||||||||||||
<return type="bool" /> | ||||||||||||
<param index="0" name="property" type="StringName" /> | ||||||||||||
<param index="1" name="value" type="Variant" /> | ||||||||||||
<description> | ||||||||||||
</description> | ||||||||||||
Comment on lines
+92
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto.
Suggested change
But you can notice a bit of struggle when attempting to properly document this method. Mainly, "saved" is a vague term here. Hence my questions in the above reviews. |
||||||||||||
</method> | ||||||||||||
<method name="setup_local_to_scene" is_deprecated="true"> | ||||||||||||
<return type="void" /> | ||||||||||||
<description> | ||||||||||||
|
@@ -87,6 +108,9 @@ | |||||||||||
</method> | ||||||||||||
</methods> | ||||||||||||
<members> | ||||||||||||
<member name="resource_inherits_state" type="Resource" setter="set_inherits_state" getter="get_inherits_state"> | ||||||||||||
Resource that this resource inherits state from. It must be the same type as this one. When resource state inheritance is used, this resource will inherit the state of properties from the base one and only local notifications vs the base ones will be saved. | ||||||||||||
</member> | ||||||||||||
<member name="resource_local_to_scene" type="bool" setter="set_local_to_scene" getter="is_local_to_scene" default="false"> | ||||||||||||
If [code]true[/code], the resource is duplicated for each instance of all scenes using it. At run-time, the resource can be modified in one scene without affecting other instances (see [method PackedScene.instantiate]). | ||||||||||||
[b]Note:[/b] Changing this property at run-time has no effect on already created duplicate resources. | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for skipping
bool Variant::operator==(const Variant&)
here? I don't understand why it's not written asp_value == default_value
. Can you elaborate?