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

Built-In Transform support for Apple Log #1941

Merged

Conversation

JGoldstone
Copy link
Contributor

Address request for Apple Log support as raised in #1916

Copy link
Collaborator

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

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

Looks great, thank you Joseph!

One more piece needs to be added. There is a check that is done in Config.cpp to ensure that a given version of the config format does not use Builtin functions that are for a newer version. Apple Log will be the first builtin added for config format 2.4, so please add a new section here that tests if the new builtin is being used with older formats.

src/OpenColorIO/transforms/builtins/AppleCameras.cpp Outdated Show resolved Hide resolved
src/OpenColorIO/transforms/builtins/AppleCameras.cpp Outdated Show resolved Hide resolved
…teral); bump LastSupportedMinorVersion to 4

Signed-off-by: Joseph Goldstone <[email protected]>
Copy link
Collaborator

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

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

LGTM!

@JGoldstone
Copy link
Contributor Author

Thanks, both of you, for the speedy review. Not having write access myself, could I ask one of you to merge the request and close this out?

@carolalynn
Copy link
Collaborator

Let’s wait for Robert to take a look, please. I’ve let him know it’s up.

@carolalynn carolalynn linked an issue Feb 5, 2024 that may be closed by this pull request
@KelSolaar
Copy link
Contributor

This would be a good PR to add as a developer guide example, i.e., how to add a new transformation.

@JGoldstone
Copy link
Contributor Author

To Thomas' point, yes, including the introspective aspects, i.e. not allowing the new builtins to be used in pre-3.4 configs, and being the first thing in a release to have such a constraint.

@rkmolholm
Copy link

Looks good to me - thank you Joseph!

@doug-walker doug-walker merged commit 184566e into AcademySoftwareFoundation:main Feb 7, 2024
23 checks passed
@JGoldstone JGoldstone deleted the feature/apple_log branch February 8, 2024 01:22
@doug-walker
Copy link
Collaborator

@rkmolholm , after looking at the results, I'm not sure this transform is correct. It may simply be that the footage I have was not correctly captured. Could you point us at some known good test material in Apple Log space?

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.

Add Built-In Transform support for Apple Log
7 participants