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

PR: Fix incorrect "Canon BT.2020 Tungsten" matrix. #12

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

KelSolaar
Copy link
Contributor

References #11.

@scoopxyz
Copy link
Collaborator

scoopxyz commented Feb 17, 2018

Thanks for the PR!

Just to be clear to others, the actual diff in this PR is here:

index 0cf0df1..f692ce4 100644
--- a/aces_1.0.3/python/aces_ocio/colorspaces/canon.py
+++ b/aces_1.0.3/python/aces_ocio/colorspaces/canon.py
@@ -225,7 +225,7 @@ def create_c_log(gamut,
             'type': 'matrix',
             'matrix': [0.724488568, 0.115140904, 0.160370529, 0,
                        0.010659276, 0.839605344, 0.149735380, 0,
-                       0.014560161, 0.028562057, 1.014001897, 0,
+                       0.014560161, -0.028562057, 1.014001897, 0,
                        0, 0, 0, 1],
             'direction': 'forward'})

The large diff is due to small changes after re-running the config generation script. For example:

index 400873b..644189a 100644
--- a/aces_1.0.3/baked/lustre/Rec.709 for ACEScc Lustre.3dl
+++ b/aces_1.0.3/baked/lustre/Rec.709 for ACEScc Lustre.3dl
@@ -3192,7 +3192,7 @@ Mesh 6 12
 318 4095 2079
 318 4095 2081
 319 4095 2083
-319 4095 2086
+320 4095 2086
 320 4095 2089
 322 4095 2093
 323 4095 2097

Unfortunately, due to a lack of Unit testing at the moment, we are unable to currently validate the level of impact on imagery. Though I think we can mutually agree that these changes are within the same level of tolerance compared to an end user running the generation scripts themselves.

@KelSolaar
Copy link
Contributor Author

KelSolaar commented Feb 17, 2018

This is quite annoying and will happen every time somebody will re-build the config with different hardware/os/architecture than the person having done the previous build.

@scoopxyz
Copy link
Collaborator

scoopxyz commented Feb 18, 2018

Interesting article of floating-point determinism: https://randomascii.wordpress.com/2013/07/16/floating-point-determinism/

But this honestly highlights that only the source code should be tracked by the repository, as the LUTs are simply a byproduct of the source execution (though they do need to be readily accessible). So instead, the configs source should be maintained and tested through a CI system that then deploys the resultant build artifacts as a separate release (https://docs.travis-ci.com/user/deployment/releases) This would also ensure that the build system is relatively consistent and these useless LUT diffs should be mitigated.

Related to #13

@scoopxyz
Copy link
Collaborator

Correction: the above only applies to the ACES LUTs since the SPI LUTs aren't currently derived from equations.

@scoopxyz scoopxyz merged commit 0bb079c into imageworks:master Feb 19, 2018
@KelSolaar KelSolaar deleted the feature/canon_idts branch January 26, 2020 01:39
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.

2 participants