Skip to content

USD CameraAlgo : Fix reading and writing of cameras without shutter values #1472

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

Merged
merged 2 commits into from
Jul 1, 2025

Conversation

murraystevenson
Copy link
Collaborator

This improves how we deal with the camera shutter when reading and writing cameras to USD. Previously we'd always read/write shutter values even if the shutter wasn't authored, which resulted in baking in either Cortex's default shutter value when writing a camera with no shutter to USD, or USD's default shutter value when reading a camera with no shutter from USD. This would make it hard to USD round-trip cameras without a shutter parameter, as they'd come back in with a baked-in -0.5, 0.5 shutter, and reading cameras authored in other DCCs with no shutter:open and shutter:close attributes would result in a shutter parameter of 0, 0 as noted by @masterkeech in Gaffer #6431.

While this is a fix and I've currently made this PR to RB-10.5, this is a change in behaviour and would be better held back for a major Gaffer release though we've yet to make a call on how to handle Cortex across the upcoming Gaffer 1.5/1.6 transition so I've marked this as draft while we figure that out...

@murraystevenson murraystevenson self-assigned this Jun 27, 2025
@johnhaddon
Copy link
Member

LGTM.

While this is a fix and I've currently made this PR to RB-10.5, this is a change in behaviour and would be better held back for a major Gaffer release though we've yet to make a call on how to handle Cortex across the upcoming Gaffer 1.5/1.6 transition.

Yeah, given where we are in the Gaffer 1.5 lifecycle, I think we'd be better off holding this till Gaffer 1.6. @ivanimanishi, it would be good to have a conversation on this general topic. I think the gist from our point of view is this :

  • Logically we should be making a new Cortex major version. But that seems somewhat daft for one tiny change, so the temptation is high to call this a bugfix, but not upgrade Gaffer 1.5 to include it. The risk being the possibility of later fixing something else we do want in 1.5, and ending up wrangling custom versions in an ad-hoc way. This isn't an isolated example - we're bumping in to similar issues pretty frequently.
  • We're highly motivated to do other work that would more fully justify a new Cortex major version. A prime example is removing the Renderer class, which was superseded by a Gaffer equivalent in 2016. Removing that will make the API much clearer, and also allow the removal of a host of related classes like EditBlock, WorldBlock etc. We've been blocked on this due to legacy usage in IECoreMaya, despite IECoreMaya notionally moving to a new Viewport 2.0 rendering mechanism in 2018.
  • There is a bunch of other much-needed maintenance we'd love to do if we can get the ball rolling.

I think it's fair to say that the initial blocker here is the Image Engine resourcing needed to remove the legacy code from IECoreMaya, and we've been waiting on this for years now. I have a proposal that I hope allows us to get unblocked :

  • We guarantee that Gaffer 1.6 is compatible with Gaffer 1.5-era dependencies. We've already agreed this anyway for VFXPlatform reasons, and are already running Gaffer 1.6 CI with both old and new dependencies.
  • Gaffer 1.6's official (new) dependencies will use a new Cortex 10.6 major version. We'll remove the legacy Renderer class in this version, and do any other large-scale clean-up we can identify and get done in the timescale. The Gaffer development team will take this on, but won't update IECoreMaya (we don't have skin in that game).
  • This gives Image Engine a grace period in which to get IECoreMaya updated, during which they will run with Cortex 10.5 as planned anyway.

The odd part here is that there'd be a period where IECoreMaya has some non-working legacy functionality. But it occurs to me that we could remove IECoreMaya from Cortex for 10.6 anyway, with it becoming an internal IE project from that point. I think it's fair to say that Cortex's open-source life is limited to it's intersection with Gaffer at this point, so this might actually be quite logical in itself?

Would be great to get your thoughts, @ivanimanishi. As I say, we're highly motivated to try to get the ball rolling here...

@ivanimanishi
Copy link
Member

@johnhaddon, I agree with your proposal.

@johnhaddon
Copy link
Member

@johnhaddon, I agree with your proposal.

That's great, thanks @ivanimanishi!

@murraystevenson, let's get this PR targeted to main, and then we can get started!

@johnhaddon
Copy link
Member

I agree with your proposal.

Just to clarify, does this include removing IECoreMaya from the public repo? And if so, do you think the same should apply to IECoreNuke and IECoreHoudini?

This avoids baking in our default [-0.5, 0.5] shutter when writing a camera with no shutter to USD.
This avoids cameras read from USD files ending up with USD's default [0, 0] shutter when the USD camera has no shutter values.
@murraystevenson murraystevenson changed the base branch from RB-10.5 to main June 30, 2025 17:34
@murraystevenson murraystevenson marked this pull request as ready for review June 30, 2025 17:39
@ivanimanishi
Copy link
Member

ivanimanishi commented Jun 30, 2025

I agree with your proposal.

Just to clarify, does this include removing IECoreMaya from the public repo?

I didn't get that from your proposal, but I think it makes sense, particularly if we don't know of anyone else using IECoreMaya elsewhere.

And if so, do you think the same should apply to IECoreNuke and IECoreHoudini?

Yes.

@murraystevenson
Copy link
Collaborator Author

let's get this PR targeted to main...

This has been switched over to a now up-to-date main, should be good to go.

@johnhaddon johnhaddon merged commit f6b876b into ImageEngine:main Jul 1, 2025
5 checks passed
@johnhaddon johnhaddon mentioned this pull request Jul 9, 2025
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.

3 participants