GLSLShader binds autogrow uniforms by list position, so a gap in the slots silently rebinds every uniform after it
- Dominant language
- Python
- Stars
- 133k
- Forks
- 15.7k
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 158
Description
### Summary
`GLSLShader` binds its autogrow inputs to shader uniforms **by list position**, but builds those lists by packing the autogrow dict's values. Autogrow omits unconnected slots from the dict entirely, so any gap silently rebinds every following slot to the wrong uniform.
The input sockets are literally named after the uniforms (`u_float0`, `u_int0`, `u_bool0`, `u_curve0`), so the user's mental model is "socket `u_float5` feeds `u_float5`". It does not.
### Where
`comfy_extras/nodes_glsl.py:763-770` builds the dense lists:
```python
image_list = [v for v in images.values() if v is not None]
float_list = ([v if v is not None else 0.0 for v in floats.values()] if floats else [])
int_list = [v if v is not None else 0 for v in ints.values()] if ints else []
bool_list = [v if v is not None else False for v in bools.values()] if bools else []
curve_luts = [v.to_lut().astype(np.float32) for v in curves.values() if v is not None] if curves else []
```
`comfy_extras/nodes_glsl.py:508-546` binds them by enumeration index:
```python
loc = gl.glGetUniformLocation(program, f"u_image{i}")
...
for i, v in enumerate(floats):
loc = gl.glGetUniformLocation(program, f"u_float{i}")
```
The `v if v is not None else 0.0` fallbacks show the intent — index positions are meant to be preserved — but they only cover a slot that is *present and None*, not one that is absent.
### Why the gap is reachable
The frontend closes gaps at disconnect time (`dynamicWidgets.ts:476-510` bubbles links down), so the interactive path usually stays dense. It is still reachable by:
- loading a saved workflow that already contains a gap — compaction runs on the disconnect event, not on load
- any workflow built programmatically or posted to `/prompt` directly (API users, agents, `comfy_api` clients)
- an upstream node erroring out so a slot resolves to nothing
The same bug class produced a real, user-visible incident on the layered-compositor work: two parallel autogrow groups (`image_N` / `mask_N`) were zipped densely, and leaving `mask_0` unconnected shifted every mask by one, silently. A dummy all-zero mask had to be added upstream purely to keep slot 0 occupied.
### Reproduction of the mechanism
Against `get_finalized_class_inputs` / `build_nested_inputs` at head, with slot 0 unconnected:
```
values in : {'u_float1': 0.25, 'u_float2': 0.5}
dense list: [0.25, 0.5] -> binds u_float0=0.25, u_float1=0.5
correct : [0.0, 0.25, 0.5] -> binds u_float0=0.0, u_float1=0.25, u_float2=0.5
```
The slot index is recoverable — it is in the dict key. `comfy_api/latest/_io.py:1153` only inserts a key when the slot is in `live_inputs`, so absence is at a *known* key.
### Proposed fix
For the scalar uniforms this is unambiguous, since the node already defines what an unset slot means:
```python
def _by_slot_index(values: dict | None, default):
"""Autogrow omits unconnected slots, and these uniforms are bound by index."""
if not values:
return []
indexed = {}
for name, value in values.items():
digits = name[len(name.rstrip("0123456789")):]
if digits and value is not None:
indexed[int(digits)] = value
return [indexed.get(i, default) for i in range(max(indexed) + 1)] if indexed else []
float_list = _by_slot_index(floats, 0.0)
int_list = _by_slot_index(ints, 0)
bool_list = _by_slot_index(bools, False)
```
### The part that needs a decision — hence an issue rather than a PR
`u_image{i}` and `u_curve{i}` bind *textures*, and there is no defined "unset" value:
1. bind nothing for the missing index and let the shader sample an unbound sampler (undefined behaviour), or
2. bind a 1x1 transparent-black placeholder texture, or
3. raise a clear error naming the gap, e.g. `"u_image1 is connected but u_image0 is not; GLSL uniforms are bound by index"`.
(3) is probably right — a shader that samples `u_image0` while the user only wired `u_image1` is a mistake worth reporting, not papering over. But it is a UX call, and `image_list[0]` is also used for the output dimensions and batch size (`nodes_glsl.py:780-782`), so the missing-index case has to be resolved before that line either way.
I do not have an EGL/GPU environment here to exercise the render path, which is the other reason this is an issue and not a PR. Happy to write it once the semantics for (1)/(2)/(3) are picked.
Contributor guide
Research direction
Read comfy_extras/nodes_glsl.py:508-546 and 763-782, then trace key creation in comfy_api/latest/_io.py:1153 and gap compaction in dynamicWidgets.ts:476-510. Reproduce the shown missing-slot mapping and determine the intended behavior for absent image and curve uniforms. Done means scalar slots retain their keys and the image/curve gap semantics are explicitly resolved without shifting later uniforms.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, python
- Domain
- backend, computer-graphics, frontend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100