Usd camera support test #1034

wengn · 2020-02-01T11:37:53Z

Add functions in USDScene.cpp so that SceneReader node will be able to read usdGeomCamera correctly and convert it to IECoreScene::camera into Gaffer.

Checklist

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

Merge ImageEngine cortex master to my fork

Add functions to UsdScene.cpp so that usdGeomCamera will be read correctly and converted to IECoreScene camera. Tests : Add Unit test for importing UsdGeomCamera

dboogert

Thanks for the PR Naiqi ! These are only suggestions on how to improve the PR and hopefully they're uncontentious to the official cortex developers.

Gafffer & Cortex have reasonably strict formatting rules and these stand out to me :

Use tabs instead of spaces
There is almost always a space after '(' and before ')' unless it's '()'
rebase your branch onto origin/master to avoid merge commits to update your topic branch to upstream/master

GetStereoRoleAttr perhaps this should be copied to the Camera somewheere? A Camera is a BlindDataHolder so perhaps the attribute should be serialized there and written back out if it exists?

dboogert · 2020-02-05T15:36:32Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

+        result->setApertureOffset(Imath::V2f(horizontalOff / 10.0f, verticalOff / 10.0f));
+
+     }
+    else { }


Currently all possible values for projToken are covered. Perhaps we should warn in the else if another camera type is added to USD?

We don't handle it on the way out, so I'd be happy just leaving out the else {} for now.

dboogert · 2020-02-05T15:37:44Z

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

+		self.assertTrue( camera.hasObject() )
+
+		cameraObj = camera.readObject( 0.0 )
+		self.failUnless( isinstance( cameraObj, IECoreScene.Camera ) )


I'd test all of the attributes in we expect to be transferred from the camera. In fact it would be great to round trip a camera in testing, i.e. create a camera in cortex, write to USD and read back in again with as little data loss as possible.

Yeah, definitely need to test all values for read at minimum. A roundtrip test as well would be ideal, but in addition to a read test - to make sure we test reading data in we haven't authored.

themissingcow · 2020-02-06T15:19:05Z

Hey Naiqi, thanks for getting this up! Looking promising. I'll get back to you with a proper review soon - sorry we're just in the middle of a big Gaffer release so have been somewhat busy.

These are only suggestions on how to improve the PR and hopefully they're uncontentious to the official cortex developers.

Thanks @dboogert, you are spot on with your comments there.

wengn · 2020-02-07T07:21:36Z

Hey guys @dboogert Don, Thanks for the comments and suggestions, I will tweak my submission accordingly. @tom, I do have a question for this: I just found out for this camera to be able to work properly inside of Gaffer as a renderable camera, I need to change some of Gaffer's RenderController code as well. I am wondering in this kind of situation, what's the proper procedure to submit PR for Gaffer repo? Thanks guys! Naiqi

…

On Thu, Feb 6, 2020 at 11:19 PM Tom Cowland ***@***.***> wrote: Hey Naiqi, thanks for getting this up! Looking promising. I'll get back to you with a proper review soon - sorry we're just in the middle of a big Gaffer release so have been somewhat busy. These are only suggestions on how to improve the PR and hopefully they're uncontentious to the official cortex developers. Thanks @dboogert <https://github.com/dboogert>, you are spot on with your comments there. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1034?email_source=notifications&email_token=ABS5XWBWCFVWMDIURPWMRQDRBQS6XA5CNFSM4KOSUEV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK7SZ2A#issuecomment-582954216>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABS5XWE2YNH265HRH33JTV3RBQS6XANCNFSM4KOSUEVQ> .

themissingcow · 2020-02-07T09:06:47Z

@tom, I do have a question for this: I just found out for this camera to be able to work properly inside of Gaffer as a renderable camera, I need to change some of Gaffer's RenderController code as well.

Are you able to explain a little more why you need to change RenderController? I wouldn't expect there to be any changes needed there.

I am wondering in this kind of situation, what's the proper procedure to submit PR for Gaffer
repo?

The procedure for gaffer is pretty much the same as for cortex. ie:

Discuss the intended changes on the list first so we can make sure it fits with the Gaffer approach before you spend any time on it.
Fork gaffer and make a topic branch from the appropriate GafferHQ branch with your changes (we have maintenance branches for older versions, depending on the nature of the work).
Submit a PR.

'best
Tom

wengn · 2020-02-07T13:22:50Z

Hi, Tom This is the reason: if I only convert the output object of SceneReader to be IECore::camera into the scene, when you have the following graph for rendering, Arnold can't recognize this object as known camera, therefore it will crash if you click on the render button on "InterativeArnoldRender" node. You can try it out with the cortex change and see. [image: image.png] I found in Gaffer source code, RenderControll.cpp, at the very end of updateObject() function, if I replace m_objectInterface = renderer->object( name, object.get(), attributesInterface( renderer ) ); with the following code bool updateObject( const ObjectPlug *objectPlug, Type type, IECoreScenePreview::Renderer *renderer, const IECore::CompoundObject *globals, const ScenePlug *scene, LightLinks *lightLinks ) { .... else { if(object->typeId() == IECoreScene::Camera::staticTypeId()) { if( const IECoreScene::Camera *camera = runTimeCast<const IECoreScene::Camera>( object.get() ) ) { IECoreScene::CameraPtr cameraCopy = camera->copy(); RendererAlgo::applyCameraGlobals( cameraCopy.get(), globals, scene ); m_objectInterface = renderer->camera( name, cameraCopy.get(), attributesInterface( renderer ) ); } } else m_objectInterface = renderer->object( name, object.get(), attributesInterface( renderer ) ); } return true; } Then in /GafferArnold/IECoreArnoldPreview/Renderer.cpp updateCamera() will not crash on this line(line 3148): Imath::V2i resolution = cortexCamera->renderResolution(); If I didn't have the above change in RenderController.cpp to register this cortex camera to Arnold, when arnold is calling updateCamera(), it can't find valid cortex camera and "cortexCamera" is nullptr and the above renderResolution() call will crash Gaffer. I may not understand the whole updating mechanism in Gaffer well, so this may be a hacky solution. Please let me know if there is any way to make this work nicely. Naiqi

…

On Fri, Feb 7, 2020 at 5:06 PM Tom Cowland ***@***.***> wrote: @tom <https://github.com/tom>, I do have a question for this: I just found out for this camera to be able to work properly inside of Gaffer as a renderable camera, I need to change some of Gaffer's RenderController code as well. Are you able to explain a little more why you need to change RenderController? I wouldn't expect there to be any changes needed there. I am wondering in this kind of situation, what's the proper procedure to submit PR for Gaffer repo? The procedure for gaffer is pretty much the same as for cortex. ie: - Discuss the intended changes on the list first so we can make sure it fits with the Gaffer approach before you spend any time on it. - Fork gaffer and make a topic branch from the appropriate GafferHQ branch with your changes (we have maintenance branches for older versions, depending on the nature of the work). - Submit a PR. 'best Tom — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1034?email_source=notifications&email_token=ABS5XWERUC5YO7Q5KGAQT5DRBUQCRA5CNFSM4KOSUEV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELCG2HI#issuecomment-583298333>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABS5XWBOQ4LJPS7UWQMNAZTRBUQCRANCNFSM4KOSUEVQ> .

johnhaddon · 2020-02-07T15:52:52Z

I suspect the Gaffer crash may be because the camera is not in the cameras set. We should probably look at generating the cameras set automatically in the USD reader as well.

themissingcow · 2020-02-10T12:19:04Z

Hey Naiqi, Thanks for the extra details.

Hi, Tom This is the reason: if I only convert the output object of SceneReader to be IECore::camera into the scene, when you have the following graph for rendering, Arnold can't recognize this object as known camera, therefore it will crash if you click on the render button on "InterativeArnoldRender" node.

Right, cool. As John alluded to, this is actually a 'known issue we've been meaning to sort for a while. You have the same crash with alembic cameras.

It happens as at the beginning of the render Gaffer translates all of the cameras in the __cameras set (which is also used to populate the Viewer UI camera pickers) to the renderer. If your camera isn't in that set, then it doesn't get converted, and the crash you noted happens. What we need to do is this:

Fix the crash - it should just be an informative error. I'll sort that soon.
Ensure the USD reader (and the Alembic one too) populates the __cameras set with all the cameras in the file that's being read. That's a bigger change though, so probably best to tackle that in another PR.

So don't worry about the IECoreArnoldPreview changes for now, we'll take care of those. If you are able to address Don's comments though that would be fantastic.

Many thanks again for all the time you've spent on this!
'best
Tom

For now, you can also use the Set node to add your cameras to the __cameras set, which should stop the crash.

themissingcow

Thanks for putting this together Naiqi, this is going to be a great addition. Don beat me to commenting on the more general coding standards topics. I've added a few other notes in here too.

themissingcow · 2020-02-10T12:24:52Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

+        result->setFocalLength(focalLengthVal / scale);
+        result->setAperture(Imath::V2f(horizontalAper / scale, verticalAper / scale));
+        result->setApertureOffset(Imath::V2f(horizontalOff / scale,  verticalOff / scale));


I don't think we need to factor in scale here. I believe it cancels out as long as they're in the same units. The only time the actual dimensions become meaningful is in relation to setFocusDistance and the resulting DoF blur. @danieldresser-ie may be able to confirm.

themissingcow · 2020-02-10T12:27:57Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

+        // We store focalLength and aperture in arbitary units.  USD uses tenths
+        // of scene units
+        float scale = 10.0f * result->getFocalLengthWorldScale();


I think because of the USD assumption, this can just be:

result->setFocalLengthWorldScale( 0.1f );

I think this is working right now as the default for a newly created IECoreScene::Camera is 0.1 so scale will always end up just being 1.0f.

I noticed that USD allows you to specify the linear encoding units in the file. I had a quick chat with @johnhaddon and we can leave supporting this for another time (as it needs some thought on the right way to do it), so just setting the 'assumed' scale is fine for now, unless it's going to cause issues for you.

themissingcow · 2020-02-10T12:43:39Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

+        result->setApertureOffset(Imath::V2f(horizontalOff / 10.0f, verticalOff / 10.0f));
+
+     }
+    else { }


We don't handle it on the way out, so I'd be happy just leaving out the else {} for now.

themissingcow · 2020-02-10T12:46:17Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -31,7 +31,6 @@
 //  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 //
 //////////////////////////////////////////////////////////////////////////
-


Can we re-instate this line, we try to keep any whitespace changes in their own commits if they're required.

themissingcow · 2020-02-10T12:50:50Z

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

+		self.assertTrue( camera.hasObject() )
+
+		cameraObj = camera.readObject( 0.0 )
+		self.failUnless( isinstance( cameraObj, IECoreScene.Camera ) )


Yeah, definitely need to test all values for read at minimum. A roundtrip test as well would be ideal, but in addition to a read test - to make sure we test reading data in we haven't authored.

johnhaddon · 2020-09-21T15:41:51Z

I've implemented camera reading again in #1082, since progress seems to have stalled here and IECoreUSD has changed a lot in the intervening period. Once #1082 is merged we should be able to close this, and then I'll open another PR to make the __cameras set.

johnhaddon · 2020-11-05T10:35:14Z

Closing - IECoreUSD now reads cameras and the __cameras set.

wengn and others added 2 commits February 1, 2020 12:31

Merge pull request #1 from ImageEngine/master

7cb30d1

Merge ImageEngine cortex master to my fork

IECoreUSD : Add UsdGeomCamera import support

8b2412b

Add functions to UsdScene.cpp so that usdGeomCamera will be read correctly and converted to IECoreScene camera. Tests : Add Unit test for importing UsdGeomCamera

dboogert reviewed Feb 5, 2020

View reviewed changes

themissingcow self-requested a review February 6, 2020 14:52

themissingcow suggested changes Feb 10, 2020

View reviewed changes

themissingcow self-assigned this Feb 10, 2020

andrewkaufman assigned johnhaddon and unassigned themissingcow Sep 3, 2020

johnhaddon closed this Nov 5, 2020

Usd camera support test #1034

Usd camera support test #1034

Uh oh!

Conversation

wengn commented Feb 1, 2020

Checklist

Uh oh!

dboogert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

themissingcow commented Feb 6, 2020

Uh oh!

wengn commented Feb 7, 2020 via email

Uh oh!

themissingcow commented Feb 7, 2020

Uh oh!

wengn commented Feb 7, 2020 via email

Uh oh!

johnhaddon commented Feb 7, 2020

Uh oh!

themissingcow commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

themissingcow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnhaddon commented Sep 21, 2020

Uh oh!

johnhaddon commented Nov 5, 2020

Uh oh!

Uh oh!

themissingcow commented Feb 10, 2020 •

edited

Loading