shader improvements by ceesem · Pull Request #108 · CAVEconnectome/nglui

ceesem · 2026-04-27T21:34:45Z

Add seaborns-style shader code builder

Copilot

Pull request overview

Adds a seaborn-style, fluent builder API for generating Neuroglancer GLSL shaders, plus tests and documentation to support the new shader-building workflow.

Changes:

Introduces AnnotationShaderBuilder and SkeletonShaderBuilder (plus palette/color utilities) for programmatic shader generation.
Updates SkeletonManager to expose make_shader_builder() and to provide a builder-based default skeleton shader.
Adds comprehensive unit tests for shader utilities/builders and expands docs (new Shaders guide + updated references/nav).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tests/test_shaders.py	New test suite covering shader builders and shader utility functions.
src/nglui/statebuilder/shaders.py	Major refactor: new builder classes, new utilities, updated default shader map.
src/nglui/skeletons/skeletons.py	Switches skeleton shader creation to `SkeletonShaderBuilder` and adds `make_shader_builder()`.
mkdocs.yml	Adds the new Shaders guide to the docs navigation.
docs/usage/skeletons.md	Documents builder-based skeleton shader workflows.
docs/usage/shaders.md	New user-facing guide for annotation/skeleton shader builders.
docs/reference/shaders.md	Updates API reference pages to focus on the new builder classes and key utilities.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T21:39:17Z

+
+        seg_block = self._segment_color_block()
+        if seg_block:
+            parts.append(seg_block)
+
+        cat_block = self._categorical_block()
+        if cat_block:
+            parts.append(cat_block)
+
+        cont_block = self._continuous_block()
+        if cont_block:
+            parts.append(cont_block)
+


SkeletonShaderBuilder.build() can generate an empty main() with no emitRGB/emitDefault when no mode is configured. That likely renders nothing (or at least surprises users) compared to AnnotationShaderBuilder’s minimal default behavior. Consider emitting a sensible default (e.g. segmentColor().rgb) when no segment/categorical/continuous configuration is provided, or raising a ValueError to force an explicit choice.

Suggested change

seg_block = self._segment_color_block()

if seg_block:

parts.append(seg_block)

cat_block = self._categorical_block()

if cat_block:

parts.append(cat_block)

cont_block = self._continuous_block()

if cont_block:

parts.append(cont_block)

has_color_output = False

seg_block = self._segment_color_block()

if seg_block:

parts.append(seg_block)

has_color_output = True

cat_block = self._categorical_block()

if cat_block:

parts.append(cat_block)

has_color_output = True

cont_block = self._continuous_block()

if cont_block:

parts.append(cont_block)

has_color_output = True

if not has_color_output:

parts.append(" emitRGB(segmentColor().rgb);")

Copilot · 2026-04-27T21:39:18Z

+```
+
+A `palette` argument (any [palettable](https://jiffyclub.github.io/palettable/) colormap name) controls auto-colour assignment when using `list[str]` input.
+The default palette is `CartoCOlors Bold_10`, chosen for visibility on Neuroglancer's black background.


Typo in palette name: CartoCOlors → CartoColors (matches palettable module name and the code default Bold_10).

Suggested change

The default palette is `CartoCOlors Bold_10`, chosen for visibility on Neuroglancer's black background.

The default palette is `CartoColors Bold_10`, chosen for visibility on Neuroglancer's black background.

Copilot · 2026-04-27T21:39:18Z

+        if cfg["range_slider"]:
+            t_expr = (
+                f"clamp((float(prop_{cfg['prop']}()) - rangeMin) "
+                f"/ (rangeMax - rangeMin), 0.0, 1.0)"
+            )
        else:
-            raise ValueError(f"Colormap object has no recognized color attribute")
+            mn = _format_float(cfg["range_min"])
+            mx = _format_float(cfg["range_max"])
+            t_expr = (
+                f"clamp((float(prop_{cfg['prop']}()) - {mn}) / ({mx} - {mn}), 0.0, 1.0)"
+            )


Continuous color normalization divides by (rangeMax - rangeMin). If the sliders are adjusted so rangeMax == rangeMin (or range_min == range_max when range_slider=False), the shader will divide by zero and t becomes NaN/Inf. Consider clamping the denominator to a small epsilon or handling the equal-range case explicitly.

Copilot · 2026-04-27T21:39:18Z

+        if cfg["range_slider"]:
+            t_expr = (
+                f"clamp(({cfg['attr']} - rangeMin) "
+                f"/ (rangeMax - rangeMin), 0.0, 1.0)"
+            )
+        else:
+            mn = _format_float(cfg["range_min"])
+            mx = _format_float(cfg["range_max"])
+            t_expr = f"clamp(({cfg['attr']} - {mn}) / ({mx} - {mn}), 0.0, 1.0)"


Skeleton continuous-color normalization has the same divide-by-zero risk as the annotation version: (rangeMax - rangeMin) can become 0 via sliders or equal hardcoded bounds, producing NaN/Inf. Add an epsilon/guard in the generated expression.

Suggested change

if cfg["range_slider"]:

t_expr = (

f"clamp(({cfg['attr']} - rangeMin) "

f"/ (rangeMax - rangeMin), 0.0, 1.0)"

)

else:

mn = _format_float(cfg["range_min"])

mx = _format_float(cfg["range_max"])

t_expr = f"clamp(({cfg['attr']} - {mn}) / ({mx} - {mn}), 0.0, 1.0)"

eps = "1e-6"

if cfg["range_slider"]:

raw_denom = "(rangeMax - rangeMin)"

numer = f"({cfg['attr']} - rangeMin)"

else:

mn = _format_float(cfg["range_min"])

mx = _format_float(cfg["range_max"])

raw_denom = f"({mx} - {mn})"

numer = f"({cfg['attr']} - {mn})"

safe_denom = (

f"((abs({raw_denom}) < {eps}) "

f"? ({raw_denom} < 0.0 ? -{eps} : {eps}) : {raw_denom})"

)

t_expr = f"clamp({numer} / {safe_denom}, 0.0, 1.0)"

Copilot · 2026-04-27T21:39:18Z

+# ---------------------------------------------------------------------------
+# Default shader map (referenced by layer defaults)
+# ---------------------------------------------------------------------------

 DEFAULT_SHADER_MAP = {
    "skeleton_compartments": simple_compartment_skeleton_shader,
    "points": simple_point_shader(),
-    "tags": PointShader(colormap="Set1", n_colors=9).code,
    "basic": basic_shader,
 }


This PR removes the previously available shader_base()/PointShader helpers from nglui.statebuilder.shaders. If those were part of the public API, this is a breaking change; consider keeping thin deprecated wrappers (or documenting the migration path to AnnotationShaderBuilder/SkeletonShaderBuilder) to avoid surprising downstream users.

Copilot · 2026-04-27T21:39:19Z

+            return _rgb_to_triple(webcolors.name_to_rgb(clr))
    else:
-        return rgb_to_triple(webcolors.IntegerRGB(*[int(255 * x) for x in clr]))
+        return _rgb_to_triple(webcolors.IntegerRGB(*[int(255 * x) for x in clr]))


parse_color_rgb() treats any non-string iterable color as 0–1 floats and multiplies by 255. This breaks callers that pass 0–255 RGB tuples/lists (e.g. (255,0,255) becomes 65025 for red). Consider detecting whether any channel > 1 and, in that case, using the values directly (or normalizing by 255) instead of multiplying again.

Suggested change

return _rgb_to_triple(webcolors.IntegerRGB(*[int(255 * x) for x in clr]))

r, g, b = clr[0], clr[1], clr[2]

if any(c > 1.0 for c in (r, g, b)):

rgb = webcolors.IntegerRGB(int(r), int(g), int(b))

else:

rgb = webcolors.IntegerRGB(int(255 * r), int(255 * g), int(255 * b))

return _rgb_to_triple(rgb)

Copilot · 2026-04-27T21:39:19Z

+    """Format a float for GLSL: no scientific notation, always has a decimal point."""
+    if f == int(f):
+        return f"{int(f)}.0"
+    return str(f)


_format_float() docstring promises “no scientific notation”, but returning str(f) will produce scientific notation for small/large values (e.g. 1e-06). If the goal is to avoid that (and keep GLSL/uicontrol parsing consistent), format with fixed-point (and trim) rather than str().

Suggested change

return str(f)

s = f"{f:.15f}".rstrip("0")

if s.endswith("."):

s += "0"

return s

Copilot · 2026-04-27T21:39:19Z

+def _palette_colors(n: int, palette: Optional[str] = None) -> list[str]:
+    """Return *n* hex color strings from a palettable colormap, cycling if needed.
+
+    Parameters
+    ----------
+    n : int
+        Number of colors needed.
+    palette : str, optional
+        Name of a palettable colormap (e.g. ``'Set1'``, ``'Tableau_10'``).
+        Searched across all palettable modules. Defaults to ``'Tableau_10'``.
+
+    Returns
+    -------
+    list[str]
+        Hex color strings like ``'#4e79a7'``, length *n*.
+    """
+    if palette is None:
+        colors = palettable.cartocolors.qualitative.Bold_10.hex_colors
+    else:


_palette_colors() docstring says the default palette is 'Tableau_10', but the implementation defaults to palettable.cartocolors.qualitative.Bold_10. Please update the docstring (and/or the default) so they match; also the inline comment says “find smallest that fits” but the code currently picks the closest size by abs difference.

Copilot · 2026-04-27T21:39:19Z

+        # Register UI controls: colors first, then checkboxes (appended after
+        # any sliders at build time because sliders are output between them).
+        for label in label_order:
+            self._color_controls.append((label, label_color[label]))
+        for label in label_order:
+            self._checkbox_configs.append((f"show_{label}", default_visible))
+
+        self._categorical_config = {
+            "prop": prop,
+            "groups": [
+                (label, label_color[label], label_values[label])
+                for label in label_order


categorical_color() uses the category label directly as a GLSL variable name and in the checkbox name (show_). For string-label inputs, real-world labels can contain spaces, dashes, start with digits, etc., which will generate invalid GLSL. Consider sanitizing labels to valid identifiers (and/or raising a clear ValueError when a label is invalid), while still keeping a mapping to the original label for UI display if needed.

Suggested change

# Register UI controls: colors first, then checkboxes (appended after

# any sliders at build time because sliders are output between them).

for label in label_order:

self._color_controls.append((label, label_color[label]))

for label in label_order:

self._checkbox_configs.append((f"show_{label}", default_visible))

self._categorical_config = {

"prop": prop,

"groups": [

(label, label_color[label], label_values[label])

for label in label_order

# Category labels are later used to construct internal names (for

# example checkbox names and shader identifiers), so reject labels

# that cannot be represented safely as identifiers.

identifier_re = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")

def _validated_category_label(label: str) -> str:

if not identifier_re.fullmatch(label):

raise ValueError(

"Invalid category label "

f"{label!r}: labels used by categorical_color() must "

"match ^[A-Za-z_][A-Za-z0-9_]*$ so they can be used in "

"generated shader/UI identifiers."

)

return label

validated_labels = [_validated_category_label(label) for label in label_order]

checkbox_names = [f"show_{label}" for label in validated_labels]

if len(set(checkbox_names)) != len(checkbox_names):

raise ValueError(

"Category labels produce duplicate checkbox identifiers; "

"please use unique labels."

)

# Register UI controls: colors first, then checkboxes (appended after

# any sliders at build time because sliders are output between them).

for label in validated_labels:

self._color_controls.append((label, label_color[label]))

for checkbox_name in checkbox_names:

self._checkbox_configs.append((checkbox_name, default_visible))

self._categorical_config = {

"prop": prop,

"groups": [

(label, label_color[label], label_values[label])

for label in validated_labels

Programmatic GLSL shader construction for Neuroglancer annotation and skeleton layers. Builders produce #uicontrol declarations and a void main() body from fluent configuration calls (opacity, highlight, point_size, categorical_color, continuous_color, etc). AnnotationShaderBuilder also exposes a from_info classmethod and a top-level auto_annotation_shader helper that build a sensible default shader from a precomputed annotation info file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Reject empty `categories` argument in both builders' categorical_color() (used to silently produce GLSL with an unmatched `}`). - SkeletonShaderBuilder now always emits a base segment-colour line before any categorical/continuous block, so vertices that don't match any branch have a defined colour. AnnotationShaderBuilder applies the configured opacity to its fallthrough setColor — previously, points whose property matched no category were drawn full-alpha regardless of the opacity slider. - Detect #uicontrol name collisions across all control kinds (slider, color, checkbox, invlerp). Calling opacity() twice, naming opacity 'rangeMin' before continuous_color(), and labels that sanitise to the same identifier (e.g. 'a-b' and 'a_b') now all raise. Builder state is left untouched on validation failure so retries work. - Validate inputs: dict-of-tuples values must be exactly 2-tuples; list entries must be exactly 3-tuples; categorical/highlight integer values must be non-negative (annotation properties compare as GLSL uint); SkeletonShaderBuilder.vertex_attribute() requires index >= 1 (vCustomN is 1-based); continuous_color() rejects range_min == range_max (would divide by zero). - _format_float() never emits scientific notation. repr(1e-7) is '1e-07'; GLSL ES 1.0 (and some strict ES 3.0 parsers) reject scientific-notation literals. Falls back to a wide fixed-point representation with trailing zeros stripped, and rejects NaN/inf. - continuous_color() is now call-once on both builders. Used to duplicate rangeMin/rangeMax sliders silently and replace the body config — caught by the new name-collision check in the range_slider=True case but slipped through with range_slider=False. 31 new regression tests; 590 total passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Lifts duplicated logic out of AnnotationShaderBuilder and SkeletonShaderBuilder into shared module-level helpers and a small _ControlRegistry class. Public API and generated GLSL output are unchanged (590 tests still pass, golden-output hashes for representative shapes are stable). - _sanitize_uicontrol_name and _sanitize_show_suffix are now module-level functions; the existing static-method names on AnnotationShaderBuilder remain as thin shims. Eliminates the cross-class call SkeletonShaderBuilder used to make to AnnotationShaderBuilder._sanitize_label. - _normalize_categories(categories, *, palette, value_caster) collapses ~70 lines of duplicated dict/list dispatch (handling list[str], dict[str,str], dict[V,tuple], list[tuple]) that both builders' categorical_color() methods previously duplicated. - New _ControlRegistry class owns the four UI control lists (colors, sliders, invlerps, checkboxes), the cross-kind _used_names registry, slider/invlerp validation and registration, and the canonical-order #uicontrol emission. Each builder holds one _controls instance instead of four lists + a name set + the claim/release/_register_slider helpers. - Drops a dead 'color' field from the categorical_config groups tuple — it was stored but never read by the categorical block emitter. shaders.py: 2253 -> 2178 lines (-75 net), -317 / +303 in diff terms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

shader improvements

9013c6a

ceesem requested a review from Copilot April 27, 2026 21:34

Copilot AI reviewed Apr 27, 2026

View reviewed changes

ceesem and others added 4 commits May 2, 2026 13:34

Improve skeleton shader from_info

01714e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shader improvements#108

shader improvements#108
ceesem wants to merge 5 commits into
masterfrom
annotation-shader-builder

ceesem commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	The default palette is `CartoCOlors Bold_10`, chosen for visibility on Neuroglancer's black background.
	The default palette is `CartoColors Bold_10`, chosen for visibility on Neuroglancer's black background.

-        return _rgb_to_triple(webcolors.IntegerRGB(*[int(255 * x) for x in clr]))
+        r, g, b = clr[0], clr[1], clr[2]
+        if any(c > 1.0 for c in (r, g, b)):
+            rgb = webcolors.IntegerRGB(int(r), int(g), int(b))
+        else:
+            rgb = webcolors.IntegerRGB(int(255 * r), int(255 * g), int(255 * b))
+        return _rgb_to_triple(rgb)

-    return str(f)
+    s = f"{f:.15f}".rstrip("0")
+    if s.endswith("."):
+        s += "0"
+    return s

Conversation

ceesem commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants