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

Arnold Default Metadata #3112

Merged
merged 4 commits into from
Apr 16, 2019
Merged

Conversation

danieldresser-ie
Copy link
Contributor

I thought we had a special label to use for exploratory work, but I can't find it now, so I've just tagged this revisionRequired.

This isn't done, but I think it's basically ready for review to make sure the approach is right:

  • Support for "gaffer.userDefault" in ArnoldShaderUI: Appears complete, should be documented somewhere, no tests yet ( only 1 type tested interactively, so the tests will likely catch actual bugs )
  • Support for "gaffer.default" in GafferArnold.ParameterHandler: Only supporting int and float, remainder of types will be handled similarly.
  • Unit tests: I had originally picture everything being tested in ArnoldShaderTest, but after looking into implementation, I see that this will be responsible just for the defaults, and userDefaults will have to be handled by ArnoldShaderUITest. Unfortunately, because the UI metadata dict is built on startup, this test won't be able to use the same trick as ArnoldShaderTest where the ARNOLD_PLUGIN_PATH has a special test directory added to it, and has the results checked. It looks like currently ArnoldShaderUITest is using all real metadata? I feel like I probably have to hack a special mtd into the path while the tests are running, I don't think there is real metadata that will exercise all the different types.

Anyway, does this look about like what you were thinking?

@danieldresser-ie danieldresser-ie added the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Apr 10, 2019
@johnhaddon
Copy link
Member

I thought we had a special label to use for exploratory work, but I can't find it now, so I've just tagged this revisionRequired.

https://github.blog/2019-02-14-introducing-draft-pull-requests/

I feel like I probably have to hack a special mtd into the path while the tests are running, I don't think there is real metadata that will exercise all the different types.

As things stand, I think the best option for that is to have one test which launches a subprocess. I've used that approach in a couple of other places, and while its not particularly elegant, its effects are localised and it does work.

Anyway, does this look about like what you were thinking?

LGTM. It seems a bit of a pity that the "find the default override" has to be done by the caller of setupNumericPlug() though, rather than it all being encapsulated in that function. It seems like it should be possible to switch on parameter type and call AiMetadataGetInt/AiMetadataGetFloat inside the function?

@danieldresser-ie danieldresser-ie removed the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Apr 12, 2019
@danieldresser-ie
Copy link
Contributor Author

Was a bit of fiddling to get the tests set up and get things sorted out in various Arnold versions, but I think this is good to go now. Still need to set up an example .mtd for overriding the standard_surface defaults in Arnold 5.3 ( which is the motivation for this ), but that can be a separate PR.

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.

Thanks Daniel - could you fix up the version conditional problem and then merge?

@@ -198,12 +198,14 @@ Gaffer::Plug *setupColorPlug( const AtNodeEntry *node, const AtParamEntry *param
bool defaultOverridden = false;
if( std::is_same< ValueType, Color4f >::value )
{
#if AI_VERSION_ARCH_NUM >= 5 && AI_VERSION_MAJOR_NUM >= 3
Copy link
Member

Choose a reason for hiding this comment

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

This will break for 6.0.

@johnhaddon johnhaddon merged commit 05f7865 into GafferHQ:master Apr 16, 2019
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