Skip to content

fix: four components hide ArenaComponent.Update(), dropping repeat attribute messages - #238

Draft
claude[bot] wants to merge 1 commit into
mainfrom
fix/component-update-hiding
Draft

claude[bot] wants to merge 1 commit into
mainfrom
fix/component-update-hiding

Conversation

@claude

@claude claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Requested by mwfarb · Slack thread

Before: you drop a material-extras, modelUpdate or gltf-morph attribute on an object and the first message applies. Send a second message changing the same attribute on the same object and nothing happens in the Editor — the object keeps the values from the first message forever. An overrideSrc texture that had not finished downloading on the first pass never appears either, even after the file lands on disk.

After: the second and every later message applies. Change gltfOpacity from 1.0 to 0.3, move a bone twice with modelUpdate, drive a morph target repeatedly — each message now reaches ApplyRender() instead of being dropped. A texture that arrives late is picked up on the retry instead of being ignored. One limit worth stating plainly: a repeat message with gltfOpacity: 1.0 is now processed, but it does not restore the object's opacity. Every transparency mutation sits inside the if (json.GltfOpacity < 1f) gate at ArenaMaterialExtras.cs:54-92, which has no else branch, so the material stays translucent. Restoring opacity requires that missing else branch to undo the shader swap, blend modes, keywords and render queue; this PR does not add it.

ArenaClickListener, ArenaMaterialExtras, ArenaModelUpdate and ArenaGltfMorph each declared private new void Update(), which hides ArenaComponent.Update() (Runtime/Components/ArenaComponent.cs:25-32) rather than extending it. That base method is the only caller of ApplyRender(), and Unity dispatches one Update per MonoBehaviour, so the base never ran: the *Applied latch set on the first pass was never cleared, and the one asset-arrival retry that sets apply = true (ArenaMaterialExtras.cs:117) was dead code.

How: the base Update() was already protected virtual, so each of the four now declares protected override void Update() and calls base.Update() as its first statement, keeping its own per-frame work byte-for-byte unchanged. This matches the existing idiom at Runtime/ArenaClientScene.cs:354. Calling the base first means the latch is cleared and the work redone in the same frame the message arrives, rather than one frame later. After this change, no new void Update/Start/OnValidate remains anywhere in Runtime/ or Editor/.

The reachable throw. ArenaClickListener.ApplyRender() was the package's only throw new System.NotImplementedException(), unreachable precisely because ApplyRender() never ran; this change makes an inbound click-listener message reach it. I chose the warning, not an implementation, because neither field is a one-liner with an obvious meaning: enabled ("Publish events, set false to disable") could reasonably mean stop publishing, stop building colliders, or disable the component, and bubble has no direct Unity analogue — Unity does not propagate mouse events to scene-graph parents, and ArenaClickListenerModel.ForwardEvent() walks up to the nearest ancestor listener, so the Unity side already behaves closer to bubble: false than to the schema default of true. Guessing here would be a behaviour change the maintainer has not asked for. So ApplyRender() now logs a single Debug.LogWarning naming both fields, and only when the scene actually asks for a non-default value, so ordinary click-listener objects stay silent. The stale // DONE: enabled line in the conversion-status header was corrected to // TODO: enabled to match.

Closes #220

How to test in the Editor

  1. Open a scene with an ARENA gltf-model object that has material-extras.
  2. From the ARENA web client (or arena-py), send material-extras with gltfOpacity: 0.3 to that object. Confirm it goes translucent — this already worked before.
  3. The key case: send a second material-extras to the same object with gltfOpacity: 1.0. Before this PR the message was dropped entirely; it should now be processed — confirm via the retry path that the message is handled at all. It will not go opaque again: the if (json.GltfOpacity < 1f) gate at ArenaMaterialExtras.cs:54-92 has no else branch, so nothing reverts the transparency settings. That else-branch is not part of this PR.
  4. Same test for modelUpdate: send two consecutive messages moving/rotating the same named bone to two different values, and confirm the second one lands.
  5. Same for gltf-morph: send two value updates for one morphtarget and confirm the second weight applies.
  6. overrideSrc retry: send material-extras with an overrideSrc URL that is not cached locally yet. The texture should appear once the asset download completes, without a further message.
  7. click-listener regression: add a click-listener object and confirm clicks still publish mousedown/mouseup/mouseenter/mouseleave clientEvents, that colliders are still built for both primitives and gltf models, and that no exception is thrown. With enabled: false or bubble: false you should see one warning line per message instead of a crash.

Untested

This is untested. There is no Unity Editor and no Unity licence in the environment this was written in, and no CI will run on this PR — the workflow that adds pull_request triggers exists only in unmerged draft PR #183, and the dotnet test project under Tests~/DotNet/ is likewise not on main (and links only UnityEngine-free sources, so it could not cover this anyway). Nothing here has been compiled. Please treat the Editor steps above as the actual verification.

Notes for review

  • This change gates the observable behaviour of several other open issues: any fix that relies on ApplyRender() re-running for these four components (repeat-message handling, asset-arrival retries) cannot be seen working until this lands.
  • Known regression this change activates, deliberately left for a follow-up: ArenaMaterialExtras's per-frame work is not idempotent — material.renderQueue += (int)json.RenderOrder (ArenaMaterialExtras.cs:99) adds to the current queue rather than setting it. On main this was harmless only because the repeat message was dropped, so the queue never moved after the first pass. With ApplyRender() now re-running, a non-1.0 renderOrder climbs by renderOrder on every inbound message and every Inspector edit — specifically when gltfOpacity >= 1 and transparentOccluder is false, since those are the two paths that would otherwise reset the queue to a fixed value (3000 and 1999). This is a regression introduced by this PR, not a pre-existing quirk. Fixing it needs a decision I do not want to make inside this diff — snapshot the pristine queue per material and add to that, or anchor the addition to material.shader.renderQueue — so it is deliberately left for a follow-up and should get its own issue.
  • No .meta files, no reformatting, no autogenerated schema files touched.

Generated by Claude Code

ArenaClickListener, ArenaMaterialExtras, ArenaModelUpdate and
ArenaGltfMorph declared "private new void Update()", hiding the base
ArenaComponent.Update() that is the only caller of ApplyRender(). Unity
dispatched only the derived method, so the *Applied latch was never
cleared and the second and later inbound material-extras, modelUpdate
and gltf-morph messages were dropped.

Each now overrides Update() and calls base.Update() first, keeping its
own per-frame work unchanged. ArenaClickListener.ApplyRender(), which
this makes reachable, no longer throws NotImplementedException; it warns
once per inbound message when a non-default enabled/bubble is requested.

Closes #220
@claude claude Bot changed the title Fix four components hiding ArenaComponent.Update() fix: four components hide ArenaComponent.Update(), dropping repeat attribute messages Aug 27, 2026
claude Bot pushed a commit that referenced this pull request Aug 27, 2026
`json.ColorWrite` is never read anywhere in the package. The only
`_ColorWriteMask` writes (ArenaMaterialExtras.cs:105-106) are gated on
`json.TransparentOccluder`, a different property, so `colorWrite` on the
wire is silently ignored.

This marker was deferred to a sibling PR, but no open PR corrects it:
#238's only hunk in this file converts `private new void Update()` to
`protected override void Update()` and leaves the status block untouched.
Correcting it here so #227 does not auto-close with a known-wrong marker.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013e7rG6DQLvDnk8jumh9DJF
@claude
claude Bot force-pushed the fix/component-update-hiding branch from 261c7e5 to 3676161 Compare August 27, 2026 18:10
claude Bot pushed a commit that referenced this pull request Aug 27, 2026
`json.ColorWrite` is never read anywhere in the package. The only
`_ColorWriteMask` writes (ArenaMaterialExtras.cs:105-106) are gated on
`json.TransparentOccluder`, a different property, so `colorWrite` on the
wire is silently ignored.

This marker was deferred to a sibling PR, but no open PR corrects it:
#238's only hunk in this file converts `private new void Update()` to
`protected override void Update()` and leaves the status block untouched.
Correcting it here so #227 does not auto-close with a known-wrong marker.
@claude
claude Bot force-pushed the fix/component-update-hiding branch from 3676161 to dee6316 Compare August 27, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: four components hide ArenaComponent.Update(), so repeat material-extras / modelUpdate / gltf-morph messages are dropped

1 participant