godotengine / godot

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

Crash when configuring the import of a gltf file and enabling Physics in a MeshInstance3D and switching body and shape types #85439

Closed Dancovich closed 11 months ago

Dancovich commented 11 months ago

Godot version

v4.2.rc2.official [1ba920fad]

System information

Windows 11, Intel Core i5 10300H, nVidia GTX 1650

Issue description

When configuring the import of a gltf file, I'm trying to configure a mesh to use physics. When switching the body and shape type of this mesh, the engine will sometimes crash.

I couldn't detect the specific action that triggered the crash. Switching possible shape types appears to make the crash happen more often, but sometimes it will crash on the first shape type I choose and sometimes I can switch the shape type several times before it crashes.

The bug also happens in 4.2_RC1, but it's WAY easier to reproduce there. Sometimes merely checking Physics on will already trigger the crash. At most, doing one switch of body type (not even shape type) will also trigger the crash.

Steps to reproduce

It can take a few changes to shape type to trigger the bug in RC2, but in RC1 sometimes merely checking Physics on can trigger it.

Minimal reproduction project

Project to reproduce the bug.

bug_project.zip

Video of bug happening on my machine (I was "lucky" and it happened right away, sometimes it takes a few switches to shape type before it happens).

https://github.com/godotengine/godot/assets/1204296/797262fd-a881-4b8d-a6f8-2ed6aa593995

akien-mga commented 11 months ago

Tested on Mageia 9 Linux (KDE/X11, Radeon RX Vega M), I couldn't reproduce the crash in 4.2-rc1 or 4.2-rc2.

Might be a Windows-specific or driver specific issue.

Can you reproduce the issue in 4.1.3-stable, and on earlier 4.2 dev/beta snapshots?

Dancovich commented 11 months ago

Could reproduce it in 4.1.3. Behaved like 4.2_RC1 (instant crash, where in RC2 it takes more work to trigger the crash).

Also happened in 4.2 Beta 6, but it didn't happen right away. I had to change shape types a few times. Then I had the idea of deleting the .godot folder before testing and the bug happened right away when I changed the body type in the import settings window.

Then I had the idea of testing the same thing on RC2. I delete the .godot folder, open the project in RC2 and Godot re-imports all assets as usual, but if I try to configure the import and change the body and shape type, editor crashes immediately.

Isn't this bug related to the recent mingw bug that RC2 was supposed to fix?

Edit: Just to make it clear, the first time the bug happened to me I did nothing to the .godot folder. I don't think erasing this folder is a requirement, but it did make the bug easier to reproduce for me.

jsjtxietian commented 11 months ago

For me delete the .godot folder will trigger this crash, it's hit this assertion message located in the thirdparty vhacd:

https://github.com/godotengine/godot/blob/f82bf35a03502b33e0b3a5987573e3c6987912ce/thirdparty/vhacd/inc/vhacdVolume.h#L365

i = 63 which is probably fine but j = k = 9223372036854775808 which is insane.

akien-mga commented 11 months ago

Still can't reproduce after deleting the .godot folder, but this is like Windows/compiler specific.

The assert shouldn't trigger in official builds as we define NDEBUG, which should disable the assert macro and make it a no-op. But apparently this isn't working here. https://github.com/godotengine/godot/blob/f82bf35a03502b33e0b3a5987573e3c6987912ce/SConstruct#L440-L441

@YuriSizov also mentioned seeing this on Windows, though I can't find the bug report anymore, if it was filed.

And #60357 seems to be the same problem, reproducible on Linux too.

akien-mga commented 11 months ago

For the record, VHACD had a major rewrite as a 4.0 release last year (now 4.1): https://github.com/kmammou/v-hacd/releases

We're still on an older version of the library. Porting to the new version might help solve this, but that's a big undertaking as everything was rewritten and it's now a single header library.

We can likely also debug and find a workaround for this assert in the current version to have a safe fix rapidly.

YuriSizov commented 11 months ago

@YuriSizov also mentioned seeing this on Windows, though I can't find the bug report anymore, if it was filed.

It was discovered in https://github.com/godotengine/godot/issues/85201 with the Tilt5 test project https://github.com/Malcolmnixon/TiltFiveMapTest. I didn't get to extracting this problem into a separate issue, but I guess we're here now :)

godot windows editor dev x86_64_2023-11-22_14-15-51

It was consistently reproduceable on reimport of the project, at the same exact time in the process, so specific to some particular assets, though I never noted down which ones.

Edit: Note that the same project showed multiple but sporadic issues with the physics server. If this issue is related to physics, then https://github.com/godotengine/godot/issues/85217 may also be related to this validation check not passing on some assets/assets being invalid.

Malcolmnixon commented 11 months ago

I've found the following model reliably causes the crash for me windturbine_tall.zip from the https://godotengine.org/asset-library/asset/2124 assets.

The assert is caused by the ludicrous voxel index because it encounters a point smaller than the minimum bounding-box coordinate, resulting in a negative position which when cast to a size_t blows up to a ludicrous positive size. I'm debugging to try and find out why any point is outside the bounding box.

Malcolmnixon commented 11 months ago

I believe the issue is ImporterMesh::convex_decompose line 972: https://github.com/godotengine/godot/blob/d76c1d0e516fedc535a2e394ab780cac79203477/scene/resources/importer_mesh.cpp#L972-L974

The index is pre-incremented resulting in the vertex array being populated from 1 up to (and including) vertex_count. Then the vertices array is resized to vertex_count entries truncating the last entry.

My testing shows that changing this to a post-increment fixes the issue on my specific mesh and appears to populate the vertex array correctly; however I'd like confirmation it's not caused by the code containing a mix of 0-based and 1-based code and an intent to "pad" a leading zero.

yythlj commented 9 months ago

I import building glb from sketchfab,and it still crash when choose enable physix and select the type Is it has fix on version 4.2 or 4.3 I use both this two ver nouse

yythlj commented 9 months ago

like this glb, can rightly import and use, but if i Check Physics on, godot will no respond and only can kill process.My version is godot 4.2.1.The glb download in sketchfab.com and another glb occur the same problem too.Please help me. shop_house.zip

Malcolmnixon commented 9 months ago

Hi @yythlj. I'm not able to get it to "crash", but it does take a truly vast amount of time to update the colliders. Analyzing the model you provided (shop_house.glb) shows it consists of:

The vertices and triangles aren't a problem; but I don't think I've ever seen a model with this many discrete parts - every individual leaf, roof-tile, and stone is a separate object. Is this some kind of stress-test model? I can't imagine it doing anything other than causing graphics pipelines to scream in agony 😊

What I can say is that SceneImportSettingsDialog doesn't seem to handle this use-case well. From what I'm reading if ANY mesh setting changes on ANY node (even turning off physics collision) then generate_collider causes SceneImportSettingsDialog::_update_view_gizmos() to recalculating the colliders for EVERY node (in this case all 1176) even though only one node changed.

This behavior in SceneImportSettingsDialog::_update_view_gizmos() seems odd to me. Rather than hitting every node (by iterating through the node_map) to update the colliders; the selected_id field contains the name of the node being edited. I feel this could be an O(1) operation hitting just this node, rather than an O(n), but I didn't write this routine.

I did an ugly experimental hack to just update the one selected_id node, and to only calculate the collider when it wants to be shown. The result is an truly enormous speed-up for this use-case; however I'm not sure if I'm missing any complexities/intricacies around the behavior. This behavior was introduced by @AndreaCatania in https://github.com/godotengine/godot/pull/52266 so I think it would be best to consult with them to confirm if my analysis is missing anything.

https://github.com/godotengine/godot/assets/1863707/cd0ff51f-b5d3-4367-b9d9-9a8c644e8f71