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

Refactor OTIO frame range collection #1060

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

jakubjezek001
Copy link
Member

@jakubjezek001 jakubjezek001 commented Dec 16, 2024

Changelog Description

Additional info

  • Removed unused function import.
  • Added detailed logging for data updates.
  • Streamlined frame range calculations and handling.
  • Introduced a new class for collecting source frame ranges.
  • Improved readability by cleaning up code structure.

Testing notes:

All should work as before

Blocking ynput/ayon-traypublisher#58

- Removed unused function import.
- Added detailed logging for data updates.
- Streamlined frame range calculations and handling.
- Introduced a new class for collecting source frame ranges.
- Improved readability by cleaning up code structure.
@ynbot
Copy link
Contributor

ynbot commented Dec 16, 2024

Task linked: AY-7125 Advanced Editorial publish to AYON

@ynbot ynbot added type: feature Adding something new and exciting to the product size/XS labels Dec 16, 2024
@jakubjezek001 jakubjezek001 self-assigned this Dec 16, 2024
@jakubjezek001 jakubjezek001 marked this pull request as ready for review December 16, 2024 22:42
Copy link
Contributor

@robin-ynput robin-ynput left a comment

Choose a reason for hiding this comment

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

I gave it a try through Hiero and confirmed it still all works perfectly.

My only concern would be implementation-wise, currently it does:

Collect OTIO Frame Ranges [resolve, hiero, flame, traypublisher]:
* frameStart
* frameEnd
* clipIn
* clipOut
* clipInH
* clipOutH

then

Collect OTIO Frame Ranges (with media range) [hiero, flame]:
* frameStart (same value)
* frameEnd (retimed or same value)
* clipIn (same value)
* clipOut (same value)
* clipInH (same value)
* clipOutH (same value)
* sourceStart
* sourceEnd
* sourceStartH
* sourceEndH

I feel like there is quite some code duplication but more importantly to keep a single source of truth, I'd rather prefer to have :

Collect OTIO Frame Ranges [resolve, hiero, flame, traypublisher]:
* frameStart
* frameEnd
* clipIn
* clipOut
* clipInH
* clipOutH

then

Collect Source OTIO Frame Ranges [hiero, flame]:
* sourceStart
* sourceEnd
* sourceStartH
* sourceEndH

and

Collect Retimed OTIO Frame Ranges [hiero, flame]:
* frameEnd (if retimed)

What do you think ?

@iLLiCiTiT
Copy link
Member

I would like to avoid hosts filtering completelly if possible, but that would probably require more changes....

- Added validation function for OTIO clips.
- Improved documentation for each plugin.
- Enhanced logging to provide clearer debug messages.
- Separated logic for collecting timeline, source, and retimed ranges into distinct classes.
- Updated frame calculations to handle retimed clips more effectively.
- Removed type hints for `Any` in function definitions.
- Simplified the `validate_otio_clip` and `process` methods across multiple classes.
- Cleaned up code for better readability without changing functionality.
jakubjezek001 and others added 2 commits January 24, 2025 14:42
Merged multiple plugins into a single one for collecting OTIO frame ranges.
- Unified handling of timeline, source media, and retimed clip ranges.
- Updated class name and docstrings for clarity.
- Simplified process method to streamline range collection logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: feature Adding something new and exciting to the product
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

4 participants