vrm-c / vrm-specification

vrm specification
232 stars 36 forks source link

Conflicting "matcapFactor" default values in README #451

Closed kayhhh closed 6 months ago

kayhhh commented 10 months ago

In both the English and Japanese README: https://github.com/vrm-c/vrm-specification/tree/master/specification/VRMC_materials_mtoon-1.0#mtoon-defined-properties-4

Table says default value is [0, 0, 0], but further down it says default value is [1, 1, 1]. The spec says it should be [0, 0, 0].

image image

kayhhh commented 10 months ago

actually i can just make a PR

0b5vr commented 10 months ago

Thank you for the point out!

However, I think it's [0, 0, 0] instead. People might want to just assign matcapTexture and not matcapFactor, and it's gonna be a problem if the matcap does not appear in the situation. The matcap texture will be black if it is not specified, so it's fine to leave the default as [1, 1, 1] even if it's unused.

kayhhh commented 10 months ago

I was just going from the json schema, where it says [0, 0, 0] is the default.

image

I think you might be right that [1, 1, 1] would be a more sensible default. I can update the PR to use [1, 1, 1] if you'd like (and also update the json schema to match).

0b5vr commented 10 months ago

Even the schema is different! I see, thank you for the fix.

0b5vr commented 10 months ago

@Santarh just want to confirm that [1, 1, 1] by default is more appropriate, right?

Santarh commented 10 months ago

Looking good to me.

But this issue reveals more complex problem because of default value of matcapTexture is undefined. The problem should be discussed in another issue.

0b5vr commented 10 months ago

@kayhhh Would you change the PR to use [1, 1, 1] for now? We are going to address the spec side in the different PR.

kayhhh commented 10 months ago

ok, updated it