AcademySoftwareFoundation / OpenTimelineIO

Open Source API and interchange format for editorial timeline information.
http://opentimeline.io
Apache License 2.0
1.4k stars 276 forks source link

Fix for find_children() with stacks #2 #1766

Open darbyjohnston opened 2 weeks ago

darbyjohnston commented 2 weeks ago

Fixes #1652

This PR is a replacement for #1755 which has some trouble with the git history.

After some investigation, it looks like the find_children() function was not working with stacks when given a search range. To fix this, an override is added for the function children_in_range() that works with "stacks" of children.

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.69%. Comparing base (c0e97b0) to head (3a61e3c). Report is 12 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766/graphs/tree.svg?width=650&height=150&src=pr&token=mfnfUB4h7u&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation)](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) ```diff @@ Coverage Diff @@ ## main #1766 +/- ## ========================================== - Coverage 84.11% 81.69% -2.43% ========================================== Files 198 176 -22 Lines 22241 12342 -9899 Branches 4687 3027 -1660 ========================================== - Hits 18709 10083 -8626 + Misses 2610 1723 -887 + Partials 922 536 -386 ``` | [Flag](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) | Coverage Δ | | |---|---|---| | [py-unittests](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) | `81.69% <92.59%> (-2.43%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) | Coverage Δ | | |---|---|---| | [src/opentimelineio/composition.h](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?src=pr&el=tree&filepath=src%2Fopentimelineio%2Fcomposition.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation#diff-c3JjL29wZW50aW1lbGluZWlvL2NvbXBvc2l0aW9uLmg=) | `65.51% <ø> (ø)` | | | [tests/test\_track.py](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?src=pr&el=tree&filepath=tests%2Ftest_track.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation#diff-dGVzdHMvdGVzdF90cmFjay5weQ==) | `96.77% <100.00%> (+1.42%)` | :arrow_up: | | [src/opentimelineio/stack.cpp](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?src=pr&el=tree&filepath=src%2Fopentimelineio%2Fstack.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation#diff-c3JjL29wZW50aW1lbGluZWlvL3N0YWNrLmNwcA==) | `63.38% <75.00%> (-6.47%)` | :arrow_down: | ... and [66 files with indirect coverage changes](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation). Last update [e10fdd2...3a61e3c](https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/pull/1766?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AcademySoftwareFoundation).
darbyjohnston commented 2 weeks ago

@jminor Sorry, I managed to mess up the git history on the previous PR while trying to update with the latest changes from main. I created this new PR as a replacement.

I believe your last comment was about using trimmed_range_in_parent() and testing with a source range on the track? I didn't realize that tracks could have source ranges, what would that test look like?

jminor commented 2 weeks ago

There's an example of a track with a source_range in Figure 3 here: https://opentimelineio.readthedocs.io/en/stable/tutorials/otio-timeline-structure.html In that diagram, Track-003 is offset by using a source_range with a negative start time. A simpler use case would be to use a track's source range to just trim a nested track to a sub-range of the whole track.

There's also an example of a stack with a source_range in Figure 4 on that same page. In that case the source_range is used to trim the nested stack. Although rare, the source_range could be set on the top level stack of a timeline also.

darbyjohnston commented 1 week ago

Thanks, those figures were very helpful; I guess I should have checked there first. :)

I changed the code to use trimmed_range_in_parent() and added extra C++ tests that put various source ranges on the track. If the C++ tests look OK I will update the Python tests to match.