Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: A series of refactorings and fixes in vMix integration #315

Conversation

ianshade
Copy link
Contributor

@ianshade ianshade commented Dec 18, 2023

About the Contributor

This PR is being opened on behalf of TV 2 Norge.

Type of Contribution

This is a:

Code improvement that also tackles some issues that can be considered as bugs

Current Behavior

  • Total control of the features available in this integration is assumed, causing TSR to reset properties that may be intended to be controlled manually or left at the defaults stored in the Preset. This causes several conflicts:
    • audio properties of inputs are reset even when not intended to be controlled by the TSR, but for example with other tools like Sisyfos (even though separate mappings for audio already exist, the state is combined together with other input properties)
    • layer (overlay / multi view) properties of inputs are incorrectly parsed from vMix and reset at unexpected times
    • output assignments are reset
    • streaming, recording, ..., are turned off
  • Because of the retry functionality, commands resetting the Alpha value are sent to all inputs every time the real state is polled (the value is not available in the XML returned by vMix)
  • Inputs previously added by TSR are incorrectly treated as fixed inputs when TSR is restarted
  • Usage of deepMerge causes duplication in some arrays
  • Some debug events are emitted without respecting the debugLogging flag

New Behavior

  • The code is split into multiple classes
  • Similarly to the TriCaster integration, a resource is controlled (and may receive TSR-default state) on an opt-in basis, only when a layer mapping associated with it exists. This includes:
    • inputs (each one individually)
    • audio channels (each one individually)
    • outputs (each one individually)
    • overlays (each one individually)
    • recording
    • streaming
    • external
    • mixes (each one individually)
  • Readme file describing implementation details and limitations is added
  • The retry functionality is refactored. Only relevant properties of inputs are synced, not deeply merging the whole state anymore. An edge case potentially causing duplicate commands to be sent due to a response to polling arriving after commands from a new resolving are sent is (hopefully) solved.
  • Commands setting Alpha are not re-sent repeatedly as a result of polling
  • Input layers (overlay / multi view) parsing is fixed
  • Inputs added by TSR are handled separately from the pre-existing inputs for an improved recovery when restarting the TSR. Note: this might need further improvements that are out of the scope for this PR and partially depend on features already existing in release51.
  • Debug logging is done through emitDebug. Note: this needs further improvements to separate commands from other logs

Testing Instructions

Other Information

Many of the changes are based on #235.
Some TODOs and one uncommented test case are left for when this will be merged with release51.

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 264 lines in your changes are missing coverage. Please review.

Comparison is base (7a40f53) 35.80% compared to head (226d20e) 37.23%.

Files Patch % Lines
...rc/integrations/vmix/VMixTimelineStateConverter.ts 31.72% 83 Missing and 16 partials ⚠️
...-resolver/src/integrations/vmix/VMixStateDiffer.ts 64.87% 81 Missing and 4 partials ⚠️
...line-state-resolver/src/integrations/vmix/index.ts 0.00% 71 Missing and 5 partials ⚠️
...solver/src/integrations/vmix/VMixXmlStateParser.ts 94.00% 3 Missing ⚠️
...ver/src/integrations/vmix/VMixStateSynchronizer.ts 92.85% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           release50     #315      +/-   ##
=============================================
+ Coverage      35.80%   37.23%   +1.42%     
=============================================
  Files             99      105       +6     
  Lines          10062    10204     +142     
  Branches        2467     2484      +17     
=============================================
+ Hits            3603     3799     +196     
+ Misses          5891     5856      -35     
+ Partials         568      549      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mint-dewit mint-dewit self-assigned this Dec 20, 2023
@ianshade ianshade marked this pull request as ready for review December 20, 2023 09:58
Copy link
Contributor

@mint-dewit mint-dewit left a comment

Choose a reason for hiding this comment

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

I'm happy to accept this into R50 since they are constrained to just the VMix integration which is not in use by NRK right now.

Before merging I just want to see the naming of the files be camelCase for consistency.

@jesperstarkar jesperstarkar changed the base branch from release50 to 50-50 January 10, 2024 13:29
@jesperstarkar jesperstarkar changed the base branch from 50-50 to release50 January 10, 2024 13:32
@jesperstarkar jesperstarkar changed the base branch from release50 to release49 January 10, 2024 13:42
@jesperstarkar jesperstarkar changed the base branch from release49 to release50 January 10, 2024 13:43
@ianshade ianshade force-pushed the contribute/EAV-142/fix-vmix-media-reload-retrying-release50 branch from 9f98967 to 5dc8fec Compare January 10, 2024 13:50
@jesperstarkar jesperstarkar merged commit fdb9a08 into nrkno:release50 Jan 10, 2024
17 of 18 checks passed
@jesperstarkar jesperstarkar deleted the contribute/EAV-142/fix-vmix-media-reload-retrying-release50 branch January 10, 2024 13:56
@jesperstarkar jesperstarkar added the contribution from TV 2 Norge Contributions sponsored by TV 2 Norge (tv2.no) label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution from TV 2 Norge Contributions sponsored by TV 2 Norge (tv2.no)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants