Regression in InstancePlaceholder behavior, instanced scene missing Node property values

[TheOrioli]

Tested versions

• Reproducible in 4.3.dev3
• Works as expected in 4.2.1

System information

Godot v4.3.dev3.mono - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 31.0.15.4633) - AMD Ryzen 9 3900X 12-Core Processor (24 Threads)

Issue description

When instantiating a node using InstancePlaceholder.create_instance() the newly instantiated node is missing exported Node properties.

This is the expected result ( viewed in Remote tab)

this is what happens instead

( number change is due to me changing it in between screenshots 😅)
Nested scenes are not affected and seem to instantiate with all properties set. Issue appears in both GDScript and C#.
Everything works as expected on 4.2.1

Steps to reproduce

Open the attached MRP, and run main.tscn
Check the Remote tab and see the properties missing.

For ease of comparison the Main node has a bool

which will trigger loading the same scene manually using InstancePlaceholder.get_instance_path() instead. This approach works fine.

Minimal reproduction project (MRP)

PlaceholderInstanceRepro.zip

[Ratamacue9112]

I think PR #83343 introduced the problem.

[TheOrioli]

Tested with the latest release, the issue is present on 4.3.dev4

[akien-mga]

I can confirm the issue described in the OP with 4.3-dev1 to 4.3-dev4 and current master.
A local revert of #83343 solves it, confirming #88662 (comment).

CC @warriormaster12 @KoBeWi @RandomShaper

[warriormaster12]

Reproduced the bug, I have this pr open #87387 that maybe will fix this issue. I’ll test at home if I can reproduce this bug with it. If I can then I’ll try to fix this issue in the same pr.

[akien-mga]

Reproduced the bug, I have this pr open #87387 that maybe will fix this issue. I’ll test at home if I can reproduce this bug with it. If I can then I’ll try to fix this issue in the same pr.

Tested #87387, it doesn't seem to solve this bug currently.

[warriormaster12]

In that case I’ll have to integrate the fix into the pr

[warriormaster12]

godot/scene/property_utils.cpp

Line 84 in f2045ba

if (deferred_properties.has(p_property)) {

Found the place that causes this regression. Commenting the if-statement fixes it though it introduces another bug in is_property_value_different function where a == Nodepath and b == Object. However, as far as I know it's only related to ui so maybe it could be removed until a better solution is found?

Edit: It will brake node duplication since it relies on variant’s type being object.

[Mickeon]

What's progress on a solution for this issue?

[warriormaster12]

Not at the moment. I haven't had that mich to take a look further how to resolve the issue. I only know that somewhere the value changes to null because of the changes that I have made in property_utils.cpp

[TheOrioli]

It seems there are a couple of problems with InstancePlaceholders being instantiated.
When instantiating a PackedScene that contains an InstancePlaceholder a couple of things will happen:

1. Node references and Node Arrays will be deferred and will attempt to resolve. This will result in null nodes, since they don't exist yet.
   

   godot/scene/resources/packed_scene.cpp

   Lines 316 to 325 in 7a42afb

   	if (nprops[j].name & FLAG_PATH_PROPERTY_IS_NODE) {
   	uint32_t name_idx = nprops[j].name & (FLAG_PATH_PROPERTY_IS_NODE - 1);
   	ERR_FAIL_UNSIGNED_INDEX_V(name_idx, (uint32_t)sname_count, nullptr);
   	DeferredNodePathProperties dnp;
   	dnp.value = props[nprops[j].value];
   	dnp.base = node;
   	dnp.property = snames[name_idx];
   	deferred_node_paths.push_back(dnp);
   	continue;

   
   

   godot/scene/resources/packed_scene.cpp

   Lines 511 to 528 in 7a42afb

   	for (const DeferredNodePathProperties &dnp : deferred_node_paths) {
   	// Replace properties stored as NodePaths with actual Nodes.
   	if (dnp.value.get_type() == Variant::ARRAY) {
   	Array paths = dnp.value;
   	bool valid;
   	Array array = dnp.base->get(dnp.property, &valid);
   	ERR_CONTINUE(!valid);
   	array = array.duplicate();
   	array.resize(paths.size());
   	for (int i = 0; i < array.size(); i++) {
   	array.set(i, dnp.base->get_node_or_null(paths[i]));
   	}
   	dnp.base->set(dnp.property, array);
   	} else {
   	dnp.base->set(dnp.property, dnp.base->get_node_or_null(dnp.value));
   	}

   
   This will wrongly set null values in the InstancePlaceholder

   godot/scene/main/instance_placeholder.cpp

   Lines 36 to 42 in 7a42afb

   	bool InstancePlaceholder::_set(const StringName &p_name, const Variant &p_value) {
   	PropSet ps;
   	ps.name = p_name;
   	ps.value = p_value;
   	stored_values.push_back(ps);
   	return true;
   	}

2. Typed NodePath arrays will not have their type set, while e.g. int and string arrays will. This will cause a failure when type checking the array during PackedScene instantiation

   godot/scene/resources/packed_scene.cpp

   Lines 356 to 369 in 7a42afb

   	if (value.get_type() == Variant::ARRAY) {
   	Array set_array = value;
   	value = setup_resources_in_array(set_array, n, resources_local_to_sub_scene, node, snames[nprops[j].name], resources_local_to_scene, i, ret_nodes, p_edit_state);
   	bool is_get_valid = false;
   	Variant get_value = node->get(snames[nprops[j].name], &is_get_valid);
   	if (is_get_valid && get_value.get_type() == Variant::ARRAY) {
   	Array get_array = get_value;
   	if (!set_array.is_same_typed(get_array)) {
   	value = Array(set_array, get_array.get_typed_builtin(), get_array.get_typed_class_name(), get_array.get_typed_script());
   	}
   	}
   	}

   and later cause a failure when attempting to set the array property value, since type checking of an untyped Array automatically fails

   godot/modules/gdscript/gdscript_function.h

   Lines 75 to 100 in 7a42afb

   	case BUILTIN: {
   	Variant::Type var_type = p_variant.get_type();
   	bool valid = builtin_type == var_type;
   	if (valid && builtin_type == Variant::ARRAY && has_container_element_type(0)) {
   	Array array = p_variant;
   	if (array.is_typed()) {
   	GDScriptDataType array_container_type = get_container_element_type(0);
   	Variant::Type array_builtin_type = (Variant::Type)array.get_typed_builtin();
   	StringName array_native_type = array.get_typed_class_name();
   	Ref<Script> array_script_type_ref = array.get_typed_script();
   	if (array_script_type_ref.is_valid()) {
   	valid = (array_container_type.kind == SCRIPT || array_container_type.kind == GDSCRIPT) && array_container_type.script_type == array_script_type_ref.ptr();
   	} else if (array_native_type != StringName()) {
   	valid = array_container_type.kind == NATIVE && array_container_type.native_type == array_native_type;
   	} else {
   	valid = array_container_type.kind == BUILTIN && array_container_type.builtin_type == array_builtin_type;
   	}
   	} else {
   	valid = false;
   	}
   	} else if (!valid && p_allow_implicit_conversion) {
   	valid = Variant::can_convert_strict(var_type, builtin_type);
   	}
   	return valid;
   	} break;

This is as far as I've gotten with this for now. But it's smelling more and more to me that InstancePlaceholders will need a bit more information to properly set everything up. Naturally, I barely know what I'm doing 🤷‍♂️ Here is my little experimental fork that works with internal node references, but breaks on arrays and external nodes TheOrioli@fe34765

Here is a modified test project that includes even more test cases. I've added both external node references, as well as Node and Nodepath arrays. One of the arrays is mixed, containing both internal nodes and external nodes. As a user I would expect all those references to remain.
PlaceholderInstanceRepro.zip