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

Revisited how transitions are handled by the cmx 3600 adapter. #1337

Closed
wants to merge 0 commits into from

Conversation

gplsteph
Copy link
Contributor

The code handling transitions in the cmx 3600 adapter was hard to follow and was introducing problems. These changes
fix these problems and make the code easier to maintain.

  • Transitions lines are now collected when parsing EDL lines and treated by the ClipHandler.
  • The Clip source and record points are replaced with the transition values when the line is parsed.
  • The transition in_offset is now 0 and the out_offset is the transition duration, matching in the timeline what would be done if the editing was read from another format.
  • The transition is added just before the clip when it is added to a track, and that's it.
  • Adjusted tests for the new way of handling transitions, added some other checks.
  • Added an EDL read/write/read roundtrip to check that the reader and writer are compatible.
  • Removed code which was not needed anymore.

Fixes #1328
Fixes #912
Fixes #977

Now the transitions read from an EDL appear at the right time in the timeline, and the timeline has the right overall length.
EDL transitions

Associated tests.
These changes are covered by the test_cmx_3600_adapter tests. A read/write/read roundtrip test was added to ensure they are compatible with the writer.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 23, 2022

CLA Not Signed

@codecov-commenter
Copy link

Codecov Report

Merging #1337 (beac098) into main (d48163e) will increase coverage by 0.00%.
The diff coverage is 88.46%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1337   +/-   ##
=======================================
  Coverage   86.17%   86.18%           
=======================================
  Files         196      196           
  Lines       19848    19825   -23     
  Branches     2319     2309   -10     
=======================================
- Hits        17105    17087   -18     
+ Misses       2175     2171    -4     
+ Partials      568      567    -1     
Flag Coverage Δ
py-unittests 86.18% <88.46%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...opentimelineio/opentimelineio/adapters/cmx_3600.py 84.77% <70.00%> (-0.79%) ⬇️
tests/test_cmx_3600_adapter.py 99.14% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d48163e...beac098. Read the comment docs.

@jminor
Copy link
Collaborator

jminor commented Jun 23, 2022

The failed jobs look like a temporary network problem. I pressed the "Re-run failed jobs" button.

@jminor jminor requested a review from reinecke June 23, 2022 19:45
@gplsteph
Copy link
Contributor Author

Hello, I signed the CLA, but the PR still says "uncovered". Should I push an empty commit to get the bot ran again and have it updated?

@jminor
Copy link
Collaborator

jminor commented Jun 25, 2022

Hmm.. I would have expected it to update automatically. Can you try resolving the DCO check first? Maybe that will re-run the CLA check also. We only recently enabled EasyCLA, so we're still learning it's quirks.

@gplsteph
Copy link
Contributor Author

Trying to resolve the DCO closed this PR, so I opened up this new one:
#1341
It seems both DCO and CLA are good now.

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