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

Add RV as a participant using the new Live Review schema #602

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

Conversation

eloisebrosseau
Copy link
Contributor

Add RV as a participant using the new Live Review schema

Summarize your change.

The read_otio_string function was added to parse the OTIO object containing the timeline sent by the Review App when RV is joining a new session as a participant. Since the timeline contains some annotation fields, the annotation schema was added in order to let OTIO know that it is a known schema during the deserialization step. However, nothing is done with the annotations at this stage.

Describe the reason for the change.

This is the first step in adding back the new OTIO schema as the preferred way of communication between RV and the Review App.

Describe what you have tested and on which operating system.

Live review using the new schema was tested between a local Review App where the feature was enabled with https://creative-collab.dev.shotguncloud.com and RV on MacOS.

@eloisebrosseau eloisebrosseau changed the title Add Rv as a participant using the new Live Review schema Add RV as a participant using the new Live Review schema Oct 10, 2024
@eloisebrosseau eloisebrosseau force-pushed the live-review branch 2 times, most recently from 69eda0b to cc45e67 Compare October 15, 2024 16:02
Copy link
Contributor

@rogernelson rogernelson left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!



@otio.core.register_type
class Annotation(otio.schema.Effect):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not specific to you, we've already gone down this road in the past, but I am mentioning it here because we might want to consider doing this a different way.

The problem with inheriting from Effect instead of just using Effect directly (and relying on the effect_name to know what the effect is) is that it prevents applications that don't have the schema from reading the OTIO. They will error out with an Unknown schema error.

It might be something fixable on the OTIO-side as well, but for now this problem still occurs, and it not necessarily the standard way of dealing with effects.

otio_obj = otio.adapters.read_from_string(otio_string)
timeline = otio_obj["otio"]

if host_prefix is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Simpler way of doing that:

context = {"sg_url": host_prefix} if host_prefix else None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks!

@@ -276,12 +294,12 @@ def _create_track(in_seq, context=None):
return new_seq


def _get_global_transform(tl):
def _get_global_transform(tl) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for adding these.

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
Thank you Éloïse!

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.

3 participants