Repository navigation
feat: wire video-control attribute through to Unity VideoPlayer - #245
Draft
claude[bot] wants to merge 3 commits into
Draft
claude[bot] wants to merge 3 commits into
claude[bot] wants to merge 3 commits into
Conversation
video-control never reached Unity: its dispatch arm was commented out, ArenaUnity.ApplyVideoControl did not exist, and ArenaVideoControl.ApplyRender() was an empty stub, so all 8 DTO fields were unmapped. Add the applier in the shape of its neighbours, uncomment the dispatch arm in place, and map video_object, video_path, video_loop, autoplay and volume onto the VideoPlayer that ArenaMaterial.AttachMaterialTexture already owns. frame_object and anyone_clicks keep their TODO markers; cleanup is marked N/A. Closes #224
…date video_path was never downloadable: DownloadAssets only runs for keys listed in msgUriTags, and pendingAssetCallbacks are only invoked from there, so checkLocalAsset always returned null and the RegisterAssetCallback callback never fired. Add "video_path" to msgUriTags alongside every other asset-bearing key. AttachMaterialTexture re-assigns vp.url, resets isLooping/playOnAwake/ renderMode and calls Play(), so any later video-control update (a volume change) restarted the clip. Attach only when the object has no VideoPlayer or the resolved absolute path differs from the current vp.url.
claude
Bot
force-pushed
the
feat/video-control
branch
from
August 27, 2026 18:10
ad05b54 to
2420ccf
Compare
…s late ApplyRender returned permanently when video_object named an object that had not arrived, or when the VideoPlayer that ArenaMaterial owns did not exist yet. Nothing re-armed apply, so the update was lost for the rest of the session, and the comment claiming a re-apply was wrong. Record the missing dependency and re-arm apply from Update until it appears, bounded by a 30s wall-clock deadline that logs one warning and stops. Also register a single-shot RegisterAssetCallback on the target's material.src when that asset is still downloading, so a download slower than the retry window still re-applies.
claude
Bot
force-pushed
the
feat/video-control
branch
from
August 27, 2026 18:26
998107b to
344c027
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by mwfarb · Slack thread
Before: publishing a
video-controlattribute to a scene had no effect at all in the Unity client. The dispatch arm was commented out (Runtime/ArenaClientScene.Messaging.cs:316),ArenaUnity.ApplyVideoControldid not exist, andArenaVideoControl.ApplyRender()was the empty// TODO: Implement this component if neededstub — so none of the 8 schema fields reached Unity, and the component was never even added to the object.After:
video-controlis dispatched like its neighbours. The component is added to the object, itsjsonis populated from the wire and shows in the Inspector, and looping, autoplay/pause and volume are applied to theVideoPlayeron the object that holds the video. A dependency that has not arrived yet no longer drops the update:RegisterAssetCallbackre-armsapplywhen the video asset lands, and a namedvideo_objector aVideoPlayerthat does not exist yet is retried fromUpdate()for a bounded window instead of being lost for the rest of the session.It makes
video-controla live inbound attribute in the Unity client for the fields that have a cleanVideoPlayercounterpart.How:
ArenaMaterial.AttachMaterialTexture(Runtime/Components/ArenaMaterial.cs:612-636) already owns theVideoPlayer— it creates one on theGameObjectcarrying theRenderer, points it at the local asset, setsisLooping/playOnAwakeand callsPlay(). Rather than stand up a competing player,ApplyRender()resolves the target object (video_objectviaArenaClientScene.Instance.arenaObjs, falling back to this object), handsvideo_pathtoAttachMaterialTexturethrough the samecheckLocalAsset/RegisterAssetCallbackpairArenaMaterialuses forsrc, then reads backvobj.GetComponent<UnityEngine.Video.VideoPlayer>()and sets the fields on it. Because the attach happens first, ourisLooping/playOnAwake/volume/pause writes land after the unconditionalPlay()insideAttachMaterialTexture. A named object that is missing, or aVideoPlayerthat does not exist yet, no longer returns for good:ApplyRender()records the missing dependency andUpdate()setsapply = trueagain on following frames until it appears, giving up after a 30 s wall-clock window with a single warning. Where the player is still waiting onmaterial.src,RegisterAssetCallbackis armed once as well, so a download slower than that window still re-applies. Both paths terminate: the retry is bounded by wall clock, and the asset callback is single-shot (Runtime/ArenaClientScene.Assets.cs:158-161removes it as it fires).Closes #224
Field mapping
video_objectArenaClientScene.Instance.arenaObjs(same idiom asArenaPhysxJoint.Target); falls back to this object when unsetvideo_pathArenaMaterial.AttachMaterialTexture, which owns theVideoPlayer;RegisterAssetCallbackre-armsapplyif the asset has not arrivedvideo_loopVideoPlayer.isLoopingautoplayVideoPlayer.playOnAwake, plusPlay()/Pause()so a laterautoplay: falseactually pausesvolumeaudioOutputMode = DirectthenSetDirectAudioVolume(0, volume), guarded oncontrolledAudioTrackCount > 0. Direct is the only output mode where the wire's scalar volume has a counterpart without inventing anAudioSource; worth a look during reviewframe_object// TODO:Runtime/Schemas/Attribute/ArenaVideoControlJson.cs:32-34, tooltip "URL of a thumbnail image"). Showing it needs its own image load and a swap to the video on play — not wiringanyone_clicks// TODO:ArenaClickListener, whose ownApplyRender()currently throwsNotImplementedExceptionand is not reached (see #220). Deliberately out of scope herecleanup// N/A:VideoPlayerwith theGameObjectHow to test in the Editor
material: { src: "store/users/<you>/videos/<clip>.mp4" }soAttachMaterialTexturecreates theVideoPlayer, or publishvideo-controlwithvideo_pathset and let this change create it.ArenaVideoControlcomponent with the wire values in its Inspector and aUnityEngine.Video.VideoPlayer.materialandvideo-controltogether in one message, once as{"material": …, "video-control": …}and once with the keys the other way round. The settings should land either way — that is the ordering race, and it is now retried on a following frame rather than dropped.video-controlwithautoplay: true— playback should start. Publish again withautoplay: false— it should pause where it is (this is the pause path; the schema has no separatepausefield). Note the open question above: on amaterial.srcvideo, avideo-controlpublish that omitsautoplaypauses it.volumechanges (0,0.5,1) and confirm the audio level follows. Note theVideoPlayer's audio track count is only known after prepare, so a volume sent before the clip is prepared may need one more publish to take.ArenaVideoControlJsonhas no time/seek field (the 8 fields areframe_object,video_object,video_path,anyone_clicks,video_loop,autoplay,volume,cleanup), so there is nothing on the wire to map toVideoPlayer.time. Worth confirming whether the ARENA schema is expected to grow one.video_loop: falseand confirm playback stops at the end instead of restarting.video-controlwithvideo_objectnaming an object that does not exist yet, then publish that object within 30 s — the settings should land as it arrives, where before the message was dropped for the session. Then name avideo_objectthat never arrives and confirm exactly onevideo-control gave up waiting for …warning after about 30 s, not one per frame.Untested
Stated plainly: 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_requesttriggers exists only on the unmerged draft #183. The diff was read adversarially (assembly-internal access toAttachMaterialTexture, fully-qualifiedUnityEngine.Video.*so no newusingis needed,ArenaClientSceneresolved from the enclosing namespace to avoid theArenaUnitynamespace/class CS0426 collision), but it has not been compiled. Please build before believing any of it.Notes for review
Update()hiding (fix: four components hide ArenaComponent.Update(), so repeat material-extras / modelUpdate / gltf-morph messages are dropped #220): checked, andArenaVideoControldoes not follow that pattern — it declares noprivate new void Update(). It now does overrideUpdate(), but asprotected overridethat callsbase.Update()(Runtime/Components/ArenaComponent.cs:25-32) before its own re-arm check, soApplyRender()is still reached on every inbound update. This component is unaffected by fix: four components hide ArenaComponent.Update(), so repeat material-extras / modelUpdate / gltf-morph messages are dropped #220. (Theanyone_clickswork would run into it, viaArenaClickListener.)Runtime/ArenaUnity.csand arms to the same dispatch table, and another changes thevisibleguard inUpdateObjectMessage. The hunks here are kept tight — one line uncommented in place in the dispatch table, one applier appended afterApplyModelUpdatejust before the// wire objectsdivider — but whichever lands second will likely need a trivial rebase in those two spots.casearms in the dispatch switch. That was wrong: the arms are a lookup, so their textual order means nothing. The order comes from the wire message.UpdateObjectMessageparses the inbound data intojData(Runtime/ArenaClientScene.Messaging.cs:66) and walks it withforeach (var result in jData)(:259), so attributes are handled in whatever JSON key order the publisher sent —video-control(:316) can be reached before or aftermaterial(:327). The appliers do no work of their own either:ArenaUnity.ApplyVideoControladds the component, deserializes the json and setsapply = true(Runtime/ArenaUnity.cs:418-424), and the actual rendering happens later, in each component'sUpdate(), in Unity's component order. So the real hazard is thatArenaVideoControlcan render beforeArenaMaterialhas created theVideoPlayer— which the bounded retry above now picks up on a following frame instead of dropping. Settingvideo_pathstill sidesteps the race by creating the player here.autoplaydefaults tofalseandanyone_clicksis unmapped.defAutoplay = false(Runtime/Schemas/Attribute/ArenaVideoControlJson.cs:78), whileAttachMaterialTexturestarts amaterial.srcvideo withplayOnAwake = trueand an unconditionalPlay()(Runtime/Components/ArenaMaterial.cs:612-636). Attachingvideo-controlto an object whose video came frommaterial.src, withoutautoplay: trueon the wire, therefore pauses a clip that was playing — and sinceanyone_clicksis not wired, there is no click-to-play to start it again. No behaviour was changed for this; it is a product decision, not something to guess at. The options are to leave it wire-faithful, to touch playback only whenautoplayis explicitly present in the message, or to wireanyone_clicksfirst. Please pick one.Generated by Claude Code