feat: allow hotcuts while using transition property#46
Conversation
PeterC89
added
the
contribution from BBC
There was a problem hiding this comment.
Pull request overview
Adds support for “hot cuts” when controlling ME program/preview via the custom transition property, aiming to allow preview-bus control during cuts (with safety constraints around selected keyers).
Changes:
- Refactors
MixEffecttyping to always extendatem-connection’sVideoState.MixEffect, while keepinginputas a deprecated alias andtransitionoptional. - Updates
resolveMixEffectsStateto choose between hot-cut (ProgramInputCommand) vs preview+cut sequencing based ontransitionandnextSelection. - Updates and expands unit tests around program/preview and deprecated
inputbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/state.ts | Makes MixEffect always extend the upstream VideoState.MixEffect, adding optional transition and deprecated input. |
| src/resolvers/mixEffect.ts | Implements hot-cut vs preview+cut behavior and changes preview/program command emission logic. |
| src/resolvers/tests/mixEffect.spec.ts | Updates tests for the new command ordering/behavior and adds coverage for deprecated input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const programInput = newMixEffect?.input ?? newMixEffect?.programInput | ||
| const oldProgramInput = oldMixEffect?.input ?? oldMixEffect?.programInput |
There was a problem hiding this comment.
The deprecated input field is currently preferred over programInput (newMixEffect?.input ?? newMixEffect?.programInput). If both are present, this will ignore the non-deprecated field and could produce surprising results. Prefer programInput first and only fall back to input when programInput is undefined.
| const programInput = newMixEffect?.input ?? newMixEffect?.programInput | |
| const oldProgramInput = oldMixEffect?.input ?? oldMixEffect?.programInput | |
| const programInput = newMixEffect?.programInput ?? newMixEffect?.input | |
| const oldProgramInput = oldMixEffect?.programInput ?? oldMixEffect?.input |
There was a problem hiding this comment.
If both are present, this will ignore the non-deprecated field and could produce surprising results
Surprising behaviour that happens to be consistent with the previous behaviour and therefore backwards compatible? I guess trying to not introduce breaking changes would be novel for an LLM.
| if (canHotCut && oldMixEffect?.previewInput !== newMixEffect?.previewInput) { | ||
| // set preview when there is no auto transition command | ||
| commands.push( | ||
| new AtemCommands.PreviewInputCommand(mixEffectId, newMixEffect?.previewInput ?? Defaults.Video.defaultInput) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Preview bus updates are now gated by canHotCut (if (canHotCut && oldMixEffect?.previewInput !== newMixEffect?.previewInput)), which means a plain preview change will emit no command whenever a USK is selected for the next transition. Previously preview changes were always applied when programPreview diffing was enabled. Consider handling previewInput diffs independently of canHotCut (and, when doing a Preview+Cut sequence, apply the requested preview after the cut if both program+preview are changing).
| ME2.previewInput = 2 | ||
| ME2.programInput = 1 | ||
| ME2.transition = Enums.TransitionStyle.CUT | ||
| const commands = ME.resolveMixEffectsState([ME1], [ME2], fullDiffObject) as Array<Commands.PreviewInputCommand> |
There was a problem hiding this comment.
Type assertion in this test casts the result to Array<Commands.PreviewInputCommand>, but the expectation checks for a ProgramInputCommand at index 0. Using a union type here (or avoiding the cast) will keep the test type-safe and prevent future refactors from being masked by an overly narrow cast.
| const commands = ME.resolveMixEffectsState([ME1], [ME2], fullDiffObject) as Array<Commands.PreviewInputCommand> | |
| const commands = ME.resolveMixEffectsState( | |
| [ME1], | |
| [ME2], | |
| fullDiffObject | |
| ) as Array<Commands.ProgramInputCommand | Commands.PreviewInputCommand> |
| const commands = ME.resolveMixEffectsState([ME1], [ME2], fullDiffObject) as Array<Commands.PreviewInputCommand> | ||
| expect(commands).toHaveLength(2) |
There was a problem hiding this comment.
Same as above: this assertion narrows to Array<Commands.PreviewInputCommand> while the test asserts a ProgramInputCommand is present. Widen the asserted type (eg a union) so TypeScript can help catch incorrect assumptions about command ordering/types.
| const canHotCut = | ||
| transition === Enums.TransitionStyle.CUT && | ||
| !newMixEffect?.transitionProperties.nextSelection.find( | ||
| (layer) => layer !== ConnectionEnums.TransitionSelection.Background | ||
| ) | ||
|
|
||
| if (newMixEffect.transition === Enums.TransitionStyle.CUT) { | ||
| commands.push(new AtemCommands.CutCommand(mixEffectId)) | ||
| } else if (newMixEffect.transition !== Enums.TransitionStyle.DUMMY) { | ||
| if (newMixEffect.transition !== oldMeTransition) { | ||
| if (programInput !== oldProgramInput) { | ||
| switch (transition) { | ||
| // cut to new source | ||
| case Enums.TransitionStyle.CUT: | ||
| if (canHotCut) { | ||
| commands.push( | ||
| new AtemCommands.ProgramInputCommand(mixEffectId, programInput ?? Defaults.Video.defaultInput) | ||
| ) | ||
| } else { | ||
| commands.push( | ||
| new AtemCommands.PreviewInputCommand(mixEffectId, programInput ?? Defaults.Video.defaultInput), | ||
| new AtemCommands.CutCommand(mixEffectId) | ||
| ) | ||
| } | ||
| break |
There was a problem hiding this comment.
The new canHotCut / Preview+Cut branching introduces important behavior (eg when USKs are selected in nextSelection). There are unit tests for the hot-cut path, but no coverage for the non-hotcut path (where a CutCommand is used) or for how preview updates behave when keyers are selected. Adding tests for those cases would prevent regressions and clarify the intended semantics.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Caution Review failedPull request was closed or merged during review WalkthroughConsolidates transition decision flow across downstream/upstream keyers and mix effects, changes several resolver return types to include a doTransition flag, narrows MixEffect typing, and updates tests to use centralized getState initialization and new command/field semantics. (43 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Orch as Orchestrator (resolvers/index)
participant DSK as DownstreamKeyer Resolver
participant USK as UpstreamKeyer Resolver
participant ME as MixEffect Resolver
participant CmdQ as CommandQueue
participant ATEM as ATEM Device
Orch->>DSK: resolveDownstreamKeyerState(oldDsks, newDsks, diffOptions)
DSK-->>Orch: { commands, doTransition }
Orch->>USK: resolveUpstreamKeyerState(mixEffectId, oldUSK, newUSK, newTransSelection, diffOptions)
USK-->>Orch: { commands, doTransition }
Orch->>ME: resolveMixEffectsState(oldME, newME, diffOptions, doTransition)
ME-->>Orch: { commands }
Orch->>CmdQ: enqueue DSK/USK/ME commands
CmdQ->>ATEM: execute queued commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolvers/mixEffect.ts`:
- Around line 55-112: The code currently only pushes a PreviewInputCommand when
canHotCut is true, so if transition is MIX/WIPE/etc and only previewInput
changed no preview command is emitted; fix by tracking whether an
auto-transition was queued (e.g., set a boolean like autoTransitionQueued when
you push AutoTransitionCommand in the default branch) and then replace the
canHotCut check at the later preview-update block with a check that
previewUpdateNeeded = oldMixEffect?.previewInput !== newMixEffect?.previewInput
&& !autoTransitionQueued; if previewUpdateNeeded push a PreviewInputCommand for
newMixEffect.previewInput; keep existing canHotCut behavior for CUT branch but
ensure the flag is set only when AutoTransitionCommand is actually enqueued
(symbols: programPreview, canHotCut, transition, AutoTransitionCommand,
PreviewInputCommand, TransitionPropertiesCommand, TransitionPositionCommand).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolvers/mixEffect.ts`:
- Around line 93-99: The check uses oldMixEffect?.transitionProperties?.style
(readonly current style) but the command updates nextStyle, so change the
comparison to compare transition against
oldMixEffect?.transitionProperties?.nextStyle; update the condition that
currently reads if (transition !== (oldMixEffect?.transition ??
oldMixEffect?.transitionProperties?.style)) to compare with nextStyle instead,
ensuring the AtemCommands.TransitionPropertiesCommand(mixEffectId) updateProps({
nextStyle: ... }) is emitted when the pending nextStyle actually differs.
| case Enums.TransitionStyle.CUT: | ||
| if (canHotCut) { | ||
| commands.push( | ||
| new AtemCommands.ProgramInputCommand(mixEffectId, programInput ?? Defaults.Video.defaultInput) |
There was a problem hiding this comment.
Should each of these be doing ?? Defaults.Video.defaultInput or should that be done when defining programInput?
Im wondering what the correct behaviour is when programInput = 0 and oldProgramInput = undefined. That will be seen as a change, even though they both resolve to 0
| const transition = newMixEffect?.transition ?? Enums.TransitionStyle.CUT | ||
| const canHotCut = | ||
| transition === Enums.TransitionStyle.CUT && | ||
| !newMixEffect?.transitionProperties?.nextSelection?.find( |
There was a problem hiding this comment.
I think checking the dsk should be included here too, as that can be tied to the cut too
| } | ||
|
|
||
| if ( | ||
| (oldProgramInput === programInput || canHotCut) && |
There was a problem hiding this comment.
I think this canHotCut could be loosened to transition === Enums.TransitionStyle.CUT.
Even if we are doing a pvw+cut, I think it will be safe to add another pvw immediately after that too.
Maybe if it did a pvw+cut it should always be sending this pvw command too, to ensure it ends up in the desired state in case it was mangled when changing pgm? That would get us closer to a state where pvw is predictable (I think just leaving cases when a non-cut transition is used)
There was a problem hiding this comment.
That's an interesting thought, I'll run it up against an atem to verify
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/resolvers/upstreamKeyers/index.ts (1)
104-111: Logic correct but bitmask calculation could be clearer.The calculation
Math.pow(2, upstreamKeyerId + 1)computes theTransitionSelectionenum value for each keyer (Key1=2, Key2=4, etc.). The logic correctly:
- Pushes direct
OnAirCommandwhen the keyer is NOT in the transition selection- Signals
doTransitionwhen the keyer IS in the transition selection (to be toggled via cut/auto)Consider using the enum directly for clarity:
const keyerSelectionValue = Math.pow(2, upstreamKeyerId + 1) as ConnectionEnums.TransitionSelection const isInTransition = newStateTransSelection?.includes(keyerSelectionValue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolvers/upstreamKeyers/index.ts` around lines 104 - 111, The bitmask calculation for detecting if a keyer is in TransitionSelection is unclear; replace the inline Math.pow(2, upstreamKeyerId + 1) usage with an explicit enum-derived value (e.g., compute keyerSelectionValue as the appropriate ConnectionEnums.TransitionSelection for upstreamKeyerId) and use that to set a clearer flag (isInTransition) instead of the current transitions variable, then use isInTransition in the existing conditional that decides between creating an AtemCommands.MixEffectKeyOnAirCommand (respecting runOrderGroup logic) or setting doTransition when thisDiffOptions.onAir and oldKeyer.onAir !== newKeyer.onAir.src/resolvers/__tests__/downstreamKeyer.spec.ts (1)
29-32: Tests adapted correctly, but missing coverage fordoTransitionbehavior.The tests properly destructure
{ commands }from the return value. However, there's no test verifying thatdoTransitionis returned astruewhen a DSK'sonAirchanges whileproperties.tieis set. Consider adding a test case:test('Unit: Downstream keyer: onAir with tie returns doTransition', function () { DSK2[0].properties.tie = true DSK2[0].onAir = true const { commands, doTransition } = resolveDownstreamKeyerState(DSK1, DSK2, fullDiff) expect(doTransition).toBe(true) expect(commands.filter(c => c.constructor.name === 'DownstreamKeyOnAirCommand')).toHaveLength(0) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolvers/__tests__/downstreamKeyer.spec.ts` around lines 29 - 32, Add a unit test for resolveDownstreamKeyerState that verifies doTransition is returned true when a downstream keyer's onAir changes and its properties.tie is true: mutate the test fixture (set DSK2[0].properties.tie = true and DSK2[0].onAir = true), call resolveDownstreamKeyerState(DSK1, DSK2, fullDiff), and assert that the returned doTransition is true and that no DownstreamKeyOnAirCommand instances are present in the commands array (e.g., filter commands by constructor.name === 'DownstreamKeyOnAirCommand' and expect length 0).src/resolvers/mixEffect.ts (1)
107-113: Preview update condition may drop preview-only changes during auto transitions.The condition allows preview updates when:
- Program input didn't change (
oldProgramInput === programInput), OR- No transition is happening (
!doTransition), OR- Transition is CUT
However, if
doTransitionis true due to USK/DSK changes (not program change), and the transition is MIX/WIPE, preview-only changes will still be dropped. This is an improvement over the previous code but may still have edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolvers/mixEffect.ts` around lines 107 - 113, The current preview-update check in mixEffect.ts (using oldProgramInput, programInput, doTransition, transition, Enums.TransitionStyle.CUT, previewInput, oldPreviewInput and AtemCommands.PreviewInputCommand with mixEffectId) can still drop preview-only changes when doTransition is true due to USK/DSK but program didn't change; update the condition so preview updates are only suppressed when the program input actually changed AND a non-CUT transition is in progress. Concretely, replace the if-condition so it allows sending PreviewInputCommand whenever previewInput !== oldPreviewInput unless (oldProgramInput !== programInput && doTransition && transition !== Enums.TransitionStyle.CUT); keep using the same variables and push the PreviewInputCommand as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolvers/__tests__/mixEffect.spec.ts`:
- Around line 91-93: The type annotation for the commands variable in the test
for resolveMixEffectsState incorrectly repeats Commands.PreviewInputCommand;
remove the duplicate and replace it with the intended union (or a single type) —
e.g. use Array<Commands.PreviewInputCommand | Commands.ProgramInputCommand> if
both are expected, or simply Array<Commands.PreviewInputCommand> if only that
type is returned; update the declaration where ME.resolveMixEffectsState is
cast.
In `@src/resolvers/index.ts`:
- Around line 24-44: The DSK onAir changes can be lost if
resolveDownstreamKeyerState sets doTransition=true but resolveMixEffectsState is
not called (diffOptions.video?.mixEffects falsy), so update the flow to provide
a fallback: after calling resolveDownstreamKeyerState (function
resolveDownstreamKeyerState) check if doTransition is true and
diffOptions.video?.mixEffects is falsy (or resolveMixEffectsState will not run),
and in that case push explicit DownstreamKeyOnAirCommand(s) for the tied DSK(s)
whose onAir changed; ensure you reuse the same keyer identifiers and state from
the dskDiff (or downstream keyer diff result) so activation is identical to what
resolveMixEffectsState would do.
In `@src/resolvers/mixEffect.ts`:
- Around line 54-56: The bug is that doTransition is being overwritten by
uskDiff.doTransition instead of combined with the existing DSK transition state;
update the assignment in the mixEffect resolver so that after merging
uskDiff.commands you combine the transition flags (e.g., set doTransition to the
logical OR of the existing doTransition/dskTransition and uskDiff.doTransition)
rather than replacing it, keeping the downstream keyer's request if it was true;
locate the code that currently does doTransition = uskDiff.doTransition and
change it to combine both flags (preserving existing doTransition) so any true
value from dskTransition or uskDiff.doTransition is honored.
- Around line 63-68: The empty-array branch in computing transitionIsBg is
unreachable because nextSelection is always set from
newMixEffect?.transitionProperties?.nextSelection with a fallback to
Defaults.Video.TransitionProperties.nextSelection (which is not empty); either
remove the length check or explicitly document intent. Fix by updating the
transitionIsBg expression in mixEffect.ts (where nextSelection is assigned) to
either: 1) remove the "nextSelection.length === 0 ||" clause and rely solely on
checking for Background via nextSelection.find(layer => layer ===
ConnectionEnums.TransitionSelection.Background), or 2) if an empty array should
semantically mean “background only” add a concise comment above the assignment
explaining that empty nextSelection is treated as Background and keep the check;
reference nextSelection, Defaults.Video.TransitionProperties.nextSelection,
transitionIsBg, and ConnectionEnums.TransitionSelection.Background when making
the change.
---
Nitpick comments:
In `@src/resolvers/__tests__/downstreamKeyer.spec.ts`:
- Around line 29-32: Add a unit test for resolveDownstreamKeyerState that
verifies doTransition is returned true when a downstream keyer's onAir changes
and its properties.tie is true: mutate the test fixture (set
DSK2[0].properties.tie = true and DSK2[0].onAir = true), call
resolveDownstreamKeyerState(DSK1, DSK2, fullDiff), and assert that the returned
doTransition is true and that no DownstreamKeyOnAirCommand instances are present
in the commands array (e.g., filter commands by constructor.name ===
'DownstreamKeyOnAirCommand' and expect length 0).
In `@src/resolvers/mixEffect.ts`:
- Around line 107-113: The current preview-update check in mixEffect.ts (using
oldProgramInput, programInput, doTransition, transition,
Enums.TransitionStyle.CUT, previewInput, oldPreviewInput and
AtemCommands.PreviewInputCommand with mixEffectId) can still drop preview-only
changes when doTransition is true due to USK/DSK but program didn't change;
update the condition so preview updates are only suppressed when the program
input actually changed AND a non-CUT transition is in progress. Concretely,
replace the if-condition so it allows sending PreviewInputCommand whenever
previewInput !== oldPreviewInput unless (oldProgramInput !== programInput &&
doTransition && transition !== Enums.TransitionStyle.CUT); keep using the same
variables and push the PreviewInputCommand as before.
In `@src/resolvers/upstreamKeyers/index.ts`:
- Around line 104-111: The bitmask calculation for detecting if a keyer is in
TransitionSelection is unclear; replace the inline Math.pow(2, upstreamKeyerId +
1) usage with an explicit enum-derived value (e.g., compute keyerSelectionValue
as the appropriate ConnectionEnums.TransitionSelection for upstreamKeyerId) and
use that to set a clearer flag (isInTransition) instead of the current
transitions variable, then use isInTransition in the existing conditional that
decides between creating an AtemCommands.MixEffectKeyOnAirCommand (respecting
runOrderGroup logic) or setting doTransition when thisDiffOptions.onAir and
oldKeyer.onAir !== newKeyer.onAir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0acc54e-ff1e-4b7e-910d-808544e60728
📒 Files selected for processing (7)
src/resolvers/__tests__/downstreamKeyer.spec.tssrc/resolvers/__tests__/mixEffect.spec.tssrc/resolvers/downstreamKeyer.tssrc/resolvers/index.tssrc/resolvers/mixEffect.tssrc/resolvers/upstreamKeyers/__tests__/upstreamKeyer.spec.tssrc/resolvers/upstreamKeyers/index.ts
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a:
Improvement (someone may argue a fix?)
Current Behavior
It's not possible to control the preview bus while performing cuts using the "input"/"transition" properties
New Behavior
It's possible to control the preview bus while performing cuts but only when the transition is set to Cut and there is no currently selected upstream keyers
Testing Instructions
Unit tests seem to imply that behaviour remains as it was before
Other Information
This mechanism is similar to the one used in Sofie's VMix integration
I believe this is fully backwards compatible with existing behaviour with one exception where the user wishes to set the Program Bus while adjusting the selected upstream keyers without taking those to air. The alternative is to not allow for Cuts to take these Keyers on air at all which may be more in line with what the state of the USK's is saying? @Julusian I'd love to hear your thoughts on this one.
Status