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

3Delight multipart render support #5844

Closed
wants to merge 17 commits into from

Conversation

gkocov
Copy link
Collaborator

@gkocov gkocov commented May 5, 2024

Generally describe what this PR will do, and why it is needed

  • Support for rendering to multipart EXRs by using the same file name output parameter across multiple outputs.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@gkocov
Copy link
Collaborator Author

gkocov commented May 6, 2024

@johnhaddon Are you OK with switching this PR from main to 1.4_maintenance?

@johnhaddon
Copy link
Member

Are you OK with switching this PR from main to 1.4_maintenance?

Yep, that would be great. I haven't tested what would have happened if we pointed two different outputs to the same file before, but I assume it wouldn't have worked. If you could also include a mention in Changes.md that would be great.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Added a couple of small comments - would be great if you were able to take care of them, but they're not essential.

Comment on lines +135 to +138
i = IECoreImage.ImageReader( os.path.join( self.temporaryDirectory(), "multipart.exr" ) )
h = i.readHeader()
subimages = h['oiio:subimages']
del i
Copy link
Member

Choose a reason for hiding this comment

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

It's not essential that you change this now, but we're close to phasing out IECoreImage.ImageReader everywhere, so if you have time it'd be great if this check could be performed with the OpenImageIO module directly instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it! Absolutely no problem to switch the test to the OpenImageIO module.

@@ -281,7 +281,7 @@ class DelightOutput : public IECore::RefCounted
driverParams.add( { "drivername", &typePtr, NSITypeString, 0, 1, 0 } );
driverParams.add( { "imagefilename", &namePtr, NSITypeString, 0, 1, 0 } );

m_driverHandle = DelightHandle( context, "outputDriver:" + name, ownership, "outputdriver", driverParams );
m_driverHandle = DelightHandle( context, "outputDriver:" + output->getName(), ownership, "outputdriver", driverParams );
Copy link
Member

Choose a reason for hiding this comment

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

Amazing how simple this is! Might be worth a comment stating that NSICreate will just return an existing node if the handle already exists and that's how we coalesce multi-part. I only learned about that detail very recently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good! Will update the code with the comment.

@gkocov
Copy link
Collaborator Author

gkocov commented May 9, 2024

Are you OK with switching this PR from main to 1.4_maintenance?

Yep, that would be great. I haven't tested what would have happened if we pointed two different outputs to the same file before, but I assume it wouldn't have worked. If you could also include a mention in Changes.md that would be great.

Great! Thanks!

Previously the same file name wouldn't have worked because the two Gaffer outputs would create two different NSI outputdriver nodes.

Absolutely no problem to update the Changes.md file.

@gkocov gkocov changed the base branch from main to 1.4_maintenance May 11, 2024 00:10
@gkocov
Copy link
Collaborator Author

gkocov commented May 11, 2024

I think switching the base branch from main to 1.4_maintenance in GitHub messed the PR up. I'll close this PR and recreate the intended changes in a new PR.

@gkocov gkocov closed this May 11, 2024
@gkocov gkocov mentioned this pull request May 13, 2024
4 tasks
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.

4 participants