aperturesize, resolution, viewplane, shuttertime fixes #33
Conversation
ccam->aperturesize changed to radius rather than width ccam->resolution changed to use camera->renderResolution() as this includes support for Gaffer's resolutionMultiplier plug ccam->viewplane.top and .bottom need to be inverted so camera apertureOffsets in Y apply in the correct direction ccam->motion_position logic simplified and updated to work from relative shutter times, as camera->getShutter() returns absolute shutter values behaviour of changes validated against GafferArnold
There was a problem hiding this comment.
Hey @murraystevenson cheers for the PR, this was definitely something that needed to be looked at again with the right test data and GafferArnold as a comparison, cheers! I've put some notes in, if you don't get around to it I'll try what I've written over here and verify with Arnold.
| @@ -36,6 +36,8 @@ | |||
| #include "GafferCycles/IECoreCyclesPreview/ObjectAlgo.h" | |||
| #include "GafferCycles/IECoreCyclesPreview/SocketAlgo.h" | |||
|
|
|||
| #include "Gaffer/Context.h" | |||
There was a problem hiding this comment.
I see that you are needing to get the current frame somehow on line 129, we don't want a dependency on Gaffer in the IECoreCycles code as it might one day appear in Cortex. What we can do here is use modf() to extract the fractional part of the shutter (good find I didn't know it included the frame in it).
| @@ -66,7 +68,7 @@ ccl::Camera *convertCommon( const IECoreScene::Camera *camera, const std::string | |||
| ccam->fov = M_PI_2; | |||
| if( camera->getFStop() > 0.0f ) | |||
| { | |||
| ccam->aperturesize = camera->getFocalLength() * camera->getFocalLengthWorldScale() / camera->getFStop(); | |||
| ccam->aperturesize = 0.5f * camera->getFocalLength() * camera->getFocalLengthWorldScale() / camera->getFStop(); | |||
There was a problem hiding this comment.
I could've sworn the camera code in Cycles was in diameter not radius, maybe it changed after I implemented this and I didn't notice? But, if it's matching to GafferArnold then I guess I was wrong/it changed! :)
There was a problem hiding this comment.
Yeah, seems a bit unintuitive but it matches visually with Arnold and based on the below it seems the same is done on the Blender camera conversion:
| @@ -104,6 +106,11 @@ ccl::Camera *convertCommon( const IECoreScene::Camera *camera, const std::string | |||
| ccam->nearclip = clippingPlanes.x; | |||
| ccam->farclip = clippingPlanes.y; | |||
|
|
|||
| // Invert the viewplane in Y so Gaffer's aperture offsets and overscan are applied in the correct direction | |||
There was a problem hiding this comment.
Can we simplify this to the lines above?
// Invert the viewplane in Y so Gaffer's aperture offsets and overscan are applied in the correct direction
ccam->viewplane.bottom = -frustum.max.y;
ccam->viewplane.top = -frustum.min.y;
And will we need to change the crop window to match?
There was a problem hiding this comment.
I started with something like that, but I've also been playing with getting overscan working and would need to apply the same flip to the overscan values when modifying the viewplane so figured it'd be cleaner to just flip it all in one place at the end. Happy to take the simpler path and revisit it if/when I put up a PR for overscan.
The crop window doesn't need to be flipped as it applies to the image, rather than the camera viewplane.
| const Imath::V2f &shutter = camera->getShutter(); | ||
| if ((shutter.x > 0.0) && (shutter.y > 0.0)) | ||
| ccam->shuttertime = shutter.y - shutter.x; |
There was a problem hiding this comment.
I'm thinking we might need to add an abs here eg. abs( shutter.y - shutter.x ); if we render with a negative frame.
| if ((shutter.x > 0.0) && (shutter.y > 0.0)) | ||
| ccam->shuttertime = shutter.y - shutter.x; | ||
|
|
||
| const Imath::V2f relativeShutter = shutter - Imath::V2f( Gaffer::Context::current()->getFrame() ); |
There was a problem hiding this comment.
const Imath::V2f relativeShutter = Imath::V2f( modf( shutter.x, nullptr ), modf( shutter.y, nullptr ) );
I'll test here and see.
There was a problem hiding this comment.
Alright had a bit more of a think to this the next day, modf won't cut it. I'm testing now by bringing this code into Renderer.cpp and hashing the frame number with the camera hash there.
There was a problem hiding this comment.
Tried out your camerafixes2 branch with the above changes, renders look good to me.
I guess a simpler way of approaching this would be to calculate the relative shutter time with something like const Imath::V2f relativeShutter = shutter - Imath::V2f( round(shutter.x + shuttertime / 2.0f) );
There was a problem hiding this comment.
When you get the chance @murraystevenson I've got a branch here to try out which should match your changes:
https://github.com/boberfly/GafferCycles/tree/camerafixes2
Let me know if that works fine for you. Cheers!
… Cycles eg. the sockets found here: https://github.com/boberfly/cycles/blob/74f4cbdee302eaacbdacf513bc76d7fe057bf220/src/render/camera.cpp#L59 Also added a few of the nice things found here: #33
… Cycles eg. the sockets found here: https://github.com/boberfly/cycles/blob/74f4cbdee302eaacbdacf513bc76d7fe057bf220/src/render/camera.cpp#L59 Also added a few of the nice things found here: boberfly#33
… Cycles eg. the sockets found here: https://github.com/boberfly/cycles/blob/74f4cbdee302eaacbdacf513bc76d7fe057bf220/src/render/camera.cpp#L59 Also added a few of the nice things found here: boberfly#33
Hey Alex,
I took a closer look at the CameraAlgo behaviour and have come up with the following changes:
The behaviour of these changes has been visually compared with GafferArnold and the results look good to me.
M