Skip to content

Add hold and ghost #781

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

eloisebrosseau
Copy link
Contributor

Add hold and ghost

Summarize your change.

This PR adds hold and ghost in the PaintIPNode based on the implementation in Create and the cherry-pick in shotgun-rv that was never merged. The following properties were added to enabled the feature in RV: startFrame, duration, hold, ghost, ghostBefore and ghostAfter. The Open RV documentation reflects this addition in the Paint node.

Hold and ghost can be enable on both polylines and text boxes. To hold annotations, a duration property was needed to know the number of frames on which the annotation should be held from the specified startFrame. To ghost annotations, a ghostBefore and ghostAfter properties were added to set the number of frames on which the ghosted annotation should be displayed before and after the actual annotation. The opacity to apply on ghosted annotations was also updated to match the web app instead of Create to make the annotations more visible the closer they are to the actual annotation, and the more transparent the further they are. Note that some code that was part of the cherry-pick was to fix some thread safeness issues, without being directly related to the hold and ghost feature, but more generally for the PaintIPNode. This code is also part of this PR to make sure everything is on par with what should have been merged back then, but I can remove it, if it makes this PR too hard to review.

Since this was added as a feature for Live Review, the annotation schema, the annotation hook and the otio reader are also updated in this PR. The hold and ghost properties are received for now as part of the annotation metadata. To access the values in the annotation hook, the context in the otio reader needed to be added the metadata propertyof OTIO effects. In the annotation hook, all the new properties to enable hold and ghost were added. The duration property is calculated so that a held annotation will stay on the frames until the next annotated frame or the end of the media. For ghosted annotations and "normal" annotations, the duration is simply the specified OTIO time range, which is normally 1, but it could be any other values.

To see the complete implementation of hold and ghost in the Live Review plugin, and some screen recordings, please refer to this other PR. Also note that a playback issue is currently present when enabling ghost where the first frame without any annotation will completely freeze until the next annotated frame. However, the ghost feature is working as expected when scrubbing on the timeline. Held annotations and "normal" annotations are not affected by this issue.

Describe the reason for the change.

RV is missing the hold and ghost feature that is needed for Live Review.

Describe what you have tested and on which operating system.

All the test were done on macos between RV and the web app during a Live Review session.

else // Command starts after the current frame (ghostAfter)
{
ghostOpacity = static_cast<float>(duration)
/ static_cast<float>(startFrame - frame)
Copy link
Contributor

@bernie-laberge bernie-laberge May 21, 2025

Choose a reason for hiding this comment

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

There is a possible division by zero error here I think if startFrame==frame.

currentFrameCommands; // visible commands, excluding hold and ghost
PerFramePaintCommands beforeCommands;
PerFramePaintCommands afterCommands;

Copy link
Contributor

@bernie-laberge bernie-laberge May 21, 2025

Choose a reason for hiding this comment

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

Perhaps it would be helpful to the reader to add the original comment here ? :

    // In this phase, we loop over all commands and we separate them in 3
     // containers.
     // 1. Commands that are visible on the current frame (based only on their
     //    visibility range settings)
     // 2. Commands that end *before* the current frame.
     // 3. Commands that start *after* the current frame.
     //

Copy link
Contributor

@bernie-laberge bernie-laberge left a comment

Choose a reason for hiding this comment

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

LGTM
Excellent work @eloisebrosseau and thank you for your constant effort of modernizing the code along the way !

Signed-off-by: Éloïse Brosseau <[email protected]>
Signed-off-by: Éloïse Brosseau <[email protected]>
Signed-off-by: Éloïse Brosseau <[email protected]>
Signed-off-by: Éloïse Brosseau <[email protected]>
Signed-off-by: Éloïse Brosseau <[email protected]>
Signed-off-by: Éloïse Brosseau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants