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

Metadata Lookup Issue #3333

Merged
merged 2 commits into from Aug 20, 2019
Merged

Conversation

mattigruener
Copy link
Contributor

Hey John,

this took me a while to track down and it might not be the best fix - could you take a look?

Background:
Arnold changed some default values for shader parameters in a recent update and we've counteracted that on some shows by putting the old values in an mtd file. That broke metadata lookups of data that was registered through the

Gaffer.Metadata.registerValue( "ai:surface:standard_surface:<parameterName>", "userDefault", "defaultValue" )

mechanism (that's a feature of the shader base class). My intuition is that registering those old defaults via mtd on other plugs shouldn't break the lookup for existing entries that are somewhat unrelated and only share the 'userDefault' key with the new entry. Does that make sense?

I've added a test that illustrates the problem. Maybe the fix should be that the method registered to lookup the plug values passes on that lookup to the base class to figure out if there were any values registered through the syntax mentioned above? Is that function now solely responsible for the lookup or should the logic live in Metadata.cpp (that's what I've done in this PR)?

Thanks :)

Matti.

@danieldresser-ie
Copy link
Contributor

danieldresser-ie commented Aug 10, 2019

I don't really have any insight into this other than being the person who accidentally triggered the problem, but this solution looks reasonable to me.

@johnhaddon
Copy link
Member

My concern with this is that we might be relying on the existing behaviour for return None to effectively "delete" a base class registration on a derived class. I've had a quick look for an example and couldn't find one, but I've got a nagging feeling there might be one out there.

But I'm confused : if we have a base class mechanism, why did we even add the derived class .mtd version in the first place? If it was standard .mtd metadata that was used by other DCCs, I could understand us adding support for it, but it's not, it's totally Gaffer-specific. So can't we just use the original mechanism the base class provides instead? Do you recall @danieldresser-ie? Did we simply forget about the original mechanism?

If there's not a good reason for having the duplicate mechanism, my proposal would be this :

  1. Leave Metadata.cpp alone.
  2. Update ArnoldShaderUI.py to convert the .mtd user defaults into calls to registerValue() for the base class mechanism to be picked up. That fixes the problem for now, and can be backported.
  3. Rip out 2) entirely for the next major version.

@danieldresser-ie
Copy link
Contributor

It's true that userDefault is not a standard Arnold mtd option, but in the scenario where you copy an mtd file from another source, but are maintaining it for Gaffer, it seems pretty convenient to set userDefault in the same place you're setting min/max/description?

In any case, is this issue actually specific to userDefault, or could the same thing go wrong with mixing anything else read by a dynamic function in ArnoldShaderUI with overriding the same thing using the standard Gaffer mechanism ( ie. the standard Arnold metadata "desc" or "max" ) ?

The proposed step 2) sounds kind of reasonable, but would be doing this just for userDesc, or for absolutely everything in ArnoldShaderUI?

If that works, I would be in favour of sticking with that, rather than ripping it out for the next major version.

@johnhaddon
Copy link
Member

in the scenario where you copy an mtd file from another source, but are maintaining it for Gaffer, it seems pretty convenient to set userDefault in the same place you're setting min/max/description?

Yeah, I guess contrib/arnold/arnoldBackwards52Defaults.mtd takes advantage of that by setting gaffer.default and gaffer.userDefault in the same place. OK, let's scrap 3.

In any case, is this issue actually specific to userDefault, or could the same thing go wrong with mixing anything else read by a dynamic function in ArnoldShaderUI with overriding the same thing using the standard Gaffer mechanism

In theory I think it applies to anything, but in practice I think it's just user defaults, because that's the only thing also provided by the base class.

The proposed step 2) sounds kind of reasonable, but would be doing this just for userDesc, or for absolutely everything in ArnoldShaderUI?

For now I'm proposing it just for userDefault, as an expedient fix for just the problem we have in practice. Longer term I think the most beneficial thing might be a mechanism to allow a dynamic metadata function to explicitly fall back to the base class value. But I don't want to get into that now - I just want to get this bug fixed without potential knock-on effects.

@mattigruener mattigruener force-pushed the metadataIssue branch 2 times, most recently from b9aa1da to 9af5bf6 Compare August 14, 2019 22:00
@mattigruener
Copy link
Contributor Author

Updated. Could you take a look, John?

for key in nodeKeys :
Gaffer.Metadata.registerValue( nodeType, key, functools.partial( __nodeMetadata, name = key ) )

for key in parametersPlugKeys :
Gaffer.Metadata.registerValue( nodeType, "parameters", key, functools.partial( __plugMetadata, name = key ) )

for key in parameterPlugKeys :

if key == 'userDefault' :
Copy link
Member

Choose a reason for hiding this comment

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

Could this, and all the other new conditionals above, be an indication that it'd be better to not put "userDefault" into __metadata in the first place? Perhaps it makes more sense to call Gaffer.Metadata.registerValue directly at line 286?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Do you like this better?

I guess the problem with this version is that it'll register the userDefaults on lights in the same way and lights don't support that syntax we're using here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we hardcode the "surface" bit in the metadata - so it wouldn't work on lights anyway..?

Copy link
Member

Choose a reason for hiding this comment

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

Updated. Do you like this better?

Thanks - yes, this is much better. Everything dealing with user defaults is in one place, and we're not putting something in a data structure that then needs to be ignored by the main consumer of that structure.

I guess the problem with this version is that it'll register the userDefaults on lights in the same way and lights don't support that syntax we're using here?

Seems pretty reasonable that we'd want to support it for lights too? Any reason we couldn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems pretty reasonable that we'd want to support it for lights too? Any reason we couldn't?

No, no reason why we couldn't support it. I was just noting that currently we don't, but the code I've added kinda suggests that we do. If you're cool with what I have now I'm happy, too :)

@mattigruener mattigruener force-pushed the metadataIssue branch 2 times, most recently from 780b4ca to 4951ab4 Compare August 14, 2019 22:31
@johnhaddon
Copy link
Member

Gah - looks like we've got some legit test failures now. Let me rephrase my earlier "this is much better" comment! I think we have a much better structure for the change now, but presumably there is something missing in the detail - could you take a look?

@johnhaddon johnhaddon added the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Aug 15, 2019
@mattigruener
Copy link
Contributor Author

Ah, I should've adjusted the test, sorry. Now that we've avoided registering a special function for that metadata, testing that behaviour doesn't make a whole lot of sense - especially as we've circumvented that issue instead of fixing it.

Do you want me to just rip out that test? Maybe instead we should test the behaviour in ShaderUI.py that we rely on even more now?

Matti Gruener added 2 commits August 19, 2019 14:41
When registering metadata for a specific plug, it shouldn't override the
metadata lookup on the base class for another plug.
@mattigruener
Copy link
Contributor Author

I've updated the test so that the ShaderUI mechanism is tested but the other test assert is gone.

@johnhaddon johnhaddon merged commit 3e5b667 into GafferHQ:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-revision PRs that should not be merged because a Reviewer has requested changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants