Use implementation name for OSO

[ld-kerley]

Use implementation name when creating the OSO for OSL Network generator.

The OSO really is the genoslnetwork implementation - so it makes sense to use that name as the naming structure has already been validated for the genosl generator.

To access the implementation we need to change the document caching strategy. We cache the implementation elements in all cases - and only indirect to a nodegraph if present during code generation. If we can measure a performance difference here, then we can add additional caching in the shader generator, but i suspect it won't be measurable under normal circumstances.

[ashwinbhat]

This looks good to me. The part I'm a little unclear about is how do we handle type based implementations. Does implmentationMap have that data?

[ld-kerley]

I actually want to tag @sdromero01 who recently posted this issue. I still stand by this PR and think we should name the OSOs after the corresponding implementation elements, but if people need to retain the ability to control the name of the OSO - I think we could retain that in the tool, if necessary.

Let's give it a few more days for them to weigh in before merging.

[sdromero01]

Hi! thanks for tagging me. I admit I'm not familiar with the implementation naming structure. Could you elaborate on how the generated OSL shaders would be named? Did you mean similar to how the functions in the monolithic OSL shaders made by generateshader.py were named? Currently I'm just using the OSL shader network generator to create OSL shaders with the ND prefix since that's the format of the node names of the .usda assets I've been working with. @ld-kerley

[ld-kerley]

As an example here are a few different implementation names

• IM_add_color3_genoslnetwork.oso
• IMP_UsdPreviewSurface_surfaceshadernetwork.oso
• NG_contrast_vector3network.oso
• NG_switch_floatInetwork.oso

Actually looking more closely at this list,i see the logic I'm using to isn't quite right - so I'll fix that up.

But the idea here is to take the <implementation> name from the corresponding OSL source - which are generally defined here, and replace the _genosl token at the end of the name with genoslnetwork.

I do see the appeal of being able to generate OSOs that match the ND_ (node definition) name, so I think maybe it does make sense to retain an option to do this. This is helpful if you're just generating OSOs that could be used in a non-materialX based shader construction system driven by the node definition names, as you might imagine building for USD based data.

The reason I'd like the <implementation> name to be used, is a little more forward looking. There could be multiple OSL Network generators in the future, and if we use the ND_ prefixed name for this one, then any future ones would need to use something different (if they needed their OSOs structured differently). I actually have some experiments already where this is necessary.

I think maybe the right middle ground here support exporting the OSOs as either name.

[ld-kerley]

I updated the tool so it can emit the OSOs named after the <nodedef> element or the <implementation> element. The <implementation> element is the default, but this leaves the availability for people to easily create OSOs that will match the names used in USD.

[jstone-lucasfilm]

This variable name looks pretty unusual, with a mix of camel case and snake case! Is there a motivation for the unique naming convention?

[ld-kerley]

open to alternatives here...

I'm just remapping a string based enum from the command line args - to allow for future expansion of the public interface - to a more efficient boolean.

I thought the name helped to make it clear that which of the current list of two values we're using for the enum.

In the future if we expand the public command line arg options - then we would need to refactor this internally.

The _ character visually helps separate the variable from its corresponding enumeration value

[jstone-lucasfilm]

Since this variable is only referenced once in the code that follows, what would you think of replacing the single variable reference with argOsoNameStrategy == "implementation", which reads clearly on its own?

If a new variable is ultimately needed here, then I'd suggest a camel case name along the lines of osoNameStrategyIsImpl, but given the context, it seems more straightforward just to omit the variable.

[ld-kerley]

I thought we would prefer to store the result - instead of doing the string comparison inside the loop - but if thats your preference I can make that change. Ultimately we just need to be able to support both modes in a forward looking way.

[jstone-lucasfilm]

It's completely up to you, and although this doesn't seem like a big optimization opportunity, it's fine to create the new camel-case variable if you prefer.

[jstone-lucasfilm]

Minor note: Ideally these new comments should start with a capital letter, mirroring the coding style elsewhere in this source file.

[jstone-lucasfilm]

This looks good to me, thanks @ld-kerley!

[kwokcb]

@ld-kerley , I've logged a regression issue as your change breaks functionality.

[ld-kerley]

replied in your issue...