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

None timeline load #86

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

Conversation

ThomasWilshaw
Copy link
Contributor

@ThomasWilshaw ThomasWilshaw commented Nov 13, 2024

I made a start at #23 and I've managed to (I think) sensibly handle none Timeline schemas. I started looking at suppporting just clips and had a couple of questions:

  1. Is this the right sort of approach?
  2. For some reason app.cpp:912 was causing the selected_object state to be cleared leading to crashes. Commenting it out fixes the issue but I can't figure out why it's happening in the first place.

This is very much still a WIP but I wanted to check it was heading in the right direction before I go any further.

image

Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
@jminor
Copy link
Member

jminor commented Nov 14, 2024

Yes, this strategy seems like a good start.

You could go one step further and keep root around for later use replacing the top level appState.timeline pointer.

Once loaded, then the call to DrawTimeline inside MainGui could be wrapped in a DrawRoot function that switches on the root object's type to decide how to render it.

@jminor
Copy link
Member

jminor commented Nov 14, 2024

Side note for future reference: after the work in this PR is complete, we can implement #72 by making a double-click on a nested composable switch the root to that object, allowing you to drill into the sub-composition. Popping back out of the nested composition would restore root to the previous object.

@ThomasWilshaw
Copy link
Contributor Author

I'm having a little trouble with this. I've replaced appState.timeline with otio::SerializableObject::Retainer<otio::SerializableObjectWithMetadata> root in app.h and that is generally working fine. However if I try to cast that (dynamic_cast<otio::Timeline*>(appState.root)) I get the following error:

the operand of a pointer dynamic_cast must be a pointer to a complete class type

I think this is probably because I don't fully inderstand how Retainers work but I wondered if you had any suggestions?

We now store the root OTIO object that is opened (whatever type it is)
and we handle it based on its schema type.

Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
@ThomasWilshaw
Copy link
Contributor Author

I've updated things to store the root OTIO object rather than assuming a timeline and it more or less seems to work. I'm still having an issue where object->to_json_string(&error_status) in SelectObject() is corrupting the root object somehow.

@jminor
Copy link
Member

jminor commented Dec 8, 2024

Hmm... I tried it out, and I attempted to diagnose the to_json_string issue - it feels like the returned string is getting deallocated. I can print it within the function, but it gets lost later.

I'm suspicious of the switch from
otio::SerializableObject::Retainer<otio::Timeline> timeline to
otio::SerializableObjectWithMetadata* root
since maybe the root object is getting deallocated. I was able to use otio::SerializableObject::Retainer< otio::SerializableObjectWithMetadata> root by adding some take_value() calls around, but it all falls apart on load.

I'll have to return to this when I have my C++ brain turned back on. I've been writing Swift and Python lately, so the rules of engagement are fuzzy in my head right now. 😳 The needed info is likely in here: https://opentimelineio.readthedocs.io/en/stable/cxx/cxx.html#memory-management

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