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

StandardOptions : Added depthOfFieldBlur enable toggle #2890

Merged
merged 3 commits into from Nov 9, 2018

Conversation

@danieldresser-ie
Copy link
Contributor

danieldresser-ie commented Nov 6, 2018

A quick attempt to resolve the issue described in the gaffer-dev topic on the new camera workflow : that many Alembic cameras have focal blur parameters, but we may not want to render focal blur by default.

This adds a toggle to StandardOptions, which defaults off, giving you no focal blur, but can be turned on.

This is technically a compatibility breaking change, or I suppose a fix. The default behaviour matches Gaffer-0.51 ( never DOF ), not Gaffer-0.52 ( default DOF ). If you want the same behaviour as 0.52, then turn on the toggle.

Implementation-wise, I guess baking this setting into the camera in applyCameraGlobals before passing it to Cortex matches how we handle everything else.

Copy link
Member

johnhaddon left a comment

A quick attempt to resolve the issue described in the gaffer-dev topic on the new camera workflow : that many Alembic cameras have focal blur parameters, but we may not want to render focal blur by default.

Thanks Daniel - will be good to get this fixed.

This is technically a compatibility breaking change, or I suppose a fix. The default behaviour matches Gaffer-0.51 ( never DOF ), not Gaffer-0.52 ( default DOF ). If you want the same behaviour as 0.52, then turn on the toggle.

I'm inclined to call this a fix, since I don't think the sudden appearance of always-on depth of field in 0.52 was something folks expected or wanted. I'll link to this from the forums thread though, so people can add their opinion either way.

Implementation-wise, I guess baking this setting into the camera in applyCameraGlobals before passing it to Cortex matches how we handle everything else.

Yep - definitely think this makes sense. But I think it would make sense to go further and add a standard "depthOfField" camera parameter which is supported in Cortex and the Camera and CameraTweaks nodes. That could be done in follow-up work though...

Cheers...
John

@@ -72,6 +72,10 @@ StandardOptions::StandardOptions( const std::string &name )
options->addOptionalMember( "render:shutter", new IECore::V2fData( Imath::V2f( -0.25, 0.25 ) ), "shutter", Gaffer::Plug::Default, false );
options->addOptionalMember( "sampleMotion", new IECore::BoolData( true ), "sampleMotion", Gaffer::Plug::Default, false );

// Depth of Field blur

options->addOptionalMember( "render:depthOfFieldBlur", new IECore::BoolData( false ), "depthOfFieldBlur", Gaffer::Plug::Default, false );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Nov 6, 2018

Member

Let's just call this "render:depthOfField" - I don't think the extra verbosity adds anything.

bool depthOfFieldBlur = depthOfFieldBlurData && depthOfFieldBlurData->readable();
if( !depthOfFieldBlur )
{
camera->setFStop( 0.0f );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Nov 6, 2018

Member

This is fine for now, but what do you think to adding an optional depthOfField parameter to the camera's themselves? This would add a little flexibility for mult-camera rendering and be consistent with the approach taken for all the other camera parameters.


"description",
"""
Whether or not f-stop settings from the camera will be passed

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Nov 6, 2018

Member

Let's remove the talk about whether or not the f-stop is passed to the renderer. I think that an implementation detail, and a short-lived one at that (see my other comments).

on the camera you are rendering.
""",

"layout:section", "Depth of Field Blur",

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Nov 6, 2018

Member

Let's just put this in the existing camera section, with all the other camera options.

@noizfactory

This comment has been minimized.

Copy link

noizfactory commented Nov 6, 2018

I like the idea of also putting this on the camera node as well as the camera tweaks node. It will be consistent with what a user would expect to override on a camera node. However, what would be the behaviour when the user adds a override upstream on a camera node to turn this on but downstream a standard option node turns this off (which is the default)?

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Nov 6, 2018

what would be the behaviour when the user adds a override upstream on a camera node to turn this on but downstream a standard option node turns this off (which is the default)?

An override on the camera would always wins - this already applies to other settings like resolution and overscan.

@danieldresser-ie danieldresser-ie force-pushed the danieldresser-ie:dofEnable branch 2 times, most recently from c5e53db to 239ca1f Nov 6, 2018
@danieldresser-ie

This comment has been minimized.

Copy link
Contributor Author

danieldresser-ie commented Nov 6, 2018

I think I've addressed all John's comments aside from the idea of adding it as an optional render override on the camera as well.

I don't much like either option there: I don't like releasing it in this form if we're about to change the functionality again by adding camera overrides, but I also don't really like the idea of having to do a new version of Gaffer-0.52 that depends on an incompatible Cortex version.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Nov 7, 2018

I don't like releasing it in this form if we're about to change the functionality again by adding camera overrides, but I also don't really like the idea of having to do a new version of Gaffer-0.52 that depends on an incompatible Cortex version.

How about we add the camera overrides in this PR, but implement them like so?

#ifdef CORTEX_CAMERA_HASDOF
camera->setDepthOfField( true );
#else
camera->parameters()["depthOfField"] = new BoolData( true );
#endif

Since the setDepthOfField() accessor is only really a convenience, this will produce identical cameras on either code path. And if you don't fancy the #ifdefing, we could just use the second code path with a todo.

@danieldresser-ie

This comment has been minimized.

Copy link
Contributor Author

danieldresser-ie commented Nov 7, 2018

How about we add the camera overrides in this PR, but implement them like so?

Oh, right, and then still bake it into fStop in applyCameraGlobals. I had actually started doing it this way, but then realised that if we're really adding this to Cortex, it would probably make more sense if we left it to the renderer backends to check if "depthOfField" is set on the camera - but now that I think about it, at least that doesn't affect current user facing behaviour.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Nov 7, 2018

I don't see why we'd still bake it into fStop - we can just have the renderers check for the "depthOfField" parameter by name. Basically, code it exactly as it would be once Cortex is updated, except access the parameter manually instead of via the accessor.

@danieldresser-ie danieldresser-ie force-pushed the danieldresser-ie:dofEnable branch from 239ca1f to 2f4dbe4 Nov 8, 2018
@danieldresser-ie

This comment has been minimized.

Copy link
Contributor Author

danieldresser-ie commented Nov 8, 2018

Updated as discussed with John this morning - with user facing camera parameter, but still baking in applyCameraGlobals because of our renderer backends being half in Cortex and annoying to change.

I threw in a final commit where I change the default fStop to 5.6 to match Alembic - if we have a separate toggle to enable DOF, I don't see any good reason to default to an invalid fStop.

Copy link
Member

johnhaddon left a comment

Looks like the tests are failing due to the addition of an undocumented plug. Otherwise looks good to me, with the adoption of the tweak suggested inline...

// First set from render globals
depthOfField = depthOfFieldData->readable();
}
if( camera->parameters().find( "depthOfField" ) != camera->parameters().end() )

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Nov 8, 2018

Member

We should be able to combine the two map lookups and avoid a potential null dereference as follows :

if( const BoolData *d = camera->parametersData()->member<BoolData>( "depthOfField" ) )
{
    depthOfField = d->readable();
}
Now that we have a toggle to disable DOF by default, it doesn't make sense to default to an invalid fStop
@danieldresser-ie danieldresser-ie force-pushed the danieldresser-ie:dofEnable branch from 2f4dbe4 to ee85690 Nov 8, 2018
@danieldresser-ie

This comment has been minimized.

Copy link
Contributor Author

danieldresser-ie commented Nov 8, 2018

Addressed John's last two comments.

@johnhaddon johnhaddon merged commit 8b9e5d7 into GafferHQ:master Nov 9, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.