Fix resource_local_to_scene in arrays and dictionaries

[Wyxaldir]

Attempts to fix resource_local_to_scene in arrays, see #71243.

Bugsquad edit:

• Fixes Resource local to scene is ignored for arrays and dictionaries (including surface materials) #71243

[RedMser]

Thanks for tackling this! ❤️

However, it doesn't seem to fix the issue in the current state. Running the MRP with your PR applied, there still are unduplicated resources, leaving the debug output the same as in the issue description (highlighted):

Your new code seems to be called and duplicating properly, so I'm not sure what's going wrong.

Redownloading the MRP fixed this. Unsure what's up with that... See later comment for more details.

---

Also, you should likely remove the fixes #... from the issue description, since it does not handle dictionaries. The issue would automatically close on merge if you keep the wording like this.

[Wyxaldir]

That's strange, as I thought it was working here. I have another test project where this fixed my issues, and the logs for the MRP seem right on my end.

pre-modified first ---------
simple:
true	1	LocalToSceneTests1:<Node#26457670861>
array:
true	2	LocalToSceneTests1:<Node#26457670861>
true	3	LocalToSceneTests1:<Node#26457670861>
dict:
true	4	<Object#null>
sub:
true	5	LocalToSceneTests1:<Node#26457670861>
sub array:
true	6	LocalToSceneTests1:<Node#26457670861>
sub dict:
true	20	LocalToSceneTests1:<Node#26457670861>
check first --------- (should be same as above)
simple:
true	1	LocalToSceneTests1:<Node#26457670861>
array:
true	2	LocalToSceneTests1:<Node#26457670861>
true	3	LocalToSceneTests1:<Node#26457670861>
dict:
true	8	<Object#null>
sub:
true	5	LocalToSceneTests1:<Node#26457670861>
sub array:
true	6	LocalToSceneTests1:<Node#26457670861>
sub dict:
true	20	LocalToSceneTests1:<Node#26457670861>
check second --------- (should be *= 2 on each field)
simple:
true	2	LocalToSceneTests2:<Node#26608665818>
array:
true	4	LocalToSceneTests2:<Node#26608665818>
true	6	LocalToSceneTests2:<Node#26608665818>
dict:
true	8	<Object#null>
sub:
true	10	LocalToSceneTests2:<Node#26608665818>
sub array:
true	12	LocalToSceneTests2:<Node#26608665818>
sub dict:
true	40	LocalToSceneTests2:<Node#26608665818>```

[RedMser]

Hmm I redownloaded the MRP and it indeed works as expected now! 🎉
Checked the diff between the two and this change stands out to me:

(left is working, right is broken)

Not sure what happened, or if it's something I unknowingly did 😅

[akien-mga]

Could you squash the commits? See PR workflow for instructions.

[Wyxaldir]

Squashed and fixed formatting, hopefully good to go!

[KoBeWi]

I'm getting a deja vu here.
This only handles resources in arrays, but arrays can be nested. Then there are also Dictionaries, which can use Resources both for keys and values. And they can be nested too. Also I'm not sure if nested resources are handled or if it's for nodes only.
Ultimately we'd end up with a unified method that just scans everything recursively. I remember we have at least 3 of them.

Although that's probably all corner cases. The issue mentions also Dictionaries, so I think this PR could also handle them. It doesn't need to be recursive for now, but it would be nice if the copied code could be moved to some method.

EDIT:
Ok I wrote the unified method:

void SceneState::setup_local_resource(Variant &r_value, HashMap<Ref<Resource>, Ref<Resource>> &p_resources_local_to_sub_scene, HashMap<Ref<Resource>, Ref<Resource>> &p_resources_local_to_scene, const SceneState::NodeData &p_node_data, Node *p_node, Node *p_base, const StringName &p_property, int p_edit_state) const {
	Ref<Resource> resource = r_value;

	if (resource.is_null() || !resource->is_local_to_scene()) {
		return;
	}

	if (p_node_data.instance >= 0) { // For the root node of a sub-scene, treat it as part of the sub-scene.
		r_value = get_remap_resource(resource, p_resources_local_to_sub_scene, p_node->get(p_property), p_node);
	} else {
		HashMap<Ref<Resource>, Ref<Resource>>::Iterator E = p_resources_local_to_scene.find(resource);
		if (E) {
			r_value = E->value;
		} else {
			if (p_edit_state == GEN_EDIT_STATE_MAIN) {
				// For the main scene, use the resource as is.
				resource->configure_for_local_scene(p_base, p_resources_local_to_scene);
				p_resources_local_to_scene[resource] = resource;
			} else {
				// For instances, a copy must be made.
				Ref<Resource> local_dupe = resource->duplicate_for_local_scene(p_base, p_resources_local_to_scene);
				p_resources_local_to_scene[resource] = local_dupe;
				r_value = local_dupe;
			}
		}
	}
}

It has ugly argument list, because there is many local variables involved. And also it doesn't work for some reason .-.
Still, we'll need something like that if we were to handle Dictionaries.

btw I also made a simpler reproduction project:
CustomLocalToScene.zip
If all shapes have different size it means the bug is fixed.

[Wyxaldir]

btw I also made a simpler reproduction project: CustomLocalToScene.zip
If all shapes have different size it means the bug is fixed.

This zip is missing the script I think.

I'll try to implement the method you outlined. As for dictionaries, I'm not certain how to tackle it, should I check for object and then dictionary? (pseudo code below)

if (value.get_type() == Variant::OBJECT) {
    if (value.get_type() == Variant::DICTIONARY) {
        // Check for resources in keys and values
    }
}

And yeah, the nesting is certainly an issue, I'll see if I can deal with it too while I'm at it.

[RedMser]

As for dictionaries, I'm not certain how to tackle it, should I check for object and then dictionary?

You can get inspired by my PR that tackled the other half of the issue: #71578
There's a switch-case which handles all cases (including nesting, arrays, dictionary keys and values).

So your pseudo code would need to switch over the variant types OBJECT, ARRAY, DICTIONARY and act accordingly.

[KoBeWi]

This zip is missing the script I think.

Sorry, here's a fixed one:
CustomLocalTooScene.zip

As for dictionaries, I'm not certain how to tackle it, should I check for object and then dictionary?

Dictionary is not Object.

Also I wouldn't really bother for recursive checking, as it often caused performance problems. For now handling only top level properties is fine I think.

[Wyxaldir]

The issue so far is that the resource is always duplicated now, even if it already was. What this does (to my understanding) is that the values get re-duplicated every time you open the scene.
For example, I have a parent scene with a Shape2D in an array with local_to_scene set to true. I make an instance of the scene in my Main scene. I'm not able to edit the values in the Main scene and it doesn't affect the values in the parent scene, great! However, if I close and reopen the Main scene, the Shape2D has now reverted to the original value found in the parent scene.

void SceneState::setup_local_resource(Ref<Resource>& r_res, Array& p_origin_array, const int p_origin_array_index, const SceneState::NodeData& p_node_data, HashMap<Ref<Resource>, Ref<Resource>>& p_resources_local_to_sub_scene, Node* p_node, const StringName p_sname, HashMap<Ref<Resource>, Ref<Resource>>& p_resources_local_to_scene, int p_i, Node** p_ret_nodes, SceneState::GenEditState p_edit_state) const
{
	if (!r_res->is_local_to_scene()) {
		printf("Resource was not local.\n");
		return;
	}
	if (p_node_data.instance >= 0) { // For the root node of a sub-scene, treat it as part of the sub-scene.
		p_origin_array[p_origin_array_index] = get_remap_resource(r_res, p_resources_local_to_sub_scene, p_node->get(p_sname), p_node);
		printf("Remapping.\n");
	} else {
		HashMap<Ref<Resource>, Ref<Resource>>::Iterator E = p_resources_local_to_scene.find(r_res);
		Node* base = p_i == 0 ? p_node : p_ret_nodes[0];
		if (E) {
			p_origin_array[p_origin_array_index] = E->value;
			printf("Found original, no change.\n");
		} else if (p_edit_state == GEN_EDIT_STATE_MAIN) {
			// For the main scene, use the resource as is.
			r_res->configure_for_local_scene(base, p_resources_local_to_scene);
			p_resources_local_to_scene[r_res] = r_res;
			printf("Main scene.\n");
		} else {
			// For instances, a copy must be made.
			Ref<Resource> local_dupe = r_res->duplicate_for_local_scene(base, p_resources_local_to_scene);
			p_resources_local_to_scene[r_res] = local_dupe;
			p_origin_array[p_origin_array_index] = local_dupe;
			printf("Duplicate for local scene.\n");
		}
	}
}

My guess is that somehow it's not finding the resource when it runs this for some reason.
EDIT: This is incorrect, the original resource implementation doesn't run this code in my circumstances either, so I'm not certain what the difference is.

HashMap<Ref<Resource>, Ref<Resource>>::Iterator E = p_resources_local_to_scene.find(r_res);

Your test scene works btw, when the nodes are modified at runtime they are properly duplicated, so that's good.

[Wyxaldir]

I'm so close, but I can't put together the final line(s) needed. The reason @KoBeWi 's method doesn't work is because the final variant needs to be assigned to value, but we were just assigning it to the Ref previously. I made a version that returns a Ref and assigns it to value and it works now.

I imagine it's the same thing with the array, but despite attempting to assign value to the modified array I still run into the same issues as above. I'm not sure if Arrays have a special way of being assigned though that I might be missing however.

This is my version of your method now btw:

Ref<Resource> SceneState::make_local_resource(Variant& r_value, const SceneState::NodeData& p_node_data, HashMap<Ref<Resource>, Ref<Resource>>& p_resources_local_to_sub_scene, Node* p_node, const StringName p_sname, HashMap<Ref<Resource>, Ref<Resource>>& p_resources_local_to_scene, int p_i, Node** p_ret_nodes, SceneState::GenEditState p_edit_state) const
{
	Ref<Resource> res = r_value;
	if (res.is_null() || !res->is_local_to_scene()) {
		return r_value;
	}

	if (p_node_data.instance >= 0) { // For the root node of a sub-scene, treat it as part of the sub-scene.
		return get_remap_resource(res, p_resources_local_to_sub_scene, p_node->get(p_sname), p_node);
	} else {
		HashMap<Ref<Resource>, Ref<Resource>>::Iterator E = p_resources_local_to_scene.find(res);
		Node* base = p_i == 0 ? p_node : p_ret_nodes[0];
		if (E) {
			return E->value;
		} else if (p_edit_state == GEN_EDIT_STATE_MAIN) { // For the main scene, use the resource as is
			res->configure_for_local_scene(base, p_resources_local_to_scene);
			p_resources_local_to_scene[res] = res;
			return res;
		} else { // For instances, a copy must be made.
			Ref<Resource> local_dupe = res->duplicate_for_local_scene(base, p_resources_local_to_scene);
			p_resources_local_to_scene[res] = local_dupe;
			return local_dupe;
		}
	}
}

EDIT: the scene does not save the resource locally, maybe the issue is somewhere else?
This is the only info in the .tscn:
[node name="Node2D" parent="." instance=ExtResource("1_6km2f")]

[Wyxaldir]

Getting more specific: the issue seems to be that the default value for the array is not properly referencing the value from the parent scene:

// packed_scene.cpp
if (!pinned_props.has(name)) {
    bool is_valid_default = false;
    // FIXME: the default_value is the current value rather than the value in the parent scene
    Variant default_value = PropertyUtils::get_property_default_value(p_node, name, &is_valid_default, &states_stack, true);
        if (is_valid_default && !PropertyUtils::is_property_value_different(value, default_value)) { 
            continue;
        }
}

I believe the error is in the PropertyUtils::get_node_states_stack method, but this is getting way over my head unfortunately.

[AThousandShips]

Is this intended to be a work in progress currently? If so you might want to convert it to a draft 🙂

[KoBeWi]

Looks better now, but something is wrong with Dictionaries. When you have both key and value local to scene, they double when loading the scene

Also you should remove the commented out code.

[KoBeWi]

Works correctly now. You still need to remove the commented out code as I mentioned.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉