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

Reel name updates for the cmx_3600 adapter #392

Merged
merged 17 commits into from Mar 12, 2019

Conversation

Projects
None yet
5 participants
@apetrynet
Copy link
Contributor

apetrynet commented Nov 29, 2018

Hi!
As discussed in #168 here is a suggestion for handling reel names in EDL export. With these changes we no longer default reel names to AX but the best suited name based on clip information.

I added a trunc_reelname=True argument to the write_to_string function of the adapter which toggles whether or not to truncate the reel name to 8 characters. When truncating is enabled the adapter adds a comment line to the EDL indicating the original source name for the reel.
Reel names get stripped of non-alphanumeric characters.

In addition I added storing of incoming reel name in a clips' metadata when reading EDLs.

Please note that one of the unit tests is broken

apetrynet and others added some commits Nov 28, 2018

apetrynet
Begun working on a reel name based on clip name or external media ref…
…erence.

Also added a trunc_reelname argument to `_reel_from_clip()`
Continuation on updated reel name handling
Support windows slashes, fixed faulty logic in stripping down paths. 
Added reel name to metadata of a clip when parsing an EDL. 
Added comment to EDL when truncating reel name.
Began updating tests to match new reel names
Still need to update `test_edl_round_trip_mem2disk2mem`
@@ -721,7 +729,7 @@ def read_from_string(input_str, rate=24, ignore_timecode_mismatch=False):
return result


def write_to_string(input_otio, rate=None, style='avid'):
def write_to_string(input_otio, rate=None, style='avid', trunc_reelname=True):

This comment has been minimized.

@ssteinbach

ssteinbach Dec 11, 2018

Member

Instead of passing in trunc_reelname you could pass in reelname_len (by default 8), which if you pass in None would not truncate at all? Or is it always 8?

This comment has been minimized.

@apetrynet

apetrynet Dec 11, 2018

Author Contributor

Good idea. The standard is 8, but a lot of apps use longer reelnames. This is a better way of supporting that.
Do you have any thoughts on the compare test of the two jsons that I mentioned? I can always create/modify the source json to what I expect and compare that?

This comment has been minimized.

@jminor

jminor Dec 11, 2018

Collaborator

Could you use the optional flavor argument to include the new stuff, keeping the old stuff as-is?
Also, it looks like the tests are failing in Travis due to some linter issues. Once that goes further to the EDL tests, then it will be easier for us to see the diffs you are talking about.

This comment has been minimized.

@apetrynet

apetrynet Dec 11, 2018

Author Contributor

When you say "the new stuff", do you mean the new comment and metadata or do you mean everything? The 8 character reelname length I believe is in the cmx3600 specs and should perhaps be part of that flavor? As for the new comment line that was just an idea to inform users about the truncated reelname as discussed in issue #168.
I'll fix the lint errors asap. Thought I caught all of them. Sorry about that.

This comment has been minimized.

@apetrynet

apetrynet Dec 11, 2018

Author Contributor

@jminor lint fixes are done. It now fails on the compare test mentioned above.
@ssteinbach I'm working on applying your comments, but have one question. When passing None to reelname_len, should I still strip the extension of the source file in the reelname?

This comment has been minimized.

@ssteinbach

ssteinbach Dec 14, 2018

Member

@apetrynet Those diffs in the test just look like you need to update the baselines. You're adding more metadata, correct?

This comment has been minimized.

@apetrynet

apetrynet Dec 14, 2018

Author Contributor

@ssteinbach Yes, I add the reelname to metadata when parsing an EDL.
Should I just add the expected metadata to the clips in test_edl_round_trip_mem2disk2mem?
What are your thoughts on the added comment by the way?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 19, 2018

Codecov Report

Merging #392 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
+ Coverage   87.15%   87.19%   +0.04%     
==========================================
  Files          67       67              
  Lines        6734     6803      +69     
==========================================
+ Hits         5869     5932      +63     
- Misses        865      871       +6
Impacted Files Coverage Δ
opentimelineio/adapters/cmx_3600.py 91.86% <100%> (+0.25%) ⬆️
...neio_contrib/adapters/advanced_authoring_format.py 90.62% <0%> (-1.21%) ⬇️
...elineio_contrib/adapters/tests/test_aaf_adapter.py 98.08% <0%> (+0.16%) ⬆️

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 88089b1...ad81bdc. Read the comment docs.

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Jan 26, 2019

@apetrynet Is this still [WIP]?

@apetrynet

This comment has been minimized.

Copy link
Contributor Author

apetrynet commented Jan 26, 2019

If it looks good to you guys I believe it's good to go. Should I remove the [WIP] from the title?
I'm not quite sure what the procedure is when it comes to removing the tag. Do I determine that my self or is it when the PR is approved for merge?

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Jan 26, 2019

@apetrynet apetrynet changed the title [WIP] Reel name updates for the cmx_3600 adapter Reel name updates for the cmx_3600 adapter Jan 26, 2019

@apetrynet

This comment has been minimized.

Copy link
Contributor Author

apetrynet commented Jan 26, 2019

Done! Please let me know if I need to pull in the latest changes from master into this branch.

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Jan 26, 2019

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Feb 28, 2019

Sorry that it took us a while to circle back to this. We just landed #434, would you mind rebasing this change given the changes from #434? Then we can merge this in. Really sorry about the delay!

@apetrynet

This comment has been minimized.

Copy link
Contributor Author

apetrynet commented Feb 28, 2019

Sure! On it now!

@ssteinbach
Copy link
Member

ssteinbach left a comment

Only question was how to handle 'AX' events. Any thoughts?

if comment_handler.unhandled:
clip.metadata.setdefault("cmx_3600", {})
clip.metadata['cmx_3600'].setdefault("comments", [])
clip.metadata['cmx_3600']['comments'] += (
comment_handler.unhandled
)

# Add reel name to metadata
if clip_handler.reel:

This comment has been minimized.

@ssteinbach

ssteinbach Feb 28, 2019

Member

So this is preserving even the 'AX' cases that @mikekoetter had pruned. Do you think the strategy to ignore the AX ones is better? (see the comment in the deleted block).

This comment has been minimized.

@apetrynet

apetrynet Feb 28, 2019

Author Contributor

Yeah, when I look at it I see no reason for adding AX to the metadata. It should still be there in the event line if no clip is present.
I'll add the test for it.

This comment has been minimized.

@ssteinbach

ssteinbach Feb 28, 2019

Member

If you add the test, can you add back the comment? I thought that was a good explanation.

This comment has been minimized.

@apetrynet

apetrynet Feb 28, 2019

Author Contributor

@ssteinbach I added the pruning of 'AX', but and issue with the test_cdl.test_cdl_round_trip might have reviled some poor design choices from my end. I'm going to dig a bit deeper to come up with a solution, but I'm afraid I need until next week as I need to sleep now and I'm away from my computer this weekend. Sorry about that.

This comment has been minimized.

@apetrynet

apetrynet Feb 28, 2019

Author Contributor

If you add the test, can you add back the comment? I thought that was a good explanation.

Sure, no problem.

This comment has been minimized.

@ssteinbach

ssteinbach Mar 1, 2019

Member

No rush! Definitely get your rest!

apetrynet added some commits Mar 11, 2019

apetrynet
Merge branch 'master' into edl_reelnames
# Conflicts:
#	opentimelineio/adapters/cmx_3600.py
apetrynet

@ssteinbach ssteinbach added this to the Public Beta 10 milestone Mar 11, 2019

@jminor

jminor approved these changes Mar 11, 2019

@ssteinbach ssteinbach requested review from mikemahony and reinecke Mar 11, 2019

@ssteinbach ssteinbach merged commit 9ffd252 into PixarAnimationStudios:master Mar 12, 2019

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 12, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.