Slight inconsistency in the Input::setValue method

[ix-dcourtois]

Hi,

See: https://github.com/materialx/MaterialX/blob/9935a83a618e27ee320c68374176008acdd3eb72/source/MaterialXCore/Element.h#L978

The way it's implemented, if you have an input of type filename (for instance the file input of tiled images) and call setValue with a string, it will overwrite the type to string. And this in turn will make shader generation fail with a quite obscure message (I think because the input's type no longer match the one of the nodedef, but I didn't check)

I "fixed" this in my codebase by always forwarding the type of the input (something like: input->setValue(the_value, input->getType()) but this feels weird, and I think most of the time setValue is called, it's to change the value, not the type. I think the linked line could (should?) be replaced by something like this:

        if (!type.empty())
            setType(type);

Or maybe something a bit more complicated where you check if the current input's type is "compatible" with T, and only mutate the input's type if it's not compatible or the type was explicitly passed.

[jstone-lucasfilm]

This is a really good observation, thanks @ix-dcourtois, and I think the second fix that you propose is a compelling path forward. We could check in ValueElement::setValue whether the existing type is a "specialization" of the passed type, and in that case no change to the existing type would be made.

As an alternative approach, I'll point out the method ValueElement::setValueString, which modifies the value attribute of an element and makes no change to the type attribute. That approach is more limited than your suggestion, as it performs no conversion of non-string values, but it can be useful when you know that only the value attribute of an element should be modified.

[bernardkwok]

I've always found that the type argument is a bit strange and wondering whether it should just be removed / deprecated as I'm not sure how it makes sense to change the type after an input/output etc has been created (wherein type should be set). BTW addInput() itself is "unsafe" in that it does not really check against the node definition).

[dcourtois]

I also agree, but I didn't want to make any assumption on the usage of this argument. Maybe someone out there have a use for this, but personally I wouldn't mind if it were removed :)