Cryptomatte fixes #5867
Merged
Cryptomatte fixes #5867
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove stray hashing of the current input channel. - Check cryptomatte channels exists before hashing them. - Include cryptomatte alpha channel in hash. This fixes one of the problems noted in GafferHQ#5866.
The second half of the hash function bore little resemblance to the compute, and had at least one bug : the call to the base class was missing. Conforming the two was simplified by restructuring the compute to reduce nesting by using as many early-outs as possible.
johnhaddon
added a commit
to johnhaddon/gaffer
that referenced
this pull request
May 24, 2024
The EXR standard states that it is only the upper case "R", "G", "B" and "A" that have a predefined interpretation, and that is how we treat channels in GafferImage. But in the last week I've received files from two different sources both using lower-case, so this seems worth fixing up on load. In the latter case, the file was from Blender and only the Cryptomatte channels were lowercased. To my surprise, that is actually enshrined in the Cryptomatte specification. I can't find a source for _why_, but based on Psyop/Cryptomatte#35 it seems that _maybe_ it's to disable lossy DWA compression when the DCC won't let you control that per-file or per-part? That seems to call into question the entire idea of using RGBA to represent ID and coverage in the first place, which I believe was originally to play well in apps that only recognised RGBA. In conjunction with GafferHQ#5867, this fixes GafferHQ#5866.
This was referenced May 24, 2024
murraystevenson
approved these changes
May 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes one of the problems I found while debugging #5866. Only the first commit is necessary to fix that, but I believe the second commit fixes hashing inaccuracies that would be even harder to diagnose (particularly the missing base-class call). The second commit does restructure things a little bit - might be easiest to review just by looking at the final code for
hashChannelData()andcomputeChannelData()side by side, rather than using the diff.