-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn on redundant property values #184
Comments
(I don't know the technicalities of the specific properties that you mentioned, so this is rather about the question of flagging schema-default values in general ) The animation pointer was also the first thing that came to my mind. What could be the medium-term path here? A property should probably not be flagged for having its default value when the property is animated. So there are roughly two options:
The first one would make many assets "invalid" (at least, contain warnings) for a short period of time, and then they turn "valid" again. That doesn't sound right. The second one could be doable. I assume that if/when the animation pointer extension is ratified, the validator will sooner or later support it anyhow, to check the validity of the (Do you have a rough idea about how how many of the sample models contain such redundant properties?) More generally: I see the point of this question, of course. Imagine an asset with 10000 nodes, where each node contains the identity matrix as its transform, but is redundantly written out as a full-blown 16-element-array. That would be far from optimal. But on the other hand, consider some pseudocode of an asset generator:
So far, so simple. But implementing asset generators in a way that they are aware of the default values can be really, really non-trivial. In order to avoid potential warnings in the above example, it would be necessary to build a tremendously complex infrastructure that does things like
for each and every property (not to mention cases like "the default value is X if condition Y holds"). So requiring to not write out default values may be a considerable burden for asset generators. And if the goal was to create an asset generator that is "guaranteed" do generate warning-free assets, this burden might be prohibitively large. |
|
The upcoming iridescence extension gives couple more domain-specific examples:
I do think that these two unique cases should produce dedicated validation issues. As the next step, we may start flagging all factor / texture pairs that yield no-ops. |
I would vote not to flag all redundant values as warnings (like But the cases @lexaknyazev describes here seem OK to me as warnings. 👍🏻 Is the reason the animation pointer extension would affect these warnings just that you may intend to keep a "disabled" extension or texture for later animation? Or something else? |
The philosophy in the design of all these PBR-related extensions is that the extension itself is always useless. The extension, and indeed the entire JSON shell of the glTF, is read once during glTF parsing / loading, and is thrown out prior to runtime rendering. An application that supports clearcoat for example, will have a clearcoat internal API parameter on every single material, not just materials that happen to carry a particular glTF extension. Blender is a good example of this: There's a clearcoat input on every PBR material that imports from glTF, not just materials that use clearcoat. After import from glTF, the user is free to edit the clearcoat input, turning the effect on or off independently from the presence of the glTF JSON or its extensions. The Blender glTF exporter will decide anew at export time whether it's appropriate to write a clearcoat extension for each material. If and when some future version of glTF or any other 3D format decides to incorporate clearcoat as a core parameter, it will be completely compatible with this behavior: The parameter will always be implicitly present, on every material, with a default value that leaves its effects turned off by default, ready to be activated by a user (or perhaps an animation) at any moment. And when exporting such a thing to glTF 2.0, the clearcoat extension is just a shell that needs to be written whenever a particular material's settings don't match the default clearcoat settings at the time of export. Getting back to validation behavior, I would suggest:
|
Some properties of the material extensions can make the whole extension useless when set to zero. For example:
KHR_materials_clearcoat
clearcoatFactor
disables the extension.clearcoatRoughnessFactor
disables the associated texture.KHR_materials_sheen
sheenColorFactor
disables the extension.sheenRoughnessFactor
disables the associated texture.KHR_materials_transmission
transmissionFactor
disables the extension.KHR_materials_volume
thicknessFactor
disables the extension.Note that the same applies to the base spec's
emissiveFactor
that has to have a non-default value for the emissive texture to have any effect.On top of that, there's no need to include, e.g., an IOR extension with a value of
1.5
as it wouldn't change the material appearance but may reduce asset portability. We could go even further and flag all glTF properties that have schema-default values.This approach may get slightly outdated with the upcoming animation pointer extension.
@emackey @javagl @bghgary WDYT?
The text was updated successfully, but these errors were encountered: