arkavo-org / arkavo-org/VRMMetalKit

MToon shader maintenance: rim gate desync risk and metallib CI freshness check

Open
#155 0 comments 0 reactions 0 assignees View on GitHub
maintainability technical-debt testing
Dominant language
Swift
Stars
6
Forks
2
Avg merge
18h 51m
Merged PRs (30d)
26

Description

## Background

Follow-up from PR #134 review.

## Issue 1 — Rim color vertex/fragment gate desync risk

In
MToonShader.metal
, the vertex stage decides whether to emit
viewNormal
and
viewDirection
based on
hasParametricRim(material)
:

metal
static inline bool hasParametricRim(constant MToonMaterial& material) {
return material.parametricRimColorR > 0.0 ||
material.parametricRimColorG > 0.0 ||
material.parametricRimColorB > 0.0;
}

static inline bool needsViewNormal(...) {
return material.hasMatcapTexture > 0 || hasParametricRim(material) || ...;
}

static inline bool needsViewDirection(...) {
return hasParametricRim(material) || ...;
}

Meanwhile, the fragment stage short-circuits rim lighting on:

metal
if (any(parametricRimColorFactor > 0.0)) { ... }

These are mathematically equivalent **today**, but if a future change lets the
rimMultiplyTexture
drive rim without a nonzero parametric color (the fragment path at line 633 checks
material.hasRimMultiplyTexture > 0
), the vertex-side heuristic would still skip
viewNormal/viewDirection
and the multiply contribution would be lost.

**Fix**: Add a one-line comment in both the vertex and fragment shaders linking the two gates, and/or include
material.hasRimMultiplyTexture > 0
in
needsViewNormal
and
needsViewDirection
.

## Issue 2 — No CI check for stale metallib

The PR updated
compile-shaders.sh
to make reproducible local builds more robust, but reviewers can't diff the
.metallib
binary. Since the script output path is deterministic given the toolchain, a CI job that runs
./compile-shaders.sh
and then
git diff --exit-code Sources/VRMMetalKit/Resources/
would catch a stale-metallib merge.

**Fix**: Add a CI workflow step (or standalone job) that compiles shaders and fails if the generated metallib differs from what's committed.

## Related

- PR #134 review (Minor rim gate comment, CI perf gate suggestion)
-
Sources/VRMMetalKit/Shaders/MToonShader.metal:185-191
(
needsViewNormal/needsViewDirection
)
-
Sources/VRMMetalKit/Shaders/MToonShader.metal:618
(fragment rim short-circuit)
-
Sources/VRMMetalKit/Shaders/MToonShader.metal:633
(rimMultiplyTexture path)

Contributor guide

Open the contributing guide

Research direction

Start with Sources/VRMMetalKit/Shaders/MToonShader.metal at the needsViewNormal/needsViewDirection helpers and the fragment rim checks around lines 618 and 633. Then inspect compile-shaders.sh and the repository's CI workflow. Done means the rim gates cannot silently diverge and CI recompiles the shader and fails when the committed metallib differs.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, shell
Domain
ci-cd, computer-graphics
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.