Fix up mtoh initialization to not create entries for invalid delagates.

[marsupial]

Initialization of the Hydra delegates is a bit awkward and can ultimately lead to invalid entries in the menu. Selecting an -invalid- delegate can wind up crashing depending on what & why they failed.

[kxl-adsk]

@marsupial Thank you for contributing to maya-usd! I believe this is your first contribution. Can you introduce yourself and/or let me know if you signed the CLA?

[lgritz]

@marsupial is covered by the CLA already signed by Sony Pictures Imageworks.

[kxl-adsk]

@lgritz Perfect, thank you.

[pmolodo]

Hi - so, making sure we only create overrides for renderDelegates that "work" totally makes sense - thanks for the PR!

However, the logic here becomes a bit more opaque than I'd like. Why do we need to generalize MtohGetRendererDescriptions by allowing it to take a custom callback? Is there any other situations in which we can foresee using this ability? Conversely, the main ways in which this particular callback filters the returned renderers - by filtering out those that fail to create a plugin or a delegate - seem generally useful. Are there situations in which we wouldn't want to filter on those criteria?

Also, moving the initialization of the renderer descriptions, and registering of the overrides, from plugin.cpp::initializePlugin() to MtohInitializeRenderGlobals() feels awkward, organizationally. There's not any intuitive reason why that should happen there... this there a practical reason? Will attempts to create renderPlugins or renderDelegates fail before this point for some reason?

Ideally, I think the way I'd like to see this tackled would be to:

• Move all filtering logic (ie, drop those that can't create a plugin or delegate) back into MtohGetRendererDescriptions, and have it not take any args again
• Restore the registering of overrides back to initializePlugin()

The main downside I can see to this approach is that we'd be creating plugins/delegates twice - once to verify they can be made, and then once when MtohInitializeRenderGlobals needs to query them. Given that I don't think these things should take too long to create, and that this is initialization code which is run only once, that doesn't really seem like a performance concern. If it really is an issue for some reason, I'd rather cache the rendererSettingDescriptors rather than have everything happen from inside MtohInitializeRenderGlobals.

Do you think this would work?

[marsupial]

My pleasure...and sorry I didn't respond (but as usual Larry was right on all points!)
Anyway, I'll try to delve more into this on Monday, but my initial thoughts:

On the first point I would say, no. Printing some message about failure, sure. But if they failed to load, they should not be accessible in any way, (more details below).

On the second, I'm in no way tied to the logic of this, there was just quite a bit of spaghetti to unwind to make it all work, and this was the easiest/fastest way to stop Maya from crashing while trying to preserve the const-ness of the singletons.

On how the first ties into the second, and "The main downside I can see to this approach is that we'd be creating plugins/delegates twice", there is a big problem here lurking in the USD plugin-factory system, where failure to load a plugin because dlopen failed, actually creates a very bad state for USD, so you really wanna honor first-time failure is the state you have to operate in.

[pmolodo]

On the first point I would say, no. Printing some message about failure, sure. But if they failed to load, they should not be accessible in any way, (more details below).

I agree that if there's a load failure (either in creating the plugin or delegate), that renderer should never be used (or tried) again. Basically, I was thinking that the pseudologic for MtohGetRendererDescriptions would become:

for (pluginDesc : GetPluginDescs())
    if (!canCreatePlugin)  continue;
    if (!canCreateDelegate)  continue;
    append(pluginDesc);

Since MtohGetRendererDescriptions already "caches" it's results (via a static variable), this loop will only be run once, and ones that fail will never be returned again. Nor will they attempt loading again - in this aspect, it's essentially the same as what your changes do.

On how the first ties into the second, and "The main downside I can see to this approach is that we'd be creating plugins/delegates twice", there is a big problem here lurking in the USD plugin-factory system, where failure to load a plugin because dlopen failed, actually creates a very bad state for USD, so you really wanna honor first-time failure is the state you have to operate in.

So, as I mentioned again, plugins that FAIL would never be created or tried again. So first-time failure would be honored. Only ones that SUCCEED would potentially get re-created again. Do you think that would potentially be a problem?

[marsupial]

Updated to remove the lambdas.

There still is an odd patter returning the render-descriptions, which then gets handed off to MtohInitializeRenderGlobals, but with 22.05 there's likely going to have to be more state tracked at the plugin/application level, so keeping it all in one place seems like a win.

[pmolodo]

I like the overall approach you've taken here, of essentially caching both the descriptions and settings inside of a single initialization function - ie, MtohInitializeRenderPlugins. This allows us to not have to worry about creating and plugins or delegates twice, without the need for a callback - and also allows us to do some more cleanup of the renderDelegate, which I noticed you added.. thanks!

However, I think this approach can allow for even more streamlining and simplification:

• We're now effectively caching the settings twice - once as a list, in the second member of the static Storage in MtohInitializeRenderPlugins, and once as a dict, in the _rendererAttributes private global, in renderGlobals.cpp. Let's eliminate the need for the second cache, _rendererAttributes, by having that be created and cached directly by MtohInitializeRenderPlugins, instead of a simple list.
• Then, we just need to add a new function to return a ref to this settings dict - say, MtohGetRendererSettings? - that is similar to your implementation of MtohGetRendererDescriptions - it just returns the second element of MtohInitializeRenderPlugins.
• Once we have both MtohGetRendererDescriptions and MtohGetRendererSettings, then we don't need to publicly expose MtohInitializeRenderPlugins, and can make it private to utils.cpp. The fact that both of these are created + cached at the same time is an implementation detail which outsiders don't need to know about.

This overall approach will eliminate the need to return a pair, the second of which we have to caution users not to "use", and generally simplify the outward interface.

Does this make sense? Looking back at that, it's a bit of a wall of text, so if it's confusing, I can just make a commit containing my proposed changes, and show rather than tell.

[huidong-chen]

Please don't delete the MRenderOverride pointer until it is deregistered.

[pmolodo]

@marsupial - splitting this discussion out into it's own conversation, for clarity:

Also there is one cache, in _rendererAttributes as it is now. The list that is built up during MtohInitializeRenderPlugins is transferred to MtohInitializeRenderGlobals via the std::move().

Well... there's one valid cache, true. But the old list is still there, just unuseable, and will still be returned by MtohInitializeRenderPlugins() if it's called again. The main point, though, is that we can likely both agree that only having one cache (which stays valid), is preferrable.

I can see if this can be avoided by doing what MtohInitializeRenderGlobals does in MtohInitializeRenderPlugins , but have a vague feeling there's trouble involved in moving _rendererAttributes to a different module (hence the lambdas from the first approach).

Hmm... as long as we make the other map accessible (via returning a reference from a new MtohGetRendererSettings function in utils.h, or something similar), I don't think moving it should present problems. Though it's possible I'm missing some implementation detail...

[kxl-adsk]

Great improvements since the first commit in this PR!

I haven't followed very closely all the discussions, what's left to do? (@marsupial @elrond79)

[pmolodo]

Awesome, this looks great!

Only one substantive issue left - you removed the implementation and usage of MtohGetRendererPlugins, but forgot to remove the declaration in the .h!

Otherwise, just noticed one minor style typo (a missing blank line)... and would ideally like to see a comment re: the usage of the unique_ptr to protect against errors from registerOverride, as discussed here. But neither of those are enough to block merging by themselves.

[marsupial]

Updated.
Assuming you'll do a squash on your end, otherwise let me know,

[kxl-adsk]

We don’t squash on merge in case someone cares about commit/s sha and to potentially prevent duplication for downstream branches. This is our general rule, but you are free to do it and force push.

[kxl-adsk]

Actually, I believe there are open PRs dependent on this PR? Then probably you don’t want to squash (or other PRs may need to be redone)

[marsupial]

I think the other two PR's will conflict after this no matter what (but both should be trivial to resolve).
But however is fine by me.

[kxl-adsk]

Indeed, they will conflict.

[pmolodo]

Looks good!