godotengine / godotengine/godot

Instance not removed from global shader buffer when instanced material removed

Open
#110,141 1 comment 4 reactions 0 assignees View on GitHub
bug confirmed topic:rendering topic:shaders
Dominant language
C++
Stars
117k
Forks
26.8k
PR merge metrics
PR metrics pending

Description

### Tested versions

- Reproduced in v4.4.1.stable.mono.official [49a5bc7b6]

### System information

Godot v4.4.1.stable.mono unknown - Windows 10 (build 19045) - Multi-window, 3 monitors - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3090 (NVIDIA; 32.0.15.6109) - AMD Ryzen 9 5950X 16-Core Processor (32 threads)

### Issue description

Materials which use instance uniforms require space in the global uniform buffer for each instance using it, this is therefore a valuable resource which needs to be carefully controlled. When assigning the instance material the engine will reserve 16 slots in the buffer for the material. However when removing the material you don't get those slots back again until the entire node is freed.

This makes it impossible to control buffer usage by swapping to a non-instance material when no longer required.

I believe, but am not certain, that this is a regression from the following commit in which the instance uniform code was substantially refactored:

https://github.com/godotengine/godot/commit/ceefc0d38a04fe933f65a819e20172a60fc5cf03

In render_scene_cull.cpp it used to create a fresh parameter list every time, populate it with all assigned materials, then decide if it needs to allocate or free:

```cpp
// It completely replaces the old array here with the new one
p_instance->instance_shader_uniforms = isparams;

if (p_instance->instance_allocated_shader_uniforms != (p_instance->instance_shader_uniforms.size() > 0))
{
p_instance->instance_allocated_shader_uniforms = (p_instance->instance_shader_uniforms.size() > 0);

if (p_instance->instance_allocated_shader_uniforms)
{
// Allocates here
}
else
{
// Frees here
}
}
```

With the refactor it appears to use a persistent list in the new InstanceUniforms class, which contains everything added by the user, or by a material, and never gets cleared. As a result the following code which is intended to free the buffer is never invoked until the node is deleted.

```cpp
bool InstanceUniforms::materials_finish(RID p_self) {
ERR_FAIL_COND_V(p_self.is_null(), false);

if (_parameters.is_empty()) {
if (is_allocated()) {
free(p_self);
return true;
}

return false;
}
```

This appears to be an easy fix, add a boolean value to InstanceUniforms to indicate whether if has any in-use parameters:

```cpp
bool _has_in_use_parameters = false;
```

In _invalidate_items set this to false

```cpp
void InstanceUniforms::_invalidate_items() {
for (KeyValue &kv : _parameters) {
kv.value.index = -1;
}

_has_in_use_parameters = false;
}
```

In materials_append set this flag if any instance uniforms are found on the material

```cpp
void InstanceUniforms::materials_append(RID p_material) {
ERR_FAIL_COND(p_material.is_null());

List params;
RSG::material_storage->material_get_instance_shader_parameters(p_material, ¶ms);

for (const RendererMaterialStorage::InstanceShaderParam &srcp : params) {
StringName name = srcp.info.name;

_has_in_use_parameters = true;
// Removed for brevity
}
}
```

In materials_finish check the flag instead or parameter count.

```cpp
bool InstanceUniforms::materials_finish(RID p_self) {
ERR_FAIL_COND_V(p_self.is_null(), false);

if (!_has_in_use_parameters) {
if (is_allocated()) {
free(p_self);
return true;
}
return false;
}
```

This appears to work correctly in a test I've done.

### Steps to reproduce

1. In a loop:
2. Create an instance, assign a material ovverride using instance uniforms, and set an instance variable
3. Change that material to one that does not include any instance uniforms
4. Note that after a while you receive the dreaded "Too many instances using shader instance variables" error

Recommend setting a small global shader buffer size to make the problem happen quicker

### Minimal reproduction project (MRP)

[instanceuniformissue.zip](https://github.com/user-attachments/files/22066965/instanceuniformissue.zip)

This uses gdscript to create 10 boxes per second which use an instance shader to set random colours. After 5 seconds a script on each box changes it to a standard shader. This limits us to about 50 instances at a time that have the instance material and require space in the buffer.

Initially this is going fine:

Image

The colourful boxes are the new ones added, the white boxes are using the normal shader.

Image

Quickly however we start reciving buffer errors and the new boxes are now black, and the error log is full of the following:

Image

With the suggest fix above applied though it works fine as instances are freed from the buffer when the material is swapped out:

Image

Contributor guide

Open the contributing guide

Research direction

Reproduce the issue with the linked minimal reproduction project and inspect render_scene_cull.cpp alongside InstanceUniforms::materials_append, _invalidate_items, and materials_finish. Confirm the instance-uniform allocation remains after swapping to a material without instance uniforms; done means released slots can be reused without the buffer exhaustion errors.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, godot
Domain
computer-graphics, game-dev, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.