Skip to content

One color decoder, and gates that report - #566

Merged
Eli Pinkerton (wallstop) merged 7 commits into
mainfrom
dev/wallstop/session-224
Aug 25, 2026
Merged

One color decoder, and gates that report#566
Eli Pinkerton (wallstop) merged 7 commits into
mainfrom
dev/wallstop/session-224

Conversation

@wallstop

@wallstop Eli Pinkerton (wallstop) commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Why: The package had two decoders for one color channel, and a relational field that found nothing cost 400x one that succeeded.

What:

  • ColorQuantization.ToNormalized divides by 255, matching Unity's own Color32 conversion on all 256 channels (the rounded reciprocal disagreed on 126).
  • An unsatisfied relational field is ~15x cheaper: 366-431 us to 25.1 us. The cost was Unity capturing a stack trace on every assignment, so Log/LogWarn/LogError now take stackTrace: false.
  • All seven remaining linters get a self-test with a red half per rule; the allowlist is empty. Writing those red halves found three gates that could not report.
  • An Instant effect that defines periodic data warns once instead of twice.
  • The Unity testing skill splits into three, from 497 of 500 lines.

Fixes #565
Fixes #564
Fixes #562
Fixes #568


Note

Medium Risk
Color decode and threshold behavior change for many channels (intentional Unity alignment) could shift pixel filtering; logging API is backward-compatible but alters default relational and effect diagnostics at scale.

Overview
Aligns 8-bit channel decoding with Unity and removes hidden load-time cost from repeated Unity logs, plus repo tooling that can actually fail when its rules break.

Color and visuals: ColorQuantization.ToNormalized now divides by 255f (not channel * ChannelStep), with docs/tests pinning agreement with Unity's Color32 conversion and fixing the CI boundary case for ToThresholdByte. Sprite color averaging in ColorExtensions uses ToThresholdByte and ToNormalized instead of per-pixel / 255f compares.

Logging and gameplay paths: Log / LogWarn / LogError (extensions and UnityLogTagFormatter) accept optional stackTrace: false, routing through Debug.LogFormat with LogOption.NoStacktrace. Missing relational components log errors without stack traces (~15× faster unsatisfied assignments per changelog). Instant effects with periodic/behaviour data warn once (duplicate warn removed from InternalApplyEffect), also without stack trace.

Docs: MCP no-license testing guidance moves from unity-devcontainer-testing into unity-mcp-fixture-runner and unity-mcp-measurement, with cross-links updated.

CI/contracts: Seven validators/linters gain self-tests with red halves; lint-bundled-assemblies and lint-doc-counts accept injectable paths; run-contract-tests and package.json register the new test scripts; test-run-repo-lint clears the "missing red half" allowlist.

Reviewed by Cursor Bugbot for commit 8ce7dd6. Bugbot is set up for automated code reviews on this repo. Configure here.

Seven linters were carried on an explicit allowlist in the meta-check added
last session: they were run, and nothing proved they could still fail. Each
now has a self-test with a red half per rule, registered in the contract
suite, and the allowlist is empty.

Five needed a way to be handed something other than the repository itself, so
they take -BinariesRoot / -RepoRoot / -SyncScriptPath / --repo-root. Two
already accepted a path and needed only fixtures.

Writing the red halves found three things the green runs could not:

* validate-github-pages-css.sh matched a required selector and its property
  independently over the whole file, so `section { float: none }` satisfied
  the rule for `header`. Removing either was invisible. The property is now
  checked inside the selector's own brace-counted block.
* validate-devcontainer-config.ps1 carried a workflow-directory guard that no
  input could reach: the publish workflow lives inside that directory and is
  checked first.
* validate-hook-sync-calls.ps1 matches its required patterns
  case-insensitively, so `-LocalSha` satisfies the requirement for
  `localSha`. The fixture removal matches that semantics, or the red half
  removes nothing and still reports green.

Fixes #562

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ColorQuantization.ToNormalized multiplied by the rounded reciprocal 1f/255f
while ColorExtensions divided by 255f, and those are not the same float:
measured on 6000.4.6f1 they disagree by one ULP on 126 of the 256 channels.
Both were live on the same "is this pixel transparent" decision, across 20+
call sites.

ToNormalized now divides. That is bit-identical to Unity's own Color32 to
Color conversion on all 256 channels -- measured, 0 disagreements, against
126 for the reciprocal -- so the package agrees with the engine and with
every consumer that writes c.a / 255f by hand.

The eight alpha-cutoff comparisons in ColorExtensions now hoist
ToThresholdByte out of their pixel loops, matching the 13 Editor sprite sites
and answering the cutoff once per call instead of once per pixel. The eight
remaining per-channel decodes route through ToNormalized.

ChannelStep stays, documented as a step size for scaling a tolerance, with
the reason it is not a decode.

The CI boundary cutoff 0.8862745f turns out to BE a channel boundary -- it is
bit-for-bit 226 / 255f -- so 226 belongs under it and ToThresholdByte now
answers 226. The old 225 was the reciprocal lifting channel 226 one ULP above
its own boundary.

Verified on 6000.4.6f1 through the MCP bridge: 395 pass / 0 fail across the
quantization, color, editor-cache and sprite fixtures. 23 [TestCaseSource]
cases could not run there, including the sprite alpha-threshold ones; CI's
Unity legs cover those.

Fixes #565

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A relational assignment that found nothing cost 366-431 us against ~1 us for
one that succeeded, paid on every assignment rather than once. For a
collection field, finding nothing is a normal state -- a Collider2D[] on an
object with no colliders is an error every time it is assigned -- so a scene
binding a few hundred objects at load paid it repeatedly, and the only
symptom was errors in the console, which reads as a content problem.

All of it was Unity capturing a managed stack trace for the error log.
Measured on 6000.4.6f1: Debug.LogError 178.4 us per call, the same message
through LogOption.NoStacktrace 13.3 us -- 13.4x.

The logger's three severities now take stackTrace: false, and
LogMissingComponentError passes it. Every message survives, one per object,
with its context still set. What goes is a stack that is the same internal
assignment path every time and names nothing the message does not.

Measured after, on the same editor with the same control:

  unsatisfied  366-431 us -> 25.1 us   (~15x)
  satisfied                  0.471 us
  control (Transform)        0.302 us  (issue recorded 0.305-0.309 us)

Still ~50x a satisfied field, because the log itself is 13.3 us. Coalescing
per (type, field) would close more and changes what the console shows, so
that stays the owner's call on #564.

Local Unity evidence: 277 pass / 0 fail across all 17 relational fixtures.
51 of them use LogAssert and cannot run through the MCP bridge, which is
exactly the set asserting this error -- whether LogAssert still matches a
NoStacktrace error is answered by CI, not here.

Fixes #564

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PLAN.md and the progress log for the session. Four MCP-bridge traps added to
the devcontainer-testing skill, all found by a control rather than by
inspection:

* a timing cell that prints 0.000 measured nothing and reads as "free"
* Convert.ChangeType throws InvalidCastException on a non-IConvertible
  [TestCase] argument and kills the whole sweep from inside the harness
* the bridge returns every Unity console line, so a logging probe truncates
  its own RESULT line
* the fixtures that cannot run there can BE the coverage: 277 pass / 0 fail
  over 17 relational fixtures, with the 51 unrunnable ones being exactly
  those asserting the log under change

test-lint-doc-counts.ps1 no longer runs the real sync script over the whole
repository. That re-answered a question lint:repo and validate:content both
already ask, and cost 26.7 s of the contract suite. The stub cases are the
green half for the wrapper's own contract: 26.7 s -> 2.9 s.

Refs #435, #543, #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 25, 2026 19:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

unity-devcontainer-testing.md was at 497 of 500 lines and reported CRITICAL
on every commit that touched it, so the next recorded trap would have broken
the gate rather than being written down. It was doing three jobs: the
licensed Docker legs, the MCP bridge as a fixture runner, and measuring in
the MCP sandbox. The last two are read together and almost never alongside
the first.

  unity-devcontainer-testing.md   497 -> 227   Docker, license, troubleshooting
  unity-mcp-fixture-runner.md         -> 222   reflection, fixtures, the loop
  unity-mcp-measurement.md            -> 93    timing, allocation, controls

The partition was verified total and disjoint over all 498 original lines
before anything was written, so no recorded trap was lost -- each one cost a
session to find.

Cross-references updated: the two links to the moved
#no-license-the-mcp-editor-still-runs-the-real-fixtures anchor now point at
the file that owns it.

Fixes #568

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
EffectHandler.ApplyEffect warns when an Instant effect also defines periodic
or behaviour data, then calls InternalApplyEffect, which tested the same two
conditions and warned again. InternalApplyEffect(AttributeEffect) is private
with exactly one caller -- that Instant branch -- so its condition was always
true and its message was always the second copy of one the console had just
shown. Each carried a {effect:json} render of the whole effect and a
stack-trace capture, on every application.

The duplicate is gone. The survivor passes stackTrace: false, since it is
reported per application and the stack is the same path every time.

It survived because the test registered one ExpectWallstopLog expectation,
LogAssert.Expect matches one message, and an unexpected WARNING does not fail
a Unity test. The test now counts emissions through
Application.logMessageReceived and asserts exactly one.

The remaining halves -- the per-application repetition of what is a static
property of the asset, and the {effect:json} render -- are #567.

Refs #567

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 25, 2026 19:33
@wallstop Eli Pinkerton (wallstop) changed the title Give the package one color decoder, and gates that report One color decoder, and gates that report Aug 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The stackTrace <param> was written between the [Conditional] attribute list
and the method, where a doc comment is not attached to the declaration, and
without a <summary> to hang it on -- a floating <param> is not a doc comment,
it is a comment that looks like one.

All four public entry points now carry a full block before their attributes,
and the three UnityLogTagFormatter methods whose signature this change
widened gain the docs they never had. The package requires <summary> on every
public member.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 25, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants