AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.79k stars 326 forks source link

Getting the value of a node def's input of type vector2array returns a string #673

Open ix-dcourtois opened 2 years ago

ix-dcourtois commented 2 years ago

Hello,

I'm not sure if I'm doing something wrong, but I think there's a small "bug" in the way the stdlib is defined. In stdlib_defs.mtlx, the input values of some node defs (for instance ND_arrayappend_vector2array_vector2array, input in1 are typed as vector2array, but if you try to to get the node def via the API, and extract the value, its type is string:

// assuming node_def is the def for ND_arrayappend_vector2array_vector2array
auto in1 = node_def->getInput("in1");
auto in1_type = int1->getTypeDef()->getName();   // < this returns "vector2array"
bool is_string = in1->getValue()->isA<std::string>(); // < this is true

I'd expect the value to be a vector<float> or something else, but not a string (on a sidenode, in MaterialXCore\Value.h, I see specialization for arrays of int, bool, float and string, but not vector2, etc. so I'm not sure how I should go about to get the value of a vector2array input)

Is this a bug? A missing [] in the value in the .mtlx file? (I noticed that the outputs of type vector2array had the value "[]" while the inputs had "")

Regards, Damien.

ix-dcourtois commented 2 years ago

Hi, anyone had a look at this? Should I "fix" this by just modifying the library and submitting a pull request? Or is it something that would need some other fixes C++ side?

jstone-lucasfilm commented 2 years ago

This is a good catch, @ix-dcourtois, and I can verify that we've only implemented support for a subset of the array types in our C++ codebase: https://github.com/materialx/MaterialX/blob/main/source/MaterialXCore/Value.cpp#L294

We'd be happy to look at a pull request to extend this, but I should note that we're considering deprecating the "arrayappend" node in a future version of MaterialX, based on the following discussion in a recent session of the MaterialX TSC:

https://academysoftwarefdn.slack.com/archives/C0230LWBE2X/p1630436698071900

I'll post a summary of the discussion here, for those without access to the ASWF Slack channel:

dcourtois commented 2 years ago

Hi, sorry for the late reply! Thanks for the answer. I'm going to wait until a decision has been made then, wouldn't make much sense to work on a pull request for a feature that's going to be removed :)