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

add deep id/manifest tools and doc #1595

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

peterhillman
Copy link
Contributor

This PR adds

  • a new document to the concepts describing use of deep images to store object IDs
  • two extra example binaries that are built but not deployed. deepidexample creates an image with IDs, deepidselect isolates selected objects from images with deep ids. These are in fact standalone tools but also are intended as code examples.
  • an extra utility, idmanifest, that prints out any manifests found in the given file(s). This could be extended to perform other operations in the future, such as stripping out manifests, or merging manifests together from multiple files

I wasn't quite sure whether deepidexample and deepidselect are in the correct place here. They aren't quite the same as the other examples, but they are not really useful tools that should be installed everywhere.

@pleprince
Copy link
Contributor

I re-read the specification document and it's great. Thanks @peterhillman.

@aconty
Copy link

aconty commented Dec 12, 2023

Very cool. I wish for a UINT64 so you don't have to split the hash for 64 bits. You could also sort the manifest and assign numerical ids in order, then 32 bits are sufficient. But then you need to consolidate ids when mixing frames. Having hashes as ids is stable, I like it better.

I also wonder about the meaning of Z. Since the element may be moving and accumulate color from different depths, I guess we would handle it with ZBACK or skip Z altogether.

@peterhillman
Copy link
Contributor Author

@aconty thanks for those points. Indeed, for moving objects or volumes you might expect a ZBack channel to specify the range of depth that is covered by a given ID. In some cases it may be desirable to store multiple adjacent samples with a discrete depth and the same ID channels.

The deepidexample uses hashed names, rather than sequential IDs. It would be a relatively small change to the code to support that option as well as the hashed scheme. The minor complication is de-duplicating the IDs. Doing that efficiently in a multi-threaded renderer would be more tricky than in this trivial example.

An internal UINT64 type could simplify things, but that would mean that software using the library must represent IDs with a uint64_t type internally. If you do have uint64_t IDs, you can read and write the 64 bit IDs directly by making two setFrameBuffer() calls, one call to put the first channel into the least significant bytes position of the uint64_t, and another call to put the second channel into the most significant bytes position. (The complication here is the position of those two changes depending on whether the hardware is little endian or big endian). Also, keeping the ids as two separate channels opens up the possibility of optimization by reading only one of the two 32 bit channels when the manifest indicates there are no collisions.

@meshula
Copy link
Contributor

meshula commented Dec 13, 2023

I think, but am not sure, that it this proposal carries information equivalent with cryptomatte? It might be worth spending a paragraph on similarities and differences, and motivations. I have a hunch that the biggest difference is that cryptomatte invents some concepts like the manifest that might exist more explicitly in OpenEXR 3?

@peterhillman
Copy link
Contributor Author

Good point @meshula. @pleprince had written some comparisons to Cryptomatte in his version of the text which I omitted for brevity. I've restored that now, and extended it a bit to make the differences clearer. A mention of prman's deep id scheme may also have been appropriate, though that doesn't address how to track names associated with numeric IDs. @jonahf (if you happen to notice this), is this a reasonably accurate summary of Cryptomatte?

@meshula
Copy link
Contributor

meshula commented Dec 13, 2023

Thanks for adding that in, I'm very excited to see these concepts worked out for OpenEXR!

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

I'm going to vote to approve, this looks great.

@cary-ilm
Copy link
Member

@peterhillman, you mentioned adding a test, and I mentioned moving the example code into separate .cpp files and including them in the build (see #1501 for an example of that), but fine to handle that later in a separate PR. Thanks!

@cary-ilm cary-ilm merged commit 831d967 into AcademySoftwareFoundation:main Dec 18, 2023
30 checks passed
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Feb 13, 2024
* add deep id/manifest tools and docs

Signed-off-by: Peter Hillman <[email protected]>

* fix const-ness in deepidexample

Signed-off-by: Peter Hillman <[email protected]>

* fix link on doc, better variable name

Signed-off-by: Peter Hillman <[email protected]>

* add discussion of cryptomatte and deep compositing

Signed-off-by: Peter Hillman <[email protected]>

* fix names of cryptomatte ID channels

Signed-off-by: Peter Hillman <[email protected]>

---------

Signed-off-by: Peter Hillman <[email protected]>
cary-ilm pushed a commit that referenced this pull request Feb 16, 2024
* add deep id/manifest tools and docs

Signed-off-by: Peter Hillman <[email protected]>

* fix const-ness in deepidexample

Signed-off-by: Peter Hillman <[email protected]>

* fix link on doc, better variable name

Signed-off-by: Peter Hillman <[email protected]>

* add discussion of cryptomatte and deep compositing

Signed-off-by: Peter Hillman <[email protected]>

* fix names of cryptomatte ID channels

Signed-off-by: Peter Hillman <[email protected]>

---------

Signed-off-by: Peter Hillman <[email protected]>
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.

5 participants