Add ArrayMesh::surface_remove

[ze2j]

Fixes #67181

Test project:
remove_surface_test_project.zip

[ze2j]

I rebased and fixed some issues when using blend shapes.

Note: when the surface count reaches zero, adding or clearing blend shapes is enabled again.

I tested the 3 renderers, the removal of surfaces with or without blend shapes and the ability to add new blend shapes when the surface count is back to 0.

[ze2j]

Rebased to resolve conflicts

[AThousandShips]

You've also modified the indentation of servers/rendering/renderer_rd/storage_rd/mesh_storage.h, it will fail CI, please restore it.

[ze2j]

Sorry about the indentation, it's fixed now

[ze2j]

Rebased to resolve conflicts

[ze2j]

I don't understand why the tests I added some version ago, fail on windows. They never failed so far and I don't have a windows to reproduce the issue.

If someone is able to reproduce it locally I would appreciate any help.

ps: I noticed the local var cylinder_aabb should be renamed in the first test case but it's not worth a push yet.

[GreenCrowDev]

@ze2j I'm interested in helping resolve this issue, and I have a windows machine. I'm pretty unexperienced though, can you help me with some instruction to test and give you the failure information you need?

[ze2j]

I need to make some fixes for blend shapes as this branch doesn't work anymore when rebased on 4.2. I set the PR to draft in the meantime.

Edit: Nothing wrong with blendshapes. My test scene had to be updated for 4.2, I remove the draft status.
Edit2: Still in draft because of the unit test failure on windows. Sorry for the spam

@matricola787 Thanks for offering your help! I am not sure if I need to give more or less details below, so let me know if I am completely off.

You need to compile my branch with target=editor dev_build=yes tests=true parameters. The branch can be rebased on top of the 4.2 version without conflict.

Then run the editor with --test from a terminal to see the stdout. If there is no error you will get something like (this is what I got with 4.2):

ERROR: Resource was not pre cached for the resource section, bug?
   at: _write_resource (scene/resources/resource_format_text.cpp:1853)
WARNING: Couldn't load resource (no cache): local://Resource_hbjy0
     at: parse_variant (core/io/resource_format_binary.cpp:412)
[doctest] doctest version is "2.4.11"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:     837 |     837 passed | 0 failed | 1 skipped
[doctest] assertions: 2390349 | 2390349 passed | 0 failed |
[doctest] Status: SUCCESS!
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object/object.cpp:2208)

If there is an error, you should see the same error as the Github action:

 ===============================================================================
.\tests/scene/test_arraymesh.h(295):
TEST CASE:  [SceneTree][ArrayMesh] Get/Set mesh metadata and actions
  Remove the first surface and check the mesh's AABB.

.\tests/scene/test_arraymesh.h(404): ERROR: CHECK( mesh->get_aabb().is_equal_approx(cylinder_aabb) ) is NOT correct!
  values: CHECK( false )

===============================================================================
.\tests/scene/test_arraymesh.h(295):
TEST CASE:  [SceneTree][ArrayMesh] Get/Set mesh metadata and actions
  Remove the last surface and check the mesh's AABB.

.\tests/scene/test_arraymesh.h(412): ERROR: CHECK( mesh->get_aabb().is_equal_approx(cylinder_aabb) ) is NOT correct!
  values: CHECK( false )

===============================================================================
[doctest] test cases:     839 |     838 passed | 1 failed | 1 skipped
[doctest] assertions: 2389667 | 2389665 passed | 2 failed |
[doctest] Status: FAILURE!
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: ObjectDB::cleanup (core\object\object.cpp:2194)

The failing test is tests/scene/test_arraymesh.h at line 412. What I would like to know is the exact value of both mesh->get_aabb() and cylinder_aabb.

I suspect they are almost the same but differ for more than 1e-5.

[GreenCrowDev]

@ze2j Thank you for the instructions! I managed to compile the rebased project as you said, but I'm not able to execute the tests: when from command line I run godot.windows.editor.dev.x86_64.exe --test I get this output (I hope it's safe to share this, correct me if I make any mistakes):

D:\Programmazione\Git Clones\godot\bin>[doctest] doctest version is "2.4.11"
[doctest] run with "--help" for options
Could not load project settings.
ERROR: Could not open specified test directory.
   at: (modules\gdscript\tests\gdscript_test_runner.cpp:319)
===============================================================================
D:\Programmazione\Git Clones\godot\modules\gdscript\tests\gdscript_test_runner_suite.h(44):
TEST SUITE: [Modules][GDScript]
TEST CASE:  Script compilation and runtime

modules\gdscript\tests\gdscript_test_runner.cpp(190): FATAL ERROR: An error occurred while making the tests.

D:\Programmazione\Git Clones\godot\modules\gdscript\tests\gdscript_test_runner_suite.h(49): FATAL ERROR: REQUIRE( fail_count == 0 ) is NOT correct!
  values: REQUIRE( -1 == 0 )
  logged: Make sure `*.out` files have expected results.
          All GDScript tests should pass.

===============================================================================
D:\Programmazione\Git Clones\godot\modules\gdscript\tests\test_lsp.h(391):
TEST SUITE: [Modules][GDScript][LSP]
TEST CASE:  [workspace][resolve_symbol]

D:\Programmazione\Git Clones\godot\modules\gdscript\tests\test_lsp.h(90): FATAL ERROR: REQUIRE( err == OK ) is NOT correct!
  values: REQUIRE( 31 == 0 )
  logged: Could not open specified root directory


================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.1.rc.custom_build (0bf12956a10d216c46368f26b00f6999d000c1d8)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] GDScriptTests::initialize (D:\Programmazione\Git Clones\godot\modules\gdscript\tests\test_lsp.h:91)
[1] GDScriptTests::DOCTEST_ANON_SUITE_7464::DOCTEST_ANON_FUNC_7474 (D:\Programmazione\Git Clones\godot\modules\gdscript\tests\test_lsp.h:392)
[2] doctest::Context::run (D:\Programmazione\Git Clones\godot\thirdparty\doctest\doctest.h:7008)
[3] test_main (D:\Programmazione\Git Clones\godot\tests\test_main.cpp:202)
[4] Main::test_entrypoint (D:\Programmazione\Git Clones\godot\main\main.cpp:725)
[5] widechar_main (D:\Programmazione\Git Clones\godot\platform\windows\godot_windows.cpp:163)
[6] _main (D:\Programmazione\Git Clones\godot\platform\windows\godot_windows.cpp:204)
[7] main (D:\Programmazione\Git Clones\godot\platform\windows\godot_windows.cpp:218)
[8] WinMain (D:\Programmazione\Git Clones\godot\platform\windows\godot_windows.cpp:232)
[9] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[10] <couldn't map PC to fn name>
-- END OF BACKTRACE --
================================================================
D:\Programmazione\Git Clones\godot\modules\gdscript\tests\test_lsp.h(391): FATAL ERROR: test case CRASHED: SIGSEGV - Segmentation violation signal

===============================================================================
[doctest] test cases:  5 |  3 passed | 2 failed | 833 skipped
[doctest] assertions: 52 | 49 passed | 3 failed |
[doctest] Status: FAILURE!
ERROR: BUG: Unreferenced static string to 0: current_animation_changed
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: BUG: Unreferenced static string to 0: interface_added
   at: StringName::unref (core\string\string_name.cpp:129)
ERROR: BUG: Unreferenced static string to 0: node_changed
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: BUG: Unreferenced static string to 0: animation_player_changed
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: BUG: Unreferenced static string to 0: animation_libraries_updated
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: BUG: Unreferenced static string to 0: animation_list_changed
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: BUG: Unreferenced static string to 0: caches_cleared
   at: StringName::unref (core\string\string_name.cpp:127)
ERROR: Pages in use exist at exit in PagedAllocator: union Variant::Pools::BucketLarge
   at: PagedAllocator<union Variant::Pools::BucketLarge,1>::~PagedAllocator (D:\Programmazione\Git Clones\godot\core/templates/paged_allocator.h:170)
ERROR: Pages in use exist at exit in PagedAllocator: union Variant::Pools::BucketMedium
   at: PagedAllocator<union Variant::Pools::BucketMedium,1>::~PagedAllocator (D:\Programmazione\Git Clones\godot\core/templates/paged_allocator.h:170)
ERROR: Pages in use exist at exit in PagedAllocator: union Variant::Pools::BucketSmall
   at: PagedAllocator<union Variant::Pools::BucketSmall,1>::~PagedAllocator (D:\Programmazione\Git Clones\godot\core/templates/paged_allocator.h:170)
ERROR: Profiler not registered: multiplayer:bandwidth
   at: (core\debugger\engine_debugger.cpp:54)
ERROR: Profiler not registered: multiplayer:rpc
   at: (core\debugger\engine_debugger.cpp:54)
ERROR: Profiler not registered: multiplayer:replication
   at: (core\debugger\engine_debugger.cpp:54)

I'm not sure what's wrong, I guess I need some tips to get on the right direction.
What am I missing?

Edit: I tried the same process on a clean 4.2-stable clone, and the output is pretty much the same, so I guess that's on my end, but I don't know what I should touch.

[AThousandShips]

(my bad the page updated and I clicked the wrong button)

[ze2j]

Rebased on top of 4.2, ready for review.

[AThousandShips]

This hasn't been approved yet, hopefully it can be soon, we don't have any schedule for this one, please have patience, you can use this for your own projects by cloning the branch

However first it needs a rebase and the conflicts need to be fixed

[ze2j]

@AThousandShips In the meantime, is it worth rebasing it every time a conflict occurs or should I wait being asked to?

[AThousandShips]

I'd say to fix them when they pop up to make it easier and not have it grow on you, as long as it's just when conflicts occur, it helps ability to review and test it (so people who test it don't have to do it themselves)

[clayjohn]

Just popping in to add some context. This is very much on my radar and should be approved soon. The PR was only finished a few weeks ago, and I have been super busy with the release of 4.2, and preparing 4.2.1, so I haven't found the time to review it yet. Now I am catching up on my 4.3 backlog and so should have a chance to review it soon.

This is definitely a wanted feature and its very nice to see it contributed.

@ze2j On the topic of rebasing. My personal view is that you should wait until you have received a technical review to rebase. I don't want you to endlessly rebase the PR while waiting for me, that's just not fair to you, and its no benefit to me either.

[GreenCrowDev]

Sorry to bother you @clayjohn, any update about this?

[Calinou]

@ze2j Could you look into rebasing this PR on top of master to fix merge conflicts? Thanks in advance 🙂

[ze2j]

@Calinou I am on it

[ze2j]

Rebased on top of master. I tested the removal of surfaces with or without blend shapes on both vulkan and opengl backends.

[Calinou]

Rebased on top of master. I tested the removal of surfaces with or without blend shapes on both vulkan and opengl backends.

Can you upload the testing project to make testing this PR easier?

[ze2j]

@Calinou I added it in the first post

[clayjohn]

Looks great! Tested locally and confirmed that it was working correctly.

I would have liked to merge this earlier, but I ended up needed to do a very careful review of the surrounding code. For context, surface_remove() was removed in 4.0. At the time we thought it couldn't be implemented given the new rendering architecture. However, none of us can remember what the exact issue was and the function was removed during Juan's big rewrite, so there are no PRs/Commits to consult. Accordingly, I had to carefully review all the surrounding code and see if there is something we are missing that would cause an issue for this function.

Ultimately, I haven't found any issues and the code looks fantastic. So I think we should go ahead and merge this PR. Its possible we will discover the reason for removing it after putting this in the hands of users, but at this point I think its worth the risk.

[AThousandShips]

Suggested change

	Removes a surface at position surf_idx, shifting greater surfaces one surf_idx slot down.
	Removes the surface at the given index from the Mesh, shifting surfaces with higher index down by one.

[AThousandShips]

@clayjohn @Repiteo These changes should really not have been dismissed just because there was disagreement over codestyle changes, please pay attention to all suggested changes and don't skip some ones by relevant teams

[AThousandShips]

Suggested change

	Removes a mesh's surface at position surf_idx, shifting greater surfaces one surf_idx slot down.
	Removes the surface at the given index from the Mesh, shifting surfaces with higher index down by one.

[Repiteo]

Thanks!

The style changes can be adjusted in a separate PR.