-
Notifications
You must be signed in to change notification settings - Fork 37
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
OIIO Transcode: Simplify Extract OIIO transcode settings #853
OIIO Transcode: Simplify Extract OIIO transcode settings #853
Conversation
Here's a snippet from a run test on my side.
|
NOTE: If it's about changing labels and adding conditional enum, then please change just labels and add conditional enums without changing attribute names, otherwise we would have to implement conversion of settings (PITA). |
Can I add conditional enum without creating a new settings class ? |
I agree that it might be PITA but this could be implemented with conversion.py settings overrides (example here ayon-core\server\settings\conversion.py). |
Consider making the colorspace section toggleable. For more details, check out the |
I'm not sure what you mean. It's not overriding the colorspace 'metadata' of the image - it's literally transcoding it from the source colorspace to the destination colorspace? Yes, it's hence also marking it as the destination colorspace? (But is there really a case where it shouldn't if it's transcoded?) I may be missing your point. |
Yes, since you can use addinional oiiotool args, you could also use preset for resizing or other reposition methods. |
Ah yes, we have that actually. But still - then the logic that specificying no colorspace value means "do not color convert" is totally sensible and when specifying a colorspace value will transcode it to that and mark that as the resulting file's colorspace. |
If I was a user, I would rather prefer the explicit way, but this is just a suggestion. You can of course keep it implicit and add it into documentation. |
Explicit as in - it's a separate option in the enum? Like:
|
I'd appreciate your help. I don't know indeed how to implement the conversion. |
@iLLiCiTiT I managed to add the settings conversion. 2024-09-20.00-27-47.mp4 |
Co-authored-by: Jakub Trllo <[email protected]>
Co-authored-by: Jakub Trllo <[email protected]>
@iLLiCiTiT @jakubjezek001 I think this PR is ready. |
Co-authored-by: Jakub Trllo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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 with previous version and adjusted UI settings, and I can confirm it works perfectly. The new UI makes way more sense indeed, so thanks for that 💪 .
As mentioned, the only caveat I found is that I had to re-create my Extract OIIO Transcode
settings when I uploaded the new addons.
That's probably out of the scope of this review, but I think this really needs to be communicated properly on roll-out (new major ? non backward compatible flag ?).
Thank you for pointing that out. in this video some tests (sorry no voice) where I copy from older version to the current version and they all have the same result. 2024-10-02.10-37-18.mp4 |
Let's merge this PR and if there any issue, we can fix them later. |
Changelog Description
This PR aims to ease and simplify the settings of
ExtractOIIOTranscode
plugin.List of TODOs
Additional info
This PR was not requested by clients or community.
The motivation came from documentation when this PR started initially as renaming the input fields.
but it evolved from continues discussions and testing to improve the user experience of the settings.
Therefore, we no longer need to explain why the display and view settings are shown if the transcode type is colorspace.
This PR is not backward compatible as it changed the settings hierarchy a little bit.
But, conversion logic is implemented so we can copy old settings to the new settings without an issue.
For reference, I was testing BigRoy's PR Collect required data for local and farm render jobs to have a 'colorspace' value specified #75
Demo
Testing notes: