Add configuration warning for overlapping shader instance uniforms - #1327
Open
Shakai-Dev wants to merge 1 commit into
Open
Add configuration warning for overlapping shader instance uniforms#1327Shakai-Dev wants to merge 1 commit into
Shakai-Dev wants to merge 1 commit into
Conversation
WalkthroughThe change exposes instance uniform indices in shader property metadata and includes instance-scoped uniforms in material lists. ChangesInstance shader parameter handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GeometryInstance3D
participant MeshInstance3D
participant Material
GeometryInstance3D->>MeshInstance3D: collect surface override materials
GeometryInstance3D->>Material: inspect material and next_pass chain
Material-->>GeometryInstance3D: return instance slot and uniform metadata
GeometryInstance3D-->>GeometryInstance3D: append configuration warning for clashes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scene/3d/visual_instance_3d.cpp`:
- Around line 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.
In `@servers/rendering/renderer_rd/storage_rd/material_storage.cpp`:
- 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.
In `@servers/rendering/shader_language.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17e0959e-82f6-4e93-ab03-860243850425
📒 Files selected for processing (3)
scene/3d/visual_instance_3d.cppservers/rendering/renderer_rd/storage_rd/material_storage.cppservers/rendering/shader_language.cpp
Comment on lines
+608
to
+630
| 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 }; | ||
| } |
There was a problem hiding this comment.
🎯 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.
|
|
||
| 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) { |
There was a problem hiding this comment.
🎯 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 -A15Repository: 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
doneRepository: 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.
Comment on lines
+5079
to
+5086
|
|
||
| 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 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 -200Repository: 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 || trueRepository: 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 -250Repository: 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]}")
PYRepository: 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 120Repository: 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
doneRepository: 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)
PYRepository: 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 -160Repository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1119
Adds an editor configuration warning when multiple
ShaderMaterialsassigned to the sameGeometryInstance3D(includingnext_pass,material_override,material_overlay& surface override materials) use conflicting per-instance shader parameter indices.The warning:
Summary by CodeRabbit
New Features
Bug Fixes