RFC: Customisable Take logic about sofie-core HOT 16 CLOSED

Overriding the default take with an adlib action sounds like a powerful idea for customisation but I do wonder a bit if it would also be a risk for an inconsistent user experience.

An alternative that would also allow for a greater deal of customisation would be to add a hook to blueprints that is called after a new part instance has been set as next. The blueprints may then modify this part instance as it sees fit, i.e. just like it can do from an adlib action.

A possible advantage of that suggestion is that it would reduce the amount of calculations (of inserting parts and pieces) during the take therefore keeping the latency of a system to a minimum.

from sofie-core.

On behalf of the BBC, I'd like to solve the problem presented in this RFC by adding two blueprint methods to the ShowStyleBlueprintManifest, with the following signatures:

/**
 * Called during a Take action.
 * Allows for part modification and/or aborting the take.
 */
onTake?: (context: IOnTakeContext) => Promise<void>

/**
 * Called when a part is set as Next, including right after a Take.
 * Allows for part modification.
 */
onSetAsNext?: (context: IOnSetAsNextContext) => Promise<void>

Contexts passed to onTake and onSetAsNext would expose a subset of methods identical or similar to those that we currently know from the IActionExecutionContext, allowing for part- and pieceInstance retrieval and modification. Whether they will be identical and can be extracted to a shared interface or not, hasn't been decided, but some slight differences might be necessary. As much of the underlying implementation as possible will be shared. This is the scope of those methods (for the reason stated, naming and types may vary):

getPartInstance(part: 'current' | 'next'): Promise<IBlueprintPartInstance | undefined>
getPieceInstances(part: 'current' | 'next'): Promise<IBlueprintPieceInstance[]>
getResolvedPieceInstances(part: 'current' | 'next'): Promise<IBlueprintResolvedPieceInstance[]>

insertPiece(part: 'current' | 'next', piece: IBlueprintPiece): Promise<IBlueprintPieceInstance>
updatePieceInstance(pieceInstanceId: string, piece: Partial<IBlueprintPiece>): Promise<IBlueprintPieceInstance>

updatePartInstance(
	part: 'current' | 'next',
	props: Partial<IBlueprintMutatablePart>
): Promise<IBlueprintPartInstance>

blockTakeUntil(time: Time | null): Promise<void>

stopPiecesOnLayers(sourceLayerIds: string[], timeOffset?: number): Promise<string[]>
stopPieceInstances(pieceInstanceIds: string[], timeOffset?: number): Promise<string[]>
removePieceInstances(part: 'next', pieceInstanceIds: string[]): Promise<string[]>

The IOnTakeContext would additionally have an extra method that would allow preventing advancing to the next part, while keeping the modifications.

abortTake() => Promise<void>

Making onTake a separate blueprint method instead of overriding "takes" with an adlib action referenced somewhere by its ID makes it potentially less fragile, takes action-specific questions (e.g. What to do with userData and triggerModes for this special action?) out of the equation, and makes it possible to tailor the context interfaces and implementations to each need.

from sofie-core.

I feel like it probably doesn't matter? As you could modify the next part instance with suitable logic in the AdLib if you needed to?
As long as blueprint developers are aware it won't get recalculated automatically then it's probably all good?

from sofie-core.

Yes, that's what I meant to say as well.

from sofie-core.

One thought, is what should happen when Sofie encounters an autonext? I feel like the action should not be used there because playout-gateway is already playing the new part.
But the set next hook would still work here, and would allow for any needed modifications.

Which is why I would strongly opt for going the onSetAsNext route: it maintains both the consistency of the Data model, UI and any other downstream systems (say, Live Status Gateway); and it also makes it reliable for things like autonext, meaning we don't end up with dead ends.

Being able to customise the 'take' action would allow us to perform that copying in blueprints, which will resolve an annoyance we have where clip hoverscrub becomes misaligned. And terminating the held pieces can be done on the following take. The only extra thing I can think we would need would be some way to track the state of the hold, and a way to block the user changing the nexted part during this. Both of those would be simple properties to add and propagate. Of course, the ui will need to be aware of this concept still, which I am ignoring here.

One problem I see with that is the conceptualization of this in the UI - I think before implementation we would need a way of conceptualizing and exposing these - then blueprint-defined - state flags so that then the GUI can retain the functionality of informing the users that they are in fact going into/in a hold.

from sofie-core.

do we have an alternative mechanism for reporting back to the user if the userNotes are currently disabled?

Anything returned from the job-worker handler functions will get propogated back to the caller, inside the ClientResponseSuccess<Result> object.
I don't think we are doing this for any at the moment, so it will probably require a bit of plumbing to then make everywhere that is called from generate notifications depending on the result.

This is a means of modifying the part that's in progress of being taken

That makes sense.
Perhaps what I am missing from the RFC is clarification of what 'current' and 'next' will refer to in the contexts for each method.

from sofie-core.

That definitely feels like a better way to do it as I was wondering about the potential latency impact (although was willing to live with it to get this working).

Does the next part instance get recalculated if you perform an adLib that modifies the current part instance (E.G. hot cut a camera)?
I'm assuming it doesn't as it doesn't feel like it would need to?
So I guess you'd have to make sure you applied any additional logic you wanted in the next part during the adLib (not that that's really an issue).

Either way +1 for @mint-dewit suggestion!

from sofie-core.

I don't believe the next part instance gets recalculated after doing any adlibs, no. If that is something you wanted to do I think it would be fairly simple to only use adlib actions and add a function to call at the end of the adlib action execution.

I guess that may also come with some latency impact though🤔

from sofie-core.

I have nothing against this idea.

I feel like 2 is the simpler of those options to do. I would say that the 'take' action should be required to call context.take() at the end of itself, so that it is able to reject takes if it likes. And any other adlib-action that calls context.take() should be assumed to have done the same processing.
My concern with this approach is whether there are methods in in IActionExecutionContext that make no sense for this take adlib action? If there are then the api could feel like a bit of a mess and justify separating out to a similar but different api.

An alternative that would also allow for a greater deal of customisation would be to add a hook to blueprints that is called after a new part instance has been set as next. The blueprints may then modify this part instance as it sees fit, i.e. just like it can do from an adlib action.

My concern with this is that then any non-action adlib could become unusable, forcing you to do them all via some simple adlib-actions. Such as clearing a layer, which is doable via an adlib-action, but its an extra layer of complexity.
On the other hand, it does mean that the changes being made by the action will be shown in the ui before the part is taken, which would be good.

Im not sure which of those two options I would prefer, and Im not sure if it should be one or the other. Should both of them be options? (My coffee hasnt kicked in yet, I may feel differently later)

from sofie-core.

I'm down for both being options?
I've not had a chance to look at this properly yet other than the early prototype

My concern with this approach is whether there are methods in in IActionExecutionContext that make no sense for this take adlib action? If there are then the api could feel like a bit of a mess and justify separating out to a similar but different api.

I don't think there's anything in IActionExecutionContext that would be an issue but I'm not 100% certain.

Perhaps this needs splitting up into 2 PRs? One to get the quick and easy solution in with a specified 'take' action and then another to do the more complex hook for a set next part process?

from sofie-core.

Perhaps this needs splitting up into 2 PRs? One to get the quick and easy solution in with a specified 'take' action and then another to do the more complex hook for a set next part process?

Yeah, I agree. I think that the 'take' action will be the easiest and give the results needed, with the 'set next' portion being a way to simplify things, or improve performance and also improve the view shown to the users.

One thought, is what should happen when Sofie encounters an autonext? I feel like the action should not be used there because playout-gateway is already playing the new part.
But the set next hook would still work here, and would allow for any needed modifications.

Maybe this will provide the missing pieces for moving hold to be purely blueprints. It has been on the wishlist for a while, but there has not been motivation to try and tackle it.

HOLD works by:

• User activates HOLD when going between two compatible parts.
  • Sofie performs some basic checks to make sure the HOLD is allowed
• User performs take to enter hold
  • Sofie performs the take
  • Sofie will copy some pieces from the previous to the current part
• User performs take to complete the hold
  • Sofie stops the copied pieces

Being able to customise the 'take' action would allow us to perform that copying in blueprints, which will resolve an annoyance we have where clip hoverscrub becomes misaligned. And terminating the held pieces can be done on the following take.
The only extra thing I can think we would need would be some way to track the state of the hold, and a way to block the user changing the nexted part during this. Both of those would be simple properties to add and propagate.
Of course, the ui will need to be aware of this concept still, which I am ignoring here.

from sofie-core.

@ianshade Thank you for the detailed proposal.
We at NRK like the ideas here, if any other Sofie users have any thoughts we are interested to hear them.

We had a couple of questions:

1. Should abortTake() have an optional parameter of a reason and level, to be reported back to the user?
2. Will calling blockTakeUntil() during onTake potentially block the take that is currently in progress?
   If so, this isn't necessarily bad, but does feel a bit odd as it will also act similarly to abortTake(). Perhaps something that should be documented on the method.

from sofie-core.

Thank you for the response, @Julusian.

Should abortTake() have an optional parameter of a reason and level, to be reported back to the user?

That sounds like a good idea! However, I don't think an aborted take should (always) result in throwing an exception, but do we have an alternative mechanism for reporting back to the user if the userNotes are currently disabled?

Will calling blockTakeUntil() during onTake potentially block the take that is currently in progress?

This is a means of modifying the part that's in progress of being taken while the onTake callback is called, so it will be the next take (out of that part) that will be blocked. The comment that I was planning to leave on the method is: Inform core that a take out of the partInstance currently being taken should be blocked until the specified time, but I'd love to hear your suggestions for clearer wording.

from sofie-core.

@Julusian I'm wondering if perhaps we shouldn't introduce a new flow for reporting back the reason for aborting a take specifically. Since there could be a multitude of scenarios in which the user may have to be notified, including when a take (or setAsNext) proceeds normally. What if instead we made delivering the notes to the initiator of the request a standard for takes, setAsNext and adlib actions - all the places where blueprint logic is executed upon user's (or external system's) request? This would mean that if a take is to be aborted, the blueprint developer would have to separately call one of: notifyUserError, notifyUserWarning, notifyUserInfo.

from sofie-core.

@ianshade good point. yeah Im happy to say to use those methods instead.
I don't expect you to figure out how to show all those to the user right now, I expect this is a path full of edge cases and missing links that needs some more thought on how to tackle.

from sofie-core.

Implemented by #1117

from sofie-core.