instanceTechnique values Should Be an Object, Not an Array

[tparisi]

I suggest we change the values property of instanceTechnique to be an object. Right now it's an array. The first thing my loader does is create an object, run through the array, adding each value as a property of the object so I can do lookups. The current design violates the general idea of minimizing processing. If we move to an object there is less processing to do and it's more compact.

I would rewrite the following example:

        "instanceTechnique": {
            "technique": "technique1",
            "values": [
                {
                    "parameter": "diffuse",
                    "value": {
                        "image": "image_0",
                        "magFilter": "LINEAR",
                        "minFilter": "LINEAR_MIPMAP_LINEAR",
                        "wrapS": "REPEAT",
                        "wrapT": "REPEAT"
                    }
                },
                {
                    "parameter": "shininesss",
                    "value": 38.4
                }
            ]
        },
        "name": "blinn3"
    }

to be:

        "instanceTechnique": {
            "technique": "technique1",
            "values": {
                     "diffuse": {
                        "image": "image_0",
                        "magFilter": "LINEAR",
                        "minFilter": "LINEAR_MIPMAP_LINEAR",
                        "wrapS": "REPEAT",
                        "wrapT": "REPEAT"
                    },
                    "shininesss" : 38.4
           },
        "name": "blinn3"
    }

[fabrobinet]

It's actually a design principle in glTF to not use an object when the keys won't be referred anywhere.
In this particular case what you want to do is to go through all the values sequentially and set them.
If you were using an object you would have an extra step to retrieve the keys with say Object.keys(values) and then do the same processing.

The second reason for this design and while the feature is important - is not ideal due to naming - is because you want to be able to change the semantic, for instance, to be able to specify another set of TEXCOORD for your lightmap... what we could do, to convey more the fact that both value and semantic can be changed is to rename values to parameters

[tparisi]

Disagree. In my code, I need to look for specific values to implement the common profile stuff. I am only looking for values that Three.js needs in its Lambert, Phong materials etc. So I have to do the extra step. There is no foreach in my implementation.

It seems like you are using your own implementation as a driver for the design here. I think that is short-sighted.

Also, my syntax is more terse. So I like it better :-)

[fabrobinet]

How do you know you need to look for "shininess" ? do you fetch every possible values in common profile that you hardcoded somewhere, or do you use the "details" to know what to look for ?

[fabrobinet]

Also, did you read what I said about semantic ?, I think you ignored this comment.

[tparisi]

I look through "details" for specific properties I know I will need in the Three.js materials. It's OK, this logic could be switched around to a loop plus a switch statement. So maybe we shouldn't focus on implementation details but design. Right now the array causes an extra level of indirection with the "values"/"parameter"/"value" business, which makes the file more bloated. I like my idea better, stylistically.

Regarding the semantics... it seems like we don't want to override semantics in the material instance; if we want a light map we should define a different material type that has a second texture coordinate parameter. I think this issue is exactly why you and I have been arguing over name ("values" vs "overrides"). In my opinion, a material instance should never allow a new semantic to be be declared. It should go in the material declaration itself. In which case, my naming it "value" still makes sense.

Do we have a separate issue for dealing with the semantics? I think that is more important; this issue is resolving to a syntax argument and I still think I am right! ;->

[fabrobinet]

If you look for "details", then I get what you're doing, it's not too bad, but you could do simpler and more efficient.
For this step you are doing potentially unnecessary work as you may have more parameters defined in details than the number of parameter values to be actually updated in instanceTechnique. So in this case, you'll fetch the parameter name in your object (as in your proposal) for nothing. For instance, you can have default values for parameters set in the technique only.

So instead of calling this design "short sighted" you should step back.

The array is here for a reason, it provides the subset parameters that needs to be updated in the most simple way ie. sequentially you don't need to get the list of parameters to be updated from anywhere else beforehand.
You can keep arguing about this, but you are adding an extra step since you get your list from details and you don't need that.

Consequently, details is mostly is here mainly give you the name of the lightning model and be able to regenerate the shaders (not needed in your case), but once you get that you don't it much at runtime.

Then, to be perfectly honest I also prefer object over array from an esthetically standpoint, but it's unfortunately not the only relevant criteria...

In my opinion, a material instance should never allow a new semantic to be be declared. It should go in the material > declaration itself

Regarding "values" vs "overrides" I think you got me in a weak moment :) I should not have changed this.
@RemiArnaud insisted for good reason that we need to be able to define an instance of a given mesh that has a specific semanic say a specific UV.

And updating the material is what we do for this, and this is through instanceTechnique.

[fabrobinet]

what we could do is to have semantics in addition to values (and just keep it then) if it feels better than than having just overrides. I would clarify the current situation.

[tparisi]

It would be much clearer if there were semantics in addition to values. I think that solves the problem, assuming that Remi did indeed provide a compelling reason for needing semantic overrides. Do you believe that in practice, there will be a lot of semantic overrides? If not, then this solution works great because most of the time, there will only be "values," and values are values. You weren't being weak... you just gave in to reason. :-)

I still want to revisit the semantic overrides issue for light maps but we can do that separately. If you and I can agree on semantics in addition to values then we can move on...

[fabrobinet]

I agree that most of the time we need we want to update values, not semantic. So adding semantics here works for me.

[tparisi]

Great. Resolve this and open new issue?

[fabrobinet]

done #110