feat: Implement QuickLoop (SOFIE-2878) #1112
Conversation
jstarpl
added
the
Contribution from BBC
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release51 #1112 +/- ##
=============================================
- Coverage 57.74% 57.63% -0.12%
=============================================
Files 512 520 +8
Lines 82571 84258 +1687
Branches 4307 4399 +92
=============================================
+ Hits 47679 48559 +880
- Misses 34841 35643 +802
- Partials 51 56 +5 ☔ View full report in Codecov by Sentry. |
jstarpl
changed the title
| * Original values of properties overriden by some features. | ||
| * Currently this only supports the QuickLoop | ||
| */ | ||
| overridenProperties?: Partial<NullableProps<IBlueprintPart>> |
There was a problem hiding this comment.
I'm not sure that I like this.
I am not convinced that it can be used by anything else safely, because it needs to be possible to 'undo' the overrides when leaving quickloop, which will be hard if the contents of this are partially from some unrelated feature.
I don't have any good ideas for alternative approaches right now.
There was a problem hiding this comment.
Notes from discussion:
Could we move this to the PartInstance instead? It seems that it's only used in playout-cases where we should have a PartInstance?
More questions:
- should adlibActions see the overridden or original value when aorking on the PartInstance?
- when we sync changes to PartInstance, are the
overridenPropertiestaken into account?
TODOs:
- rename to
overriddenProperties - change the type to not be as generic.
{expectedDuration, autoNext}would be enough to achieve this use case, and limits the future scope.
There was a problem hiding this comment.
After discussion, I am leaning towards that this will be rather hard to ensure behaviour is correct in all scenarios.
See the questions in the comment above.
It feels like persisting these changes could be avoided and calculated on the fly. For the ui, this could be achieved by a custom UIPartInstances publication (to save having to chase the change through hundreds of references). For the backend, only the playout logic should be interested in this and I can't imagine there to be too many references to the properties which could be overridden, so changing these to use some helper functions to on the fly calculate the overrides and use those may not be too painful
I am open to other solutions too
| /** | ||
| * A playout UI version of Parts. | ||
| */ | ||
| export const UIParts = createSyncCustomPublicationMongoCollection(CustomCollectionName.UIParts) // TODO |
There was a problem hiding this comment.
It's more of a to-ask-you, whether this is the right place for it. This collection stands out as the only UI collection in meteor/lib, while the other are in meteor/client, but it works, so I hope it's fine...? It's like that because this collection is used by RundownPlaylistCollectionUtil
There was a problem hiding this comment.
Hmm yeah, it probably shouldn't be in meteor/lib as it wont work from the server-side.
But moving it is a little non-trivial as it gets used by RundownPlaylistCollectionUtil which has other methods which have to reside in lib.
I suppose the proper solution would be to move the methods which need this UIParts into a client specific lib file. @jstarpl any thoughts on this and suggestions on where to put it?
There was a problem hiding this comment.
It looks like the places this could be called from are all in the client, via a few methods in lib.
So I think the solution here is to verify this and move the relevant code into the client
There was a problem hiding this comment.
Thanks, it indeed turned out possible! I addressed this in 36dc3ce
| invalidateRundownIds: RundownId[] | ||
| invalidateSegmentIds: SegmentId[] | ||
| invalidatePartIds: PartId[] |
There was a problem hiding this comment.
These properties don't appear to be used, only set
There was a problem hiding this comment.
Same as #1112 (comment)
| // We know that `collection` does diffing when 'commiting' all of the changes we have made | ||
| // meaning that for anything we will call `replace()` on, we can `remove()` it first for no extra cost |
There was a problem hiding this comment.
this is true, but when we know that a select parts/segments have changed it would be nice to not recalculate every part in the playlist.
While the publication won't emit those which have not changed, that relies on doing a deep-equals check of each part. As the publicData can be arbitrarily large, this may not be cheap when performing this for hundreds of parts
There was a problem hiding this comment.
This code comment is taken from other publications. Are you're suggesting that I actually use invalidatePartIds and so on, mentioned in your previous comment? I eventually decided to invalidate everything every time. I assumed that it is relatively cheap because diffObject was doing a shallow comparison of objects' properties, as its documentation would suggest: * Perform a shallow diff of a pair of objects
There was a problem hiding this comment.
I think the docs of that function are misleading/incorrect? It is calculating a shallow difference, but it is comparing each property with a deep equality check.
But even so, inside the replace call, it is doing a clone of whatever you pass in, so its never going to be a cheap/quick diff.
So yes, I think this does need to be cleverer on what it updates each time it is run.
At least handling the cases which can be easily cheap, such as when a part/segment changes it shouldnt need to invalidate everything. When the playlist quickLoop prop changes, I expect this could get very complicated to figure out exactly what should be invalidated. But it also won't change too often, so could get away with being less optimal.
Or perhaps take inspiration from what I did in the PieceContentStatus publication. Another approach would be to re-generate each Part as it changes. Then each time the publication changes, iterate through each part in the collection and update just the properties which quickloop may override. Be aware that the updateAll and updateOne methods of the collection don't perform any diffing themselves (and will always perform a clone), so manually diffing of the changes will be required to avoid making the ddp layer doing a deep comparison
There was a problem hiding this comment.
This is addressed in a793397. Please let me know what you think.
|
A workshop has been held on 2024/04/12 and the contributor will still address the comments above. |
| } | ||
| } | ||
|
|
||
| function stringsToIndexLookup(strings: string[]): Record<string, number> { |
There was a problem hiding this comment.
It might be nicer if this returned a Map, then it could preserve the RundownId or whatever ProtectedString typings
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a:
Feature
Current Behavior
Currently Sofie allows looping of an entire Rundown Playlist. Whether the playlist should be looping can only be set by the Blueprints (usually done according to properties coming from the NRCS).
New Behavior
What it does:
How to use it:
Set Markers:
Activate QuickLoop:
Modify the QuickLoop:
Exit QuickLoop Mode:
Deviations from the RFC, and some more details:
Testing Instructions
Setting and Clearing Markers:
Loop Mode Functionality:
Additional Scenarios:
Testing Instructions for Studio Settings:
Enable QuickLoop:
Force Auto in a Loop:
QuickLoop Fallback Part Duration:
Regression testing instructions for the existing Playlist Looping Feature:
Other Information
Status