godotengine / godot

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

CSGMesh is not updated when it's properties are changed #46610

Open Einlander opened 3 years ago

Einlander commented 3 years ago

v3.2.4.rc3.official

Windows 10 PRO; Radeon RX 570 8GB; Driver 20.12.1; GLES3

Issue description: CSGMesh is not updated when it's properties are changed

Steps to reproduce:

  1. Add a CSGMesh to the scene.
  2. Change the mesh property. Some of them show up immediatly, some do not.
  3. If the mesh is not visible, changing to a different tab and coming back will fix it.
    • If you undo changing the mesh change, the meshes wireframe stays visible.

Minimal reproduction project:

https://user-images.githubusercontent.com/12641229/109754737-3643c580-7baa-11eb-84af-58d115801c8d.mp4

gongpha commented 3 years ago

Regression from https://github.com/godotengine/godot/commit/e8804b9978a4bd2647e1367bc2258a19d73dcc95. And caused on chosen CubeMesh, PlaneMesh, PrismMesh or QuadMesh

akien-mga commented 3 years ago

Is it reproducible in 3.2.4 RC 5? Parts of #39635 were missing in 3.2.4 RC 3, added in RC 4.

CC @Meriipu

gongpha commented 3 years ago

It can reproduce it from 3.2.4 beta2 to 3.2.4 RC 5.

I have no idea about this commit is about viewport input will breaking mesh generating.

akien-mga commented 3 years ago

Regression from e8804b9. And caused on chosen CubeMesh, PlaneMesh, PrismMesh or QuadMesh

Tested and I can confirm the regression seems to be caused by e8804b9 (reverting that commit solves it), even though I really don't see the link between the two :|

Probably causes the 3D viewport not to be properly refreshed as it should be. Maybe here: https://github.com/godotengine/godot/blob/8dd6fd058a2c960c820ad17dcde53efeb3d65d14/editor/plugins/spatial_editor_plugin.cpp#L4148-L4152

CC @Meriipu @RandomShaper @JFonS

akien-mga commented 3 years ago

BTW this error is raised when adding a CubeMesh as mesh to a CSGMesh:

ERROR: mesh_surface_get_array: Index p_surface = 0 is out of bounds (mesh->surfaces.size() = 0).
   At: drivers/gles3/rasterizer_storage_gles3.cpp:4084.
ERROR: mesh_surface_get_arrays: Condition "vertex_data.size() == 0" is true. Returned: Array()
   At: servers/visual_server.cpp:1595.
ERROR: _build_brush: Condition "arrays.size() == 0" is true. Returned: _post_initialize(new ("") CSGBrush)
   At: modules/csg/csg_shape.cpp:738

There's also some more errors when adding or updating a CSGMesh though these don't seem caused by e8804b9 (also happen if reverted) and might not be linked to this issue:

ERROR: mesh_add_surface_from_arrays: Condition "array_len == 0" is true.
   At: servers/visual_server.cpp:965.
ERROR: add_surface_from_arrays: Condition "len == 0" is true.
   At: scene/resources/mesh.cpp:851.
akien-mga commented 3 years ago

For the reference, this issue doesn't seem reproducible in master (which also had the changes from #39635), maybe the regression was fixed there or did not happen in the first place.

madmiraal commented 3 years ago

This is a multi-thread syncing issue.

The multi-threading allows calls to the VisualSever to be queued and return before they are completed, and subsequent calls to be made before previously calls have completed. This causes a problem with simple Primitive Meshes here: https://github.com/godotengine/godot/blob/5942a705964e539239989beacdfd92bbc491721e/scene/resources/primitive_meshes.cpp#L87-L90 The mesh_add_surface_from_arrays() returns and pending_request is set to false before it has created the surface on the RasterizerStorage: https://github.com/godotengine/godot/blob/5942a705964e539239989beacdfd92bbc491721e/drivers/gles3/rasterizer_storage_gles3.cpp#L3977-L3978 If an attempt is made to retrieve the array before the surface is created, if fails with

ERROR: mesh_surface_get_array: Index p_surface = 0 is out of bounds (mesh->surfaces.size() = 0).
   At: drivers/gles2/rasterizer_storage_gles2.cpp:2756.

because there is nothing to retrieve yet: https://github.com/godotengine/godot/blob/5942a705964e539239989beacdfd92bbc491721e/scene/resources/primitive_meshes.cpp#L129-L136 Ironically, it "works" with more complex meshes, because the call to create the mesh doesn't reach the call to add the surface before the call to retrieve the array is made; so pending_request is still true. This causes a second (unnecessary) call to be made to update the mesh which doesn't return before the first call to update the mesh has finished creating the surface.

I suspect, e8804b9 exposed this preexisting bug, by bringing forward the call to retrieve the array, called by the CSGShapeSpatialGizmoPlugin: https://github.com/godotengine/godot/blob/5942a705964e539239989beacdfd92bbc491721e/modules/csg/csg_gizmos.cpp#L318

This happens on 3.3 with both GLES2 and GLES3. It probably doesn't happen on master because of the entire VisualServer rewrite.