Skip to content

Add configuration warning for overlapping shader instance uniforms #1327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ghost wants to merge 1 commit into from
+110 −1
Closed

Add configuration warning for overlapping shader instance uniforms #1327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions scene/3d/visual_instance_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "visual_instance_3d.h"

#include "core/config/project_settings.h"
#include "scene/3d/mesh_instance_3d.h"

AABB VisualInstance3D::get_aabb() const {
AABB ret;
Expand Down Expand Up @@ -541,6 +542,106 @@ PackedStringArray GeometryInstance3D::get_configuration_warnings() const {
warnings.push_back(RTR("GeometryInstance3D visibility range transparency fade is only available when using the Forward+ rendering method."));
}

List<Ref<Material>> materials_to_check;

if (material_override.is_valid()) {
materials_to_check.push_back(material_override);
}
if (material_overlay.is_valid()) {
materials_to_check.push_back(material_overlay);
}

// Also account for surface override materials if this node is a MeshInstance3D
const MeshInstance3D *mesh_instance = Object::cast_to<MeshInstance3D>(this);
if (mesh_instance) {
int count = mesh_instance->get_surface_override_material_count();
for (int i = 0; i < count; i++) {
Ref<Material> mat = mesh_instance->get_surface_override_material(i);
if (mat.is_valid()) {
materials_to_check.push_back(mat);
}
}
}

struct UniformSlot {
String uniform_name;
Ref<Material> material;
};

HashMap<int, UniformSlot> occupied_slots;
String conflict_details;
bool has_conflict = false;

// Traverse through materials and their next_pass chains
for (const Ref<Material> &top_mat : materials_to_check) {
Ref<Material> curr = top_mat;

while (curr.is_valid()) {
Ref<ShaderMaterial> s_mat = curr;
if (s_mat.is_valid() && s_mat->get_shader().is_valid()) {
Ref<Shader> shader = s_mat->get_shader();
RID shader_rid = shader->get_rid();

if (shader_rid.is_valid()) {
List<PropertyInfo> param_list;
RenderingServer::get_singleton()->get_shader_parameter_list(shader_rid, &param_list);

for (const PropertyInfo &p : param_list) {
const String tag = "instance_index:";
int pos = p.hint_string.find(tag);

// Not an instance uniform
if (pos == -1) {
continue;
}

String idx_str = p.hint_string.substr(pos + tag.length());

// Strip any metadata after the index
int comma = idx_str.find(",");
if (comma != -1) {
idx_str = idx_str.left(comma);
}

idx_str = idx_str.strip_edges();

int idx = idx_str.to_int();

if (occupied_slots.has(idx)) {
const UniformSlot &prev = occupied_slots[idx];

// Same material encountered again (shared material/duplicate reference)
if (prev.material == curr) {
continue;
}

// Only warn if a different uniform occupies the same slot
if (prev.uniform_name != p.name) {
has_conflict = true;

String prev_material_name = prev.material.is_valid() && !prev.material->get_name().is_empty() ? prev.material->get_name() : "Material";

String curr_material_name = !curr->get_name().is_empty() ? curr->get_name() : "Material";

conflict_details += vformat("\n- Slot %d clash: '%s' (%s) and '%s' (%s)", idx, prev.uniform_name, prev_material_name, p.name, curr_material_name);
}
} else {
occupied_slots[idx] = { p.name, curr };
}
Comment on lines +608 to +630

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Conflict detection only compares uniform names, not types.

Issue #1119 (linked in this PR) explicitly calls out corruption from instance parameters that differ in "names or types" occupying the same slot. Here, prev.uniform_name != p.name is the sole conflict criterion — a same-named uniform with a different Variant::Type at the same slot (e.g. float in one material vs vec4 in another) silently passes through undetected, even though it's the exact class of bug this PR is meant to surface.

🐛 Suggested fix 
 	struct UniformSlot {
 		String uniform_name;
+		Variant::Type uniform_type;
 		Ref<Material> material;
 	};
@@
-						if (prev.uniform_name != p.name) {
+						if (prev.uniform_name != p.name || prev.uniform_type != p.type) {
 							has_conflict = true;
@@
-						} else {
-							occupied_slots[idx] = { p.name, curr };
+						} else {
+							occupied_slots[idx] = { p.name, p.type, curr };
 						}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scene/3d/visual_instance_3d.cpp` around lines 608 - 630, Update the conflict
condition in the occupied_slots handling to detect clashes when either the
uniform names or their Variant::Type values differ. Extend UniformSlot to retain
the previous uniform type and compare it with the current parameter’s type,
while preserving the existing same-material exemption and conflict_details
reporting.

}
}
}

curr = curr->get_next_pass();
}
}

if (has_conflict) {
warnings.push_back(
RTR("Per-instance shader parameters conflict across materials/next_passes assigned to this node:") + conflict_details +
RTR("\n\nSet explicit indices in your shader code (e.g. 'instance uniform vec4 my_var : instance_index(1);') to resolve the clash."));
}

return warnings;
}

Expand Down
2 changes: 1 addition & 1 deletion servers/rendering/renderer_rd/storage_rd/material_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ void MaterialStorage::ShaderData::get_shader_uniform_list(List<PropertyInfo> *p_
LocalVector<Pair<StringName, int>> filtered_uniforms;

for (const KeyValue<StringName, ShaderLanguage::ShaderNode::Uniform> &E : uniforms) {
if (E.value.scope != ShaderLanguage::ShaderNode::Uniform::SCOPE_LOCAL) {
if (E.value.scope != ShaderLanguage::ShaderNode::Uniform::SCOPE_LOCAL && E.value.scope != ShaderLanguage::ShaderNode::Uniform::SCOPE_INSTANCE) {

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Trace who consumes get_shader_uniform_list / get_shader_parameter_list
rg -n 'get_shader_uniform_list' --type=cpp
rg -n 'get_shader_parameter_list' --type=cpp -C2
ast-grep run --pattern 'class ShaderMaterial {
  $$$
}' --lang cpp scene/resources/material.cpp
rg -n '_get_property_list' scene/resources/material.cpp -A15

Repository: Redot-Engine/redot-engine

Length of output: 163

🏁 Script executed:

#!/bin/bash
set -u

echo "== files =="
git ls-files | rg '(^|/)material_storage\.cpp$|(\.h)$' | rg 'rendering|material|shader' | head -100

echo "== locate symbols (all tracked) =="
rg -n 'void get_shader_uniform_list|get_shader_uniform_list|get_shader_parameter_list|_get_property_list|class ShaderMaterial' . --type=cpp --type=h -C2 | head -300

echo "== material_storage outline if exists =="
for f in $(git ls-files | rg 'material_storage\.cpp$'); do
  echo "--- $f"
  wc -l "$f"
  sed -n '560,670p' "$f"
done

echo "== material source candidates =="
for f in $(git ls-files | rg 'material.(cpp|h)$|shader_material.(cpp|h)$'); do
  echo "--- $f"
  rg -n '_get_property_list|ShaderMaterial|_get_property_list|ShaderMaterial' "$f" -C3 | head -120 || true
done

Repository: Redot-Engine/redot-engine

Length of output: 44166

🏁 Script executed:

#!/bin/bash
set -u

echo "== RenderingServer get_shader_parameter_list declarations/definitions =="
rg -n 'get_shader_parameter_list|get_shader_uniform_list|ShaderMaterial::_get_property_list|ShaderMaterial::_get_property_list' servers scene drivers --type=cpp --type=h --type=gen -C3

echo "== Shader RD declaration/definition around uniform list =="
rg -n 'class RenderingServerRD|get_shader_uniform_list|get_shader_parameter_list' servers/rendering/renderer_rd -g '*rendering_server*.h' -g '*rendering_server*.cpp' -C4

echo "== visual_instance_3d parameter usage snippets =="
rg -n 'get_shader_parameter_list|get_instance_param_list|instance_index' scene servers tests -g '*visual_instance_3d*' -g '*.cpp' -g '*.h' -C4 | head -240

echo "== RD uniform declaration snippets =="
fd -a '^(shader_rd|h)\.(cpp|h)$' servers/rendering/renderer_rd | sed 's#^\./##' | xargs -r -n1 sh -c 'echo "--- $0"; rg -n "get_shader_uniform_list|get_shader_parameter_list|instance_index|SCOPE_INSTANCE|SCOPE_LOCAL" "$0" -C3 || true'

Repository: Redot-Engine/redot-engine

Length of output: 22867

Keep instance-scoped uniforms out of the material property list.

ShaderMaterial::_get_property_list() calls Shader::get_shader_uniform_list(), which routes through RenderingServer::get_shader_parameter_list() and ends up in MaterialStorage::ShaderData::get_shader_uniform_list(). If this change adds SCOPE_INSTANCE uniforms there, they become material-level editable properties, while instance parameters are already exposed via material_get_instance_shader_parameters(). Split or filter the uniform source so SCOPE_INSTANCE is only available through the instance-param path.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/rendering/renderer_rd/storage_rd/material_storage.cpp` at line 603,
Update MaterialStorage::ShaderData::get_shader_uniform_list() so SCOPE_INSTANCE
uniforms are excluded from the material uniform list returned through
RenderingServer::get_shader_parameter_list() and
ShaderMaterial::_get_property_list(). Preserve their availability through
material_get_instance_shader_parameters(), filtering or separating the uniform
source as needed.

continue;
}
filtered_uniforms.push_back(Pair<StringName, int>(E.key, E.value.prop_order));
Expand Down
8 changes: 8 additions & 0 deletions servers/rendering/shader_language.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5076,6 +5076,14 @@ PropertyInfo ShaderLanguage::uniform_to_property_info(const ShaderNode::Uniform
case ShaderLanguage::TYPE_MAX:
break;
}

if (p_uniform.scope == ShaderNode::Uniform::SCOPE_INSTANCE) {
if (!pi.hint_string.is_empty()) {
pi.hint_string += ",";
}
pi.hint_string += "instance_index:" + itos(p_uniform.instance_index);
}

Comment on lines +5079 to +5086

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg 'servers/rendering/shader_language\.cpp|resource/material_storage\.(h|cpp)$' || true

echo "== surrounding shader_language.cpp =="
sed -n '4820,5095p' servers/rendering/shader_language.cpp | nl -ba -v4820

echo "== uniform_to_property_info callers/get_shader_uniform_list references =="
rg -n "uniform_to_property_info|get_shader_uniform_list|SCOPE_INSTANCE|instance_index" servers rendering -g '*.{cpp,h,hpp,inc}' | head -200

Repository: Redot-Engine/redot-engine

Length of output: 291

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== line counts and candidate files =="
wc -l servers/rendering/shader_language.cpp
git ls-files | grep -E 'material_storage\.(h|cpp)$' || true

echo "== surrounding shader_language.cpp =="
sed -n '4800,5105p' servers/rendering/shader_language.cpp

echo "== uniform_to_property_info and get_shader_uniform_list references =="
grep -RInE "uniform_to_property_info|get_shader_uniform_list|SCOPE_INSTANCE|instance_index" . --include='*.cpp' --include='*.h' --include='*.hpp' | head -300

echo "== property hint handling in Resource Property Inspector? =="
grep -RInE "PROPERTY_HINT_RANGE|PROPERTY_HINT_ENUM|PROPERTY_HINT_FLAGS|PROPERTY_HINT_TYPE_STRING" core servers scene editor --include='*.cpp' --include='*.h' --include='*.hpp' 2>/dev/null | head -200 || true

Repository: Redot-Engine/redot-engine

Length of output: 50381

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== visual shader editor uniform list consumers =="
sed -n '530i715p' 'editor/shader/visual_shader_editor_plugin.cpp'
sed -n '2035i2110p' 'editor/shader/visual_shader_editor_plugin.cpp'
sed -n '8338i8395p' 'editor/shader/visual_shader_editor_plugin.cpp'

echo "== shader uniform list implementation =="
sed -n '120i245p' scene/resources/shader.cpp
sed -n '85i145p' scene/resources/material.cpp
sed -n '240i285p' scene/resources/material.cpp

echo "== property hint parsing locations =="
grep -RInE "get_property_list|_validate_property|show_option|show_flags|skeleton|property_utils|get_string_options|validate_value" scene core servers editor --include='*.cpp' --include='*.h' --include='*.hpp' | head -300

echo "== PropertyUtils / validate_property exact references =="
grep -RIne "PropertyUtils::validate_property|validate_property.*hint_string|EnumProperty|FlagsProperty|TypeStringProperty|HintString" scene core servers editor --include='*.cpp' --include='*.h' --include='*.hpp' | head -250

Repository: Redot-Engine/redot-engine

Length of output: 31002

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== visual_shader_editor_plugin.uniform_to_property_info usage =="
python3 - <<'PY'
from pathlib import Path
p = Path("editor/shader/visual_shader_editor_plugin.cpp")
text = p.read_text(errors="replace").splitlines()
for needle in ["uniform_to_property_info", "shader->get_shader_uniform_list", "get_shader_uniform_list"]:
    print(">", needle)
    for i,line in enumerate(text, 1):
        if needle in line:
            start=max(1,i-20); end=min(len(text), i+80)
            print(f"#{i}")
            for j in range(start,end):
                print(f"{j:05d}: {text[j-1]}")
PY

echo "== shader/material consumer slices =="
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
    ("scene/resources/shader.cpp", 130, 180),
    ("scene/resources/shader.cpp", 240, 275),
    ("scene/resources/material.cpp", 470, 560),
    ("drivers/gles3/storage/material_storage.cpp", 580, 675),
    ("drivers/gles3/storage/material_storage.cpp", 720, 760),
    ("renderer_rd/storage_rd/material_storage.cpp", 585, 655),
    ("renderer_rd/storage_rd/material_storage.cpp", 745, 775),
]:
    text = Path(path).read_text(errors="replace").splitlines()
    print(f"> {path} {start}-{end}")
    for j in range(start, min(end, len(text))+1):
        print(f"{j:05d}: {text[j-1]}")
PY

Repository: Redot-Engine/redot-engine

Length of output: 25761

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate property hint validation files =="
python3 - <<'PY'
from pathlib import Path
terms = ["validate_property", "PropertyUtils", "EnumProperty", "FlagsProperty", "TypeStringProperty", "hint_string"]
for root in ["scene", "core/drivers", "editor", "servers"]:
    base = Path(root)
    if not base.exists(): continue
    for p in base.rglob("*"):
        if p.suffix not in {".cpp",".h",".hpp"} or "thirdparty" in p.parts: continue
        try: s=p.read_text(errors="replace")
        except Exception: continue
        if any(t in s for t in terms):
            print(p)
PY

echo "== focused hint parser candidates =="
rg -n "validate_property|PROPERTY_HINT_(ENUM|FLAGS|TYPE_STRING|RANGE)|EnumProperty|FlagsProperty|TypeStringProperty|hint_string\.split|splitv" scene core editor servers --include='*.cpp' --include='*.h' --include='*.hpp' --max-count 120

Repository: Redot-Engine/redot-engine

Length of output: 7584

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== focused hint parser candidates =="
rg -n "validate_property|PROPERTY_HINT_(ENUM|FLAGS|TYPE_STRING|RANGE)|EnumProperty|FlagsProperty|TypeStringProperty|hint_string" -g '*.cpp' -g '*.h' -g '*.hpp' scene core editor servers | head -200

echo "== property_utils implementation =="
wc -l scene/property_utils.cpp scene/property_utils.h
sed -n '1,520p' scene/property_utils.cpp 2>/dev/null || true
sed -n '1,180p' scene/property_utils.h 2>/dev/null || true

echo "== visual shader parameter set/_set_preview_shader_parameter implementation =="
rp=(scripts/* visual_shader.* scene/* visual_shader* editor/shader/*)
for f in $(git ls-files | grep -E '(^|/)visual_shader\.(cpp|h)$|editor/shader/(visual_shader_editor_plugin|visual_shader_editor).*(cpp|h)$'); do
  echo "FILE $f"
  wc -l "$f"
  grep -n "_set_preview_shader_parameter\|set_shader_parameter\|get_argument_options" "$f" || true
  if grep -q "_set_preview_shader_parameter" "$f"; then
    grep -n "_set_preview_shader_parameter" "$f" | while read -r line; do
      l=${line%%:*}
      s=$((l-30)); [ $s -lt 1 ] && s=1
      e=$((l+120)); [ $e -gt $(wc -l < "$f") ] && e=$(wc -l < "$f")
      sed -n "$s,$ep" "$f"
    done
  fi
done

Repository: Redot-Engine/redot-engine

Length of output: 50381

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== focused hint parser candidates =="
rg -n "validate_property|PROPERTY_HINT_(ENUM|FLAGS|TYPE_STRING|RANGE)|EnumProperty|FlagsProperty|TypeStringProperty" -g '*.cpp' -g '*.h' -g '*.hpp' scene core editor servers | head -250

echo "== enum/flags hint parser snippets =="
rg -n "EnumProperty|FlagsProperty|PROPERTY_HINT_ENUM|PROPERTY_HINT_FLAGS|show_option|show_flags|validate_value.*hint_string|hint_string\.split|splitv" -g '*.cpp' -g '*.h' -g '*.hpp' editor scene servers core | head -250

echo "== visual_shader files outline/search =="
python3 - <<'PY'
import subprocess
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
for f in files:
    if 'visual_shader' in f or f == 'editor/shader/visual_shader_editor_plugin.cpp':
        if f.endswith(('.cpp','.h','.hpp')):
            print(f"FILE {f}")
            out = subprocess.run(['grep','-n',"set_shader_parameter|_set_preview_shader_parameter|get_shader_uniform_list|uniform_to_property_info"], f, capture_output=True, text=True)
            if out.stdout:
                print(out.stdout)
PY

Repository: Redot-Engine/redot-engine

Length of output: 50381

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== shader language parse enum/flag uniform parsing =="
sed -n '9910,9990p' servers/rendering/shader_language.cpp
sed -n '10090,10110p' servers/rendering/shader_language.cpp

echo "== get_shader_uniform_list filtering behavior =="
sed -n '620,638p' drivers/gles3/storage/material_storage.cpp
sed -n '628,645p' drivers/gles3/storage/material_storage.cpp 2>/dev/null || true
sed -n '592,632p' drivers/gles3/storage/material_storage.cpp

echo "== visual shader parser uniform_to_property_info occurrences =="
grep -n "uniform_to_property_info" editor/shader/visual_shader_editor_plugin.cpp || true

echo "== enum/flags parser implementation candidates =="
rg -n 'PROPERTY_HINT_ENUM|PROPERTY_HINT_FLAGS|EnumProperty|FlagsProperty|show_option|show_flags|ValidateProperty|validate_property' editor core scene servers -g '*.cpp' -g '*.h' -g '*.hpp' | grep -v thirdparty | head -160

Repository: Redot-Engine/redot-engine

Length of output: 37839

Remove the instance_index hint_string mutation.

get_shader_uniform_list() skips SCOPE_INSTANCE uniforms, so this appends instance_index:N only to the separate instance-param consumer path. Modifying pi.hint_string here still overwrites the property hint semantics for ENUM, FLAGS, RANGE, and typed-array hints; carry instance index metadata out of band instead of appending it to hint_string.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/rendering/shader_language.cpp` around lines 5079 - 5086, Remove the
SCOPE_INSTANCE block in the uniform metadata construction that appends
instance_index to pi.hint_string. Preserve pi.hint_string exclusively for
property hint semantics, and carry the instance index through a separate
metadata path used by the instance-parameter consumer.

return pi;
}

Expand Down
Loading