Error Condition "p_value.get_type() != Variant::BOOL" is true. Returning: false on opening editor with project in 4.3

[chrisl8]

Tested versions

• Reproducible in v4.3.dev.custom_build [bd8380d] and all subsequent commits including master
  bd8380d
• Not Reproducible in v4.3.dev.custom_build [c5e6a58]
  c5e6a58

System information

Godot v4.3.dev (bd8380d) - Windows 10.0.22621 - GLES3 (Compatibility) - NVIDIA GeForce RTX 3080 Ti (NVIDIA; 31.0.15.5161) - AMD Ryzen 9 5900X 12-Core Processor (24 Threads)

Issue description

Starting with v4.3.dev.custom_build [bd8380d] bd8380d Godot appears to throw this error once for each synced variable contained within any MultiplayerSynchronizer in an open scene:
modules\multiplayer\scene_replication_config.cpp:59 - Condition "p_value.get_type() != Variant::BOOL" is true. Returning: false

Steps to reproduce

gh repo clone chrisl8/space-game
Open it up with 4.3 bd8380d or newer.
Open up the player folder
Open the player scene

Minimal reproduction project (MRP)

I was not able to reproduce this with any basic startup project or Godot demo project.
You can clone my project from here and the error is consistent:
https://github.com/chrisl8/space-game

Then open up the player.tscn inside of the player folder.
It also happens with any other scene with a MultiplayerSynchronizer in it such as most of the scenes in folders under things.

[mr-dreich]

@akien-mga I have moved this issue to: 89066

[akien-mga]

@mr-dreich Your error doesn't appear to be related to the OP. Could you open a new issue?

Edit: I see it mentions MultiplayerAPI in the stacktrace, but this is a bug of the crash handler, it's hallucinating symbols in a build stripped of debug symbols. The crash is likely not related at all, as the other stack frames also make no sense. You would need to compile from source with debug_symbols=yes to get better info.

[Faless]

It seems that something changed in the way we set the initial values of resources from a file, and they are now being set twice.
The first time, it all goes well, the second time, the consistency check in the setter now fails.

Adding some prints, it's clear that _set is being called 2 times for each properties.

We can probably play around with the check, though this seems like a regression in resource loading?

WARNING: Prop num: 0, idx: 0, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 1, idx: 0, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 1, idx: 0, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 1, idx: 1, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 2, idx: 1, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 2, idx: 1, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 2, idx: 2, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 3, idx: 2, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 3, idx: 2, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)

[.....]

WARNING: Prop num: 8, idx: 0, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
ERROR: Invalid value '.:player' for property 'properties/0/path'
   at: _set (modules/multiplayer/scene_replication_config.cpp:60)
WARNING: Prop num: 8, idx: 0, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 8, idx: 0, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 8, idx: 1, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
ERROR: Invalid value '.:position' for property 'properties/1/path'
   at: _set (modules/multiplayer/scene_replication_config.cpp:60)
WARNING: Prop num: 8, idx: 1, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 8, idx: 1, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 8, idx: 2, what: path
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
ERROR: Invalid value '.:linear_velocity' for property 'properties/2/path'
   at: _set (modules/multiplayer/scene_replication_config.cpp:60)
WARNING: Prop num: 8, idx: 2, what: spawn
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)
WARNING: Prop num: 8, idx: 2, what: replication_mode
     at: _set (modules/multiplayer/scene_replication_config.cpp:43)

[...]

EDIT: Using this patch to get the print:

diff --git a/modules/multiplayer/scene_replication_config.cpp b/modules/multiplayer/scene_replication_config.cpp
index 836fa1014d..10199107a7 100644
--- a/modules/multiplayer/scene_replication_config.cpp
+++ b/modules/multiplayer/scene_replication_config.cpp
@@ -40,6 +40,7 @@ bool SceneReplicationConfig::_set(const StringName &p_name, const Variant &p_val
 		int idx = prop_name.get_slicec('/', 1).to_int();
 		String what = prop_name.get_slicec('/', 2);
 
+		WARN_PRINT(vformat("Prop num: %d, idx: %d, what: %s", properties.size(), idx, what));
 		if (properties.size() == idx && what == "path") {
 			ERR_FAIL_COND_V(p_value.get_type() != Variant::NODE_PATH, false);
 			NodePath path = p_value;
@@ -56,7 +57,7 @@ bool SceneReplicationConfig::_set(const StringName &p_name, const Variant &p_val
 			property_set_replication_mode(prop.name, mode);
 			return true;
 		}
-		ERR_FAIL_COND_V(p_value.get_type() != Variant::BOOL, false);
+		ERR_FAIL_COND_V_MSG(p_value.get_type() != Variant::BOOL, false, vformat("Invalid value '%s' for property '%s'", p_value, p_name));
 		if (what == "spawn") {
 			property_set_spawn(prop.name, p_value);
 			return true;

[akien-mga]

Tested versions

• Reproducible in v4.3.dev.custom_build [bd8380d] and all subsequent commits including master
  bd8380d

• Not Reproducible in v4.3.dev.custom_build [c5e6a58]
  c5e6a58

This is the comparison between those two commits: c5e6a58...bd8380d

I tested a local revert and confirmed that this is caused by #85501.

CC @Jordyfel @adamscott

[Jordyfel]

godot/core/io/resource.h

Line 96 in f2045ba

virtual void reset_state(); //for resources that use variable amount of properties, either via _validate_property or _get_property_list, this function needs to be implemented to correctly clear state

@Faless it seems SceneReplicationConfig will need to implement this function.

Doing it lazily like so fixes the issue.

void SceneReplicationConfig::reset_state() {
	properties.clear();
	sync_props.clear();
	spawn_props.clear();
	watch_props.clear();
}

[Faless]

@Jordyfel thanks! I had no clue that function existed , I've opened #89108 with your suggested change.

[alexeocto]

I am getting the same error on 4.2.

Should I just upgrade to 4.3 to take advantage of the proposed fix in #89108.

[Jordyfel]

@alexeocto I've come across this error in 4.2 as well, and since the PR hasn't been cherry-picked, the only way to get rid of it is by upgrading to 4.3, yes