Implement Resource State Inheritance #86779

reduz · 2024-01-04T12:34:04Z

Ability to inherit a resource and do local changes to it.
Ability to customize in each resource how inheritance happens.

An inherited resource has a reference to the original one and shows the values of the original one. It can override these values and only those overrides are saved, otherwise the values in the original resource are used. This allows to have a base resource with a lot of parameters customized, and then inherited ones that may tweak one thing or two.

Ideally, this feature will also allow to tweak animations and animationlibraries from imported scenes (without having to do the hack of saving them to disk), although this needs other code changes that will happen in separate PRs.

How it looks:

This PR is intended to play together with this one and this proposal, to allow much better editing and usage of imported assets.

Production edit: This closes Add support for Resource inheritance (prototypal inheritance) godot-proposals#4089
and closes Allow defining inheriting Styleboxes using a parent Stylebox with overrides godot-proposals#8094.

Mickeon · 2024-01-04T13:21:01Z

Is a inherited Resource still its own thing at run-time?

That is, at run-time, modifying any property of a Resource does not affect any inherited Resource, is that correct?

By the way this would allow closing godotengine/godot-proposals#8094

reduz · 2024-01-04T14:26:04Z

@Mickeon It is its own thing. The only thing I did not implement in this PR is, if while during the editor, changing the base resource should change the inherited ones (basically something editor-only to check if the property edited in the base one is the same value as in the inherited ones, then change the inherited ones). This should not be hard to do.

During run-time, however, the inherited ones are its own thing and only fetch the non-modified properties from the base one.

Mickeon · 2024-01-04T14:53:15Z

That is excellent, and simple to describe, too. Users that want properties to propagate at run-time can listen for the changed signal of the inherited Resource, and/or override _setup_inherited. As an exposed method, it's marginally more useful than _setup_local_to_scene 💦.

Not sure about the current names, but this overall looks like a surprisingly simple yet powerful feature.

Ansraer · 2024-01-04T23:47:25Z

It would be nice to have some kind of logo in the corner of inherited resources to let the user know that it depends on something else. And maybe we could highlight the parent resource if the "sub-resource-icon" is pressed?
Otherwise this could get messy quickly, especially if you are collaborating with other people and have no clue that some resources are sub resources. Trying to figure out why some of your resource's values change seemingly spontanously sounds like a nightmare to me.

Oh, it would also be nice if the inspector would some show (e.g. with a different background color) which fields the sub resource overwrites. And a way to un_overwrite a field would probably also be necessary.

reduz · 2024-01-05T08:18:57Z

@Ansraer yeah I could put the instance logo in the inspector. Regarding the editor, anything that has the revert button is what it has been overriden.

TokisanGames · 2024-01-05T15:55:09Z

Does this store only changes from the original or is it a unique instance that is easily revertable to the original?

The context I'm thinking about is a vertex painting tool, which is not practical to use in Godot due to the current design.

As we've discussed at length, the only safe option is to keep meshes tied to GLBs so they can be reimported on demand. But the meshes are locked, so we can't paint vertices. In order to do so, we need unique resources, but then we have disassociated ArrayMeshes and when the format inevitably changes, we have a problem. We can reimport the GLBs, but Godot will complain about the old format on the disassociated ArrayMeshes in all mesh instances we have painted.

Does this PR help solve this dilemma for vertex painting?

Calinou · 2024-01-05T16:02:34Z

Does this PR help solve this dilemma for vertex painting?

A different solution is needed, similar to the existing UV2 caching system used for lightmap unwraps: godotengine/godot-proposals#6486

reduz · 2024-01-10T09:44:21Z

Vertex painting is far more involved and I have the feeling it should need a separate implementation (not even using vertex arrays). I will open a proposal soon about this.

reduz · 2024-01-10T12:46:12Z

This is ready for review.

nikitalita · 2024-01-10T14:31:51Z

core/io/resource_format_binary.cpp

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


Since we have this now, shouldn't we also replace the same sort of check done in gdextension here with this?

YuriSizov · 2024-01-10T16:25:35Z

So there is a limitation in the current implementation that only the same type can be inherited by a resource. But I think we should allow narrowing types. I can imagine a situation where I have a base StyleBox with some content margins configured, and then have a StyleBoxFlat and a StyleBoxEmpty inheriting it, but adding their own unique properties as well. In fact, this is definitely a use case we want to be able to handle in various multistate controls, like buttons.

reduz · 2024-01-11T09:12:53Z

@YuriSizov The limitation is because by default it may lead to mistakes, but it definitely could be more lean (as in, if the resource class also inherits the base class its should be fine).

I can modify it for this as you suggest.

YuriSizov · 2024-01-11T11:56:39Z

Yes, please do, I think it would be useful and expected.

* Ability to inherit a resource and do local changes to it. * Ability to customize in each resource how inheritance happens.

reduz · 2024-01-14T18:39:23Z

I applied the suggested changes. Also renamed the feature to "state inheritance" which makes more sense and is less confusing vs regular inheritance.

fire · 2024-02-26T14:43:39Z

@reduz Were we expecting to queue this for merging in 4.3? You mentioned letting this bake in the last Pipeline and Animations meeting.

theraot

This works as expected for materials.
The usability of this PR is poor. It would be better to have a way to create an inherited resource from the filesystem, similar to creating an inherited scene.
The option to set resource_inherits_state is available for PacketScene (opening the PacketScene in the inspector, not editing the scene), and it does not work. I believe it should not be visible to the inspector.
I tested this in the context this proposal of mine: Add animation inheritance godot-proposals#9802 and found it lacking for animations, but I believe that could be addressed separately.

DaloLorn · 2024-06-05T11:15:27Z

Point 2 doesn't seem like a dealbreaker - better a bad UX than no UX for this sort of thing - but on the other hand, now that it's missed the deadline for inclusion in 4.3, it does sound like something that should be addressed in time for 4.4.

theraot · 2024-06-05T14:39:45Z

None of what I said is a deal breaker. I didn't say it was.

Addendum: I'm glad people are looking at this, because I need it.

Mickeon

It is my realization ONLY NOW that the below comments have always been pending and Github never reminded me of this.

Mickeon · 2024-01-10T13:01:27Z

core/io/resource.cpp

@@ -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", "base"), &Resource::set_inherits);
+ ClassDB::bind_method(D_METHOD("get_inherits"), &Resource::get_inherits);
+ ClassDB::bind_method(D_METHOD("is_property_value_saved", "property", "value"), &Resource::is_property_value_saved);


What would be the benefit of exposing is_property_value_saved to the scripting languages, at least with the given name? Would it not be better for users to know whether or not a given property's value is being inherited or not? That is the information users are after.

This may also be easily confused with @export or PROPERTY_EXPORT_STORAGE.

Mickeon · 2024-01-10T13:11:17Z

doc/classes/EditorSettings.xml

+ <member name="resource_inherits" type="Resource" setter="set_inherits" getter="get_inherits">
+ </member>


Maybe it's better not to expose new features, without a bit of information at least. This is to not leave users that do not religiously follow the blogposts wandering in confusion.

Suggested change

<member name="resource_inherits" type="Resource" setter="set_inherits" getter="get_inherits">

</member>

<member name="resource_inherits" type="Resource" setter="set_inherits" getter="get_inherits">

The resource inherited by this resource, or [code]null[/code] if it does not exist.

In the editor, a new inherited resource can be created by clicking on "Inherit" in the Inspector's popup menu.

</member>

Wait a minute, EditorSettings? Why is this property here? What is it for?

Mickeon · 2024-01-10T13:16:38Z

doc/classes/Resource.xml

+ <description>
+ </description>


Ditto.

Suggested change

<description>

</description>

<description>

Returns [code]true[/code] if the given [param property] would be saved inside this resource's file, given the specified [param value].

</description>

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.

Calinou · 2024-06-19T13:51:00Z

@CsloudX Please don't bump issues without contributing significant new information. Use the 👍 reaction button on the first post instead.

Macksaur · 2024-08-28T15:49:37Z

core/io/resource.cpp

+ 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))) {


What is the reason for skipping bool Variant::operator==(const Variant&) here? I don't understand why it's not written as p_value == default_value. Can you elaborate?

reduz requested a review from a team as a code owner January 4, 2024 12:34

Calinou added enhancement topic:core labels Jan 4, 2024

Calinou added this to the 4.x milestone Jan 4, 2024

reduz force-pushed the resource-inheritance branch from 552e162 to 859e28b Compare January 10, 2024 12:45

reduz requested a review from a team as a code owner January 10, 2024 12:45

nikitalita reviewed Jan 10, 2024

View reviewed changes

Implement Resource Inheritance

64f0c42

* Ability to inherit a resource and do local changes to it. * Ability to customize in each resource how inheritance happens.

reduz force-pushed the resource-inheritance branch from 859e28b to 64f0c42 Compare January 14, 2024 18:38

reduz changed the title ~~Implement Resource Inheritance~~ Implement Resource State Inheritance Jan 14, 2024

fire mentioned this pull request Feb 26, 2024

Allow importing a 3D model file as a mesh resource (or material/etc) instead of a scene godotengine/godot-proposals#7494

Open

fire requested a review from a team February 26, 2024 14:45

Summersay415 mentioned this pull request May 22, 2024

Add animation inheritance godotengine/godot-proposals#9802

Open

theraot reviewed Jun 4, 2024

View reviewed changes

Mickeon reviewed Jun 8, 2024

View reviewed changes

ydeltastar mentioned this pull request Jun 9, 2024

Create some basic standalone tools or scripts for asset/scene conversion godotengine/godot-proposals#9927

Open

This comment was marked as off-topic.

Sign in to view

Summersay415 mentioned this pull request Aug 16, 2024

MultiplayerSynchronizer doesn't expose replication_config in Inspector #80717

Open

Macksaur reviewed Aug 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Resource State Inheritance #86779

Implement Resource State Inheritance #86779

reduz commented Jan 4, 2024 •

edited by Calinou

Loading

Mickeon commented Jan 4, 2024 •

edited

Loading

reduz commented Jan 4, 2024

Mickeon commented Jan 4, 2024

Ansraer commented Jan 4, 2024

reduz commented Jan 5, 2024

TokisanGames commented Jan 5, 2024

Calinou commented Jan 5, 2024

reduz commented Jan 10, 2024

reduz commented Jan 10, 2024

nikitalita Jan 10, 2024

YuriSizov commented Jan 10, 2024

reduz commented Jan 11, 2024

YuriSizov commented Jan 11, 2024

reduz commented Jan 14, 2024

fire commented Feb 26, 2024

theraot left a comment

DaloLorn commented Jun 5, 2024

theraot commented Jun 5, 2024 •

edited

Loading

Mickeon left a comment

Mickeon Jan 10, 2024

Mickeon Jan 10, 2024

Mickeon Jan 10, 2024

Mickeon Jan 10, 2024

This comment was marked as off-topic.

Calinou commented Jun 19, 2024

Macksaur Aug 28, 2024

		<member name="resource_inherits" type="Resource" setter="set_inherits" getter="get_inherits">
		</member>

Implement Resource State Inheritance #86779

Are you sure you want to change the base?

Implement Resource State Inheritance #86779

Conversation

reduz commented Jan 4, 2024 • edited by Calinou Loading

Mickeon commented Jan 4, 2024 • edited Loading

reduz commented Jan 4, 2024

Mickeon commented Jan 4, 2024

Ansraer commented Jan 4, 2024

reduz commented Jan 5, 2024

TokisanGames commented Jan 5, 2024

Calinou commented Jan 5, 2024

reduz commented Jan 10, 2024

reduz commented Jan 10, 2024

nikitalita Jan 10, 2024

Choose a reason for hiding this comment

YuriSizov commented Jan 10, 2024

reduz commented Jan 11, 2024

YuriSizov commented Jan 11, 2024

reduz commented Jan 14, 2024

fire commented Feb 26, 2024

theraot left a comment

Choose a reason for hiding this comment

DaloLorn commented Jun 5, 2024

theraot commented Jun 5, 2024 • edited Loading

Mickeon left a comment

Choose a reason for hiding this comment

Mickeon Jan 10, 2024

Choose a reason for hiding this comment

Mickeon Jan 10, 2024

Choose a reason for hiding this comment

Mickeon Jan 10, 2024

Choose a reason for hiding this comment

Mickeon Jan 10, 2024

Choose a reason for hiding this comment

This comment was marked as off-topic.

Calinou commented Jun 19, 2024

Macksaur Aug 28, 2024

Choose a reason for hiding this comment

reduz commented Jan 4, 2024 •

edited by Calinou

Loading

Mickeon commented Jan 4, 2024 •

edited

Loading

theraot commented Jun 5, 2024 •

edited

Loading