godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.1k stars 21.18k forks source link

Scenes with 3D MultiMeshes print errors on load #95617

Open 0x0ACB opened 2 months ago

0x0ACB commented 2 months ago

Tested versions

4.3

System information

Windows 11

Issue description

https://github.com/godotengine/godot/blob/ee363af0ed65f6ca9a906f6065d0bd9c19dcf9e4/scene/resources/multimesh.cpp#L309 prints an error if the instance_count is set before set_transform_format. This will be true for any non empty 3D MultiMesh stored in a scene file.

Steps to reproduce

Add a 3D MultiMesh with some instances to a scene. Save scene. Reopen scene. <scene\resources\multimesh.cpp(309): MultiMesh::set_transform_format> Condition "instance_count > 0" is true.

Minimal reproduction project (MRP)

-

tetrapod00 commented 2 months ago

Setting the format before the instance count is required, see here.

But this may still be a bug - should this be a case where a C++ error is surfaced?

Maybe transform_format's description should also have a note about the order it needs to be called, like instance_count does.

Edit: Sorry, I misread the issue!

0x0ACB commented 2 months ago

Setting the format before the instance count is required, see here.

But this may still be a bug - should this be a case where a C++ error is surfaced?

Maybe transform_format's description should also have a note about the order it needs to be called, like instance_count does.

Edit: Sorry, I misread the issue!

Means this is a more fundamental issue. There can be no hard initialization order requirements for properties since the initialization order during load is practically undefined. Either there needs to be a way to define property dependencies for the loader or properties need to be designed in a way that initialization order does not matter.

clayjohn commented 2 months ago

I can't reproduce this issue in a new scene by following the reproduction steps.

Can you upload an MRP that consistently reproduces the issue?

0x0ACB commented 2 months ago

Sure here you go. This one actually breaks completely: multimesh.zip

I just created the MultiMesh via the scatter populate tool

clayjohn commented 2 months ago

Thanks for that. Looking at your MRP I can see that instance_count is stored as the first property while in mine it is stored under transform_format.

So we need to figure out what would change the order of properties when saving

0x0ACB commented 2 months ago

I would say its best to not rely on the order at all or implement a way to enforce the ordering in the class definition. Relying on the ResourceLoader/Saver to just randomly save and load in the correct order does not seem like a robust design.

aspectwaltz commented 1 week ago

I noticed this as well on v4.3.stable.mono.official.77dcf97d8

when in a tool script where I instantiate a MultiMesh with new() and create the mesh with surface_tool's commit(). When I reopen a scene the following errors appear, one for each of the settings on the multimesh and for each multimesh in the scene.

ERROR: Condition "instance_count > 0" is true.
   at: set_transform_format (scene/resources/multimesh.cpp:309)
ERROR: Condition "instance_count > 0" is true.
   at: set_use_colors (scene/resources/multimesh.cpp:291)
ERROR: Condition "instance_count > 0" is true.
   at: set_use_custom_data (scene/resources/multimesh.cpp:300)

However, I was unable to reproduce this when I isolate scripts into in an MRP and it doesn't appear to happen always... Is this issue related? 87166

I should also note that these errors are only thrown in editor, but not during run project or release