godotengine / godot

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

[4.4 dev 4] RenderingServer.InstanceCreate() with large instance count triggers seg fault #99162

Open MilesRomanum opened 5 days ago

MilesRomanum commented 5 days ago

Tested versions

System information

Windows 10 and 11 - Godot 4.4 dev 4 - Ryzen 5800x3d + GTX 3080ti & Ryzen 7435HS + GTX 4060

Issue description

Calls made for RenderingServer.InstanceCreate started failing in 4.4 dev4 with the following exception message: Exception thrown at 0x00007FF6C409E72A in Godot_v4.4-dev4_mono_win64.exe: 0xC0000005: Access violation writing location 0x00000000000001A0.

image

Call stack looks like this.

Steps to reproduce

Try to build and launch the project attached below It will launch quickly in 4.4 dev 3 It will crash in 4.4 dev 4

The instance count is essential for achieving the gameplay, effect & scale of my game. And it actually works quite well in 4.4 dev3, (the none minimal repro project) used to run at 150fps fully animated on my 3080ti/5800x3d build.

Minimal reproduction project (MRP)

renderingservertest.zip

justofisker commented 5 days ago

Bisected to 08947d366fba19fe28733538eec0dafe11304032

justofisker commented 5 days ago

Also made a non-mono mrp.zip

RandomShaper commented 5 days ago

In 4,4, a thread-safe RID_Owners has a limit on the amount of RIDs it can hold, not changeable at runtime. The constructor has a parameter for that, that defaults to 262144.

That's exactly the case of the RenderingSceneCull::instance_owner, the one relevant to the MRP. The MRP tries to create 1.2 M instances, which is above the limit. If the MRP tried to create 262144 or less instances, the project would run fine.

If a real-world project really needs to instantiate that many objects, we could add a project setting. It this is a purely theoretical test, I'd just close this as a no-issue.

MilesRomanum commented 4 days ago

That makes sense. How can I set it as a project setting? I do need it to animate around 30k humanoids with 30 joints and composite models {for clothes on top of the base model)

MilesRomanum commented 4 days ago

Fwiw, I think the default limit of 256k is definitely on the low side for a pc game. Even with my project that instanciate the visualinstance with a mid poly model 30k times, the memory usage only got to 6gig which is well within spec for a typical pc game.

clayjohn commented 4 days ago

@MilesRomanum A typical object count budget for AAA games is in the low tens of thousands. When you start needing more than that you should be exploring ways of batching together work. For example, using MultiMeshInstance or particles to control thousands of instances at once.

Also consider using multiple mesh surfaces instead of multiple instances for composite models. By using individual instances for each item you will be guaranteeing performance issues

MilesRomanum commented 4 days ago

I can probably use multi-mesh surface for composite models. But regarding object count budget, my game resolves around modelling armies of npc with animations. instancing limbs of npc has been how i make it run with a good framerate, and with some lod and animation sampling techniques it actually works really well with rendering server. This vid below is an example of the game in motion. There is a lot more off screen of course, but fwiw, it's running at 170fps so Godot can clearly handle it. https://github.com/user-attachments/assets/e877d950-6ca8-405e-8512-cfc3bf095e29 s

I want to be able to allow artists to create their animations into gltf formats, which I think would be difficult with the multimeshinstance or particles

I saw the commit that changed it and I understand why it is in place. Would there be a way to make the rid_owner max configurable? I suppose worst case would be me learning to compile just changing that constant, but would appreciate it if it can be configured because I think there might be other devs who also appreciate it.

MilesRomanum commented 4 days ago

I also dont mind using the not thread safe version of rendering server if that's available. I already do a long of batching and sycnrhonization in my own code to make sure renderingserver only get called on a single thread. Is there a flag to make renderingserver not thread safe? thanks

RandomShaper commented 4 days ago

The unsafe thread model for rendering was never a thing in Godot 4. In fact, I've made a PR that makes that explicit: #98383.

That said, it would be technically possible to bring it back. However, that would add some extra burden. For instance, already very complex areas of the engine, such as the ResourceLoader, would require extra logic . My point, ultimately, is that we won't likely want to support something like that unless it's deemed worth it.

(For the records, there's an alternative implementation path that may make things a little easier, if still complex. I'm leaving it here for the records. It would be optimizing the engine for threadless builds in a way that it assumes everything happens on the same thread. That way, everything would be "unsafe" by default and it'd only be a matter of still allowing such kind of build to spawn threads. Again, the risk-benefit statement stands.)

MilesRomanum commented 4 days ago

Thats fair. If it is possible (and not require additional long term support), Im just interested in the simplest way for an end user like me to be able to allocate a bit more than the initial default.

The approach may be unorthodox, but it has allowed me to have really good performance doing things this way, leveraging multitheading and caching for skeletal animations. If there is a simple and cheap way for this to be supported, I would be immensely grateful

RandomShaper commented 4 days ago

A project setting seems like a good compromise, I'd say.

justofisker commented 3 days ago

I would love to give making this change a shot. I want to get into developing for the engine and this seems like a simple enough PR to work on!