Skip to content
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

Version 2.0.0-dev.3.0 #124

Merged
merged 16 commits into from
Jan 9, 2020
Merged

Version 2.0.0-dev.3.0 #124

merged 16 commits into from
Jan 9, 2020

Conversation

lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Nov 11, 2019

2.0.0-dev.3.0 (November 2019)

New Features

  • Added support for EXT_texture_webp extension.

    • When the extension is not declared in extensionsUsed, image objects with WebP data are reported with a new IMAGE_NON_ENABLED_MIME_TYPE error.
  • Added support for KHR_mesh_quantization draft extension.

  • Added NON_REQUIRED_EXTENSION error for extensions that cannot be optional (such as KHR_mesh_quantization).

  • Added vendor prefixes: MESHOPT, POLUTROPON, and AGT (Add missing vendor extensions prefixes MESHOPT and POLUTROPON #119).

  • Added vendor prefixes: ALCM and SKYLINE.

  • Added vendor prefixes: FOXIT, KDAB, and CAPTURE.

  • Extension names are now validated to have an upper-case prefix separated from the rest on the name with underscore (new INVALID_EXTENSION_NAME_FORMAT warning).

  • Referencing an image of type defined by an extension from the core objects now results in TEXTURE_INVALID_IMAGE_MIME_TYPE error.

  • Added IMAGE_FEATURES_UNSUPPORTED warning for images that contain features (like animation in WebP) not supported by glTF.

  • Validation report now includes color-space information for provided images.

  • Validation report info object now includes more information about asset's performance characteristics (see Add object counts and sizes to report #120).

  • Vertex colors with values outside of [0.0 .. 1.0] range now issue ACCESSOR_NON_CLAMPED error.

  • Skinning validation (fixes Skins data validation #58):

    • When a node with a skinned mesh is used in a scene, the skeleton's common root must be available in the same scene (NODE_SKIN_NO_SCENE error).

    • The skeleton nodes of a skin must have a common root (SKIN_NO_COMMON_ROOT error).

    • The skin.skeleton property, when present, must point to a common root (SKIN_SKELETON_INVALID error).

    • Animation channels should not target a node with a skinned mesh (ANIMATION_CHANNEL_TARGET_NODE_SKIN warning).

    • A node with a skinned mesh should be a root node (NODE_SKINNED_MESH_NON_ROOT warning).

    • A node with a skinned mesh should not have local transforms (NODE_SKINNED_MESH_LOCAL_TRANSFORMS warning).

    • Vertex influences validation:

      • All joints values must be within the range of joints in the skin (ACCESSOR_JOINTS_INDEX_OOB error).

      • No joint may have more than one non-zero weight for a given vertex (ACCESSOR_JOINTS_INDEX_DUPLICATE error).

      • Weights must be non-negative (ACCESSOR_WEIGHTS_NEGATIVE error).

      • Weights for each vertex must be normalized to have a linear sum of 1.0 (ACCESSOR_WEIGHTS_NON_NORMALIZED error).

      • Unused joint values (i.e. joints with a weight of zero) should be set to zero (ACCESSOR_JOINTS_USED_ZERO_WEIGHT warning).

Integration updates

  • Upgraded to the latest stable SDK.

  • Generated npm package no longer requires polyfills when using webpack with certain configurations (fixes NPM Package not working in Angular7 App #110).

  • It is now possible to omit timestamps from validation reports.

  • Validation report now consistently uses lower-case enums.

  • Native executable binaries can now be compiled. See the README for details (fixes Windows EXE command line validation tool #113).

  • Generated report filename is now <asset>.report.json.

  • Unit tests (300+) are now consistently stored: an asset and its validation report. Combined with provided JSON catalogs, they could be used for testing other glTF implementations.

Changes

  • Major refactoring of binary data validation. Now, mesh and animation accessor issues are attributed to the corresponding binding points rather than to accessor objects. This change makes validation reports more precise when accessors are reused. Namely:

    • ACCESSOR_ANIMATION_INPUT_NEGATIVE and ACCESSOR_ANIMATION_INPUT_NON_INCREASING are attributed to animation.sampler.input.

    • ANIMATION_SAMPLER_OUTPUT_ACCESSOR_NON_NORMALIZED_QUATERNION (new) is attributed to animation.channel.sampler.

    • ACCESSOR_INVALID_SIGN is attributed to mesh.primitive.attributes.TANGENT; its message is more sound now.

    • ACCESSOR_VECTOR3_NON_UNIT (renamed from ACCESSOR_NON_UNIT) is attributed to mesh.primitive.attributes.NORMAL or mesh.primitive.attributes.TANGENT.

    • ACCESSOR_NON_CLAMPED is attributed to mesh.primitive.attributes.COLOR.

    • ACCESSOR_INVALID_IBM is attributed to skin.inverseBindMatrices.

    • ACCESSOR_INDEX_OOB, ACCESSOR_INDEX_PRIMITIVE_RESTART, and ACCESSOR_INDEX_TRIANGLE_DEGENERATE are attributed to mesh.primitive.indices.

  • ACCESSOR_INVALID_FLOAT message is more specific.

  • Fixed incorrect message in ACCESSOR_INDEX_OOB.

  • BUFFER_VIEW_TOO_LONG is now attributed to bufferView.byteLength when bufferView.byteOffset fits the referenced buffer.

  • FILE_NOT_FOUND is renamed to IO_ERROR; its error messages are now more consistent across platforms.

  • Web drag-n-drop validator now issues an error when external resources are not available among dropped files.

  • ACCESSOR_INDECOMPOSABLE_MATRIX renamed to ACCESSOR_INVALID_IBM; IBM data is no longer checked for TRS decomposition.

  • Unused texture coordinates are now consistently reported as UNUSED_OBJECT with proper pointers regardless of material presence; MESH_PRIMITIVE_UNUSED_TEXCOORD is removed (fixes No message for unused texcoords if there is no material #117).

  • MESH_PRIMITIVE_JOINTS_WEIGHTS_MISMATCH now reports mismatching numbers.

  • Removed BUFFER_NON_FIRST_GLB warning (fixes Should all buffers with empty uri be treated as GLB buffers? #121).

  • Removed ANIMATION_SAMPLER_OUTPUT_INTERPOLATION.

  • Removed WEB3D_quantized_attributes and CESIUM_RTC extensions support.

Bugfixes

  • Fixed possible crash on empty input stream.

  • Fixed possible crash on unresolved animation sampler inputs.

  • BUFFER_VIEW_TOO_LONG is now correctly attributed to the invalid property.

  • When buffer.byteLength is undefined for an embedded buffer, BUFFER_EMBEDDED_BYTELENGTH_MISMATCH is no longer reported.

  • When asset.version is undefined or incorrect, ASSET_MIN_VERSION_GREATER_THAN_VERSION, UNKNOWN_ASSET_MAJOR_VERSION, and UNKNOWN_ASSET_MINOR_VERSION are no longer reported.

  • When reported, UNKNOWN_ASSET_MAJOR_VERSION and UNKNOWN_ASSET_MINOR_VERSION errors now point to asset.version.

  • MESH_PRIMITIVE_TANGENT_WITHOUT_NORMAL and MESH_PRIMITIVE_TANGENT_POINTS now point to tangent attribute instead of primitive.attributes object.

  • Numerical thresholds for unit-length vectors are now chosen to accommodate possibly-quantized values.

  • Fixed missing UNRESOLVED_REFERENCE errors for KHR_lights_punctual extension.

  • When the root KHR_lights_punctual (with lights array) extension object is not present, its node counterpart now issues UNSATISFIED_DEPENDENCY error.

  • Reported issues about invalid JSON syntax or invalid root object type no longer have JSON pointers.

  • Fixed incorrect message in BUFFER_VIEW_TOO_BIG_BYTE_STRIDE.

  • Fixed possible crash on invalid buffer or image objects (fixes Validation failed: NoSuchMethodError: method not found: 'a' on null #125).


Please review the slightly updated validation report JSON format. Given that there's an interest in providing TypeScript definitions, we should make them consistent, especially around null/undefined values.

@zeux
Copy link

zeux commented Nov 12, 2019

It looks like there's an issue with validating normal/tangent data for quantized normalized accessors.

Here's an example:
pirate.glb.zip

I'm getting the following type mismatch when trying to validate buffer contents here:

C:\work\glTF-Validator>dart bin/gltf_validator.dart -r pirate.glb -a | more                                                                                                                                                                                                          Unhandled exception:
type 'int' is not a subtype of type 'double' of 'value'
#0      UnitVec3FloatChecker.check (package:gltf/src/utils.dart)
#1      _processAccessorElements (package:gltf/src/data_access/validate_accessors.dart:218:35)
#2      validateAccessorsData (package:gltf/src/data_access/validate_accessors.dart:75:3)
#3      ResourcesLoader.load (package:gltf/src/data_access/resources_loader.dart:76:11)
<asynchronous suspension>
#4      _processFile (package:gltf/cmd_line.dart:313:27)
<asynchronous suspension>
#5      run (package:gltf/cmd_line.dart:265:15)
<asynchronous suspension>
#6      main (file:///C:/work/glTF-Validator/bin/gltf_validator.dart:20:41)
#7      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:303:32)
#8      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:172:12)

@lexaknyazev
Copy link
Member Author

@zeux
Thanks for the repro case! Fixed.

@lexaknyazev
Copy link
Member Author

By the way, what asset did cause the original stack trace (with MinIntegerChecker)?

@zeux
Copy link

zeux commented Nov 12, 2019

@lexaknyazev This was me copy&pasting the error from a wrong build, sorry about that - I tried to fix the UnitVec3FloatChecker error by adding a .toDouble() cast in accessor validation, but that broke integer validation. After your fix I don't see any crashes on my test data.

@zeux
Copy link

zeux commented Nov 12, 2019

I suspect this is just not implemented yet but it looks like this change only adds extra attribute types for base attributes when KHR_quantized_geometry is enabled, but not for morph targets? Here's a morph target example using quantized geometry, this gets a few errors in the morph target accessors:

alien.glb.zip

        Errors:
                /meshes/0/primitives/0/targets/0/NORMAL: Invalid accessor format '{VEC3, BYTE normalized}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').
                /meshes/0/primitives/0/targets/0/POSITION: Invalid accessor format '{VEC3, SHORT}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').
                /meshes/0/primitives/0/targets/0/TANGENT: Invalid accessor format '{VEC3, BYTE normalized}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').
                /meshes/0/primitives/0/targets/1/NORMAL: Invalid accessor format '{VEC3, BYTE normalized}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').
                /meshes/0/primitives/0/targets/1/POSITION: Invalid accessor format '{VEC3, SHORT}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').
                /meshes/0/primitives/0/targets/1/TANGENT: Invalid accessor format '{VEC3, BYTE normalized}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').

@lexaknyazev
Copy link
Member Author

@zeux
Done.

@zeux
Copy link

zeux commented Nov 12, 2019

@lexaknyazev Thanks! After this change I've confirmed that running all models from glTF-Sample-Models through gltfpack (which quantizes geometry & animation data) and running the output through gltf-validator with -r doesn't result in any errors on any models.

@lexaknyazev
Copy link
Member Author

@zeux
By "animation data" you mean quaternions and weights, right? Translation and scale support only float32.

@zeux
Copy link

zeux commented Nov 12, 2019

By "animation data" you mean quaternions and weights, right? Translation and scale support only float32.

Yeah - correct.

@lexaknyazev
Copy link
Member Author

doesn't result in any errors on any models

Monster, CesiumMan, and RiggedFigure (all three from the Sample-Models repo) have non-normalized vertex skinning weights. Does gltfpack fix that?

@zeux
Copy link

zeux commented Nov 12, 2019

Yeah - gltfpack renormalizes the skinning weights after quantization. This is necessary because if the weights don't add up to exactly 1 (which happens trivially when quantizing individual weights), vertices can be deformed very significantly because the weight error will get multiplied by the joint position, so an error of 1/255.f in the weight sum is actually unacceptably large. As a side effect I guess it fixes the validation errors :)

Some of these files do generate validation warnings after gltfpack because some skinning weights get rounded to 0, but the joint index is preserved, so I get "Joints accessor element at index 60 (component index 0) is used with zero weight but has non-zero value (1).". I'll look into cleaning this up.

@lexaknyazev
Copy link
Member Author

so an error of 1/255.f in the weight sum is actually unacceptably large

Does it mean that we should enforce strict 1.0 (i.e. 255 or 65535) sum for quantized vertex weights?

@zeux
Copy link

zeux commented Nov 12, 2019

Yeah - it should be safer to validate the sum against 1.0 with some floating point epsilon that is independent of quantization, and enforce this in the same way regardless of the encoded type. While in principle renderers can normalize the weight in the shader during transformation, neither three.js nor Babylon.JS do this, and the spec does say that weights should sum to 1.0.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Nov 12, 2019

Assuming that normalization happens on original float data (prior to quantization), what do you think of the effective epsilon depending on the number of non-zero weights. Namely:

  • 1 weight: 0 (just write 255).
  • 2 weights: also 0 (given that quantization uses round(), two values would always sum to 255).
  • 3 or 4 weights: 1/255.
  • 5 or 6 weights: 2/255.
  • 7 or 8 weights: 3/255.

These thresholds would obviously cover unorm16 case.

@lexaknyazev
Copy link
Member Author

@zeux
Could you please run the latest commit against gltfpack output?

@zeux
Copy link

zeux commented Nov 12, 2019

While this change also produces no errors, I don't think I understand the logic behind it.

If normalization happens on original float data without regards to quantization and individual weights are quantized, then even for two weights the sum isn't guaranteed to be 1.0 - if you start with two equal weights (0.5), the normalizing them retains the weights, and then quantization produces round(0.5 * 255) = 128 for each of them, for a total of 256/255 > 1.

As per my comments above, if you introduce an error into the weight sum, then you can have noticeable distortion in the output if skinning is done in world space (as prescribed by glTF) and the joint offsets are large.

So if we are assuming a "perfect" output, then I'd expect the threshold to be approximately equal to the floating point error of w0 / wsum + w1 / wsum + w2 / wsum + w3 / wsum that is independent of quantization, because it's practical to renormalize after quantization which is what gltfpack does. Indeed, if I change threshold to final threshold = 1e-6, all files from glTF-Sample-Models validate after gltfpack.

@lexaknyazev
Copy link
Member Author

round(0.5 * 255) = 128 for each of them, for a total of 256/255 > 1

Yeah. Apparently, my inputs never had exact 0.5 value.

the threshold to be approximately equal to the floating point error of w0 / wsum + w1 / wsum + w2 / wsum + w3 / wsum

Would this error depend on the number of non-zero weights? Practically, there could be from 1 to 8 of them per vertex.

@zeux
Copy link

zeux commented Nov 12, 2019

Would this error depend on the number of non-zero weights? Practically, there could be from 1 to 8 of them per vertex.

Hmm - maybe. I would expect that, handwaving a bit:

  • When weights are floats, weight sum produces 0.5 ulp roundoff error for each addition, which can add up to 4ulp for 8 weights

  • Dividing each weight by the sum adds 0.5 ulp for each weight, for a total of 4ulp more

  • Adding the results up produces the same error again, which adds up to 12ulp or so. With 1ulp = 1e-07 around 1, the total is around 1.4e-6

  • When weights are quantized and pre-normalized, the sum of the weights is exactly 255 / 65535 as stored, but I'd expect the hardware normalization (as in "normalized accessors") to induce up to 0.5 ulp for each division (total of 4 ulp), and the sum introduces an extra 0.5 ulp per add => total error is 8 ulp or 0.9e-6

I've tested this with threshold=1e-6 on existing models before quantization but admittedly none of them have 8 weights. We could perhaps be a bit conservative and set the threshold to 1e-5 - or, maybe in line with your suggestion, to 1.5 ulp (= 1.785e-7 ~= 2e-7) per non-zero weight?

@zeux
Copy link

zeux commented Nov 12, 2019

By the way, just for exposition, here's a character where 8-bit weights have not been renormalized post quantization, with screenshots for root joint offset ~=0, root joint offset ~= 3x character size, root joint offset ~= 30x character size - the camera follows the character:

image

image

image

This is with weights normalized before quantization and then quantized through rounding. When you normalize after quantization, the last screenshots looks clean:

image

The screenshots are produced with three.js.

@lexaknyazev
Copy link
Member Author

When values are stored as integers and their sum is exactly 255 or 65535, there's nothing else to check, right? Separating this case is relatively easy and will actually make validation faster.

Since the validator already tracks the number of non-zero weights per each vertex (no matter how many influence sets), we could set the threshold to 2e-7 * weightsCount for float32 weights.

What do you think?

@zeux
Copy link

zeux commented Nov 12, 2019

When values are stored as integers and their sum is exactly 255 or 65535, there's nothing else to check, right? Separating this case is relatively easy and will actually make validation faster.

Yeah, correct - if you want to separate this case this works (although the 2e-7 threshold should cover it trivially I think). This looks good to me assuming this math actually passes on real models :D

@lexaknyazev
Copy link
Member Author

@bghgary
undefined in API is basically the same as omitted value in JSON.

@emackey
Explicit null slightly improves readability (when seeing JSON in editor) for fields that would otherwise have some useful value. On the other hand, null is a separate value type so it complicates the API.

The other question is how to deal with enums. Consider /info.resources[].storage. It can be one of:

  • data-uri
  • buffer-view
  • glb
  • external

This could be expressed as

type Storage = "data-uri" | "buffer-view" | "glb" | "external";

When the corresponding resource object (image or buffer) is invalid, what value should this field have:

  • null;
    storage: Storage | null;
  • "unknown" (or some other similar string);
    storage: Storage; // Just add "unknown" to the list above
  • omit entirely?
    storage?: Storage;

@emackey
Copy link
Member

emackey commented Dec 3, 2019

My vote is still undefined (omit property), even for Storage (as opposed to an "unknown" string). I don't have any strong reasoning here, I just think it simplifies the report a little.

@lexaknyazev
Copy link
Member Author

Since there was no further feedback, I'm going to change the definitions of all optional or unknown fields in the report so that they could be omitted (undefined).

@bghgary @donmccurdy Any objections?

@donmccurdy
Copy link
Contributor

Looks good!

@bghgary
Copy link

bghgary commented Jan 8, 2020

No objections!

@lexaknyazev lexaknyazev merged commit 1be41dd into master Jan 9, 2020
@lexaknyazev lexaknyazev deleted the dev branch January 9, 2020 16:16
@emackey
Copy link
Member

emackey commented Jan 9, 2020

The CHANGELOG.md still shows November 2019, do you want to correct that in master to January 2020?

Done.

@emackey
Copy link
Member

emackey commented Jan 9, 2020

Published to npm. /cc @donmccurdy

https://www.npmjs.com/package/gltf-validator

@emackey
Copy link
Member

emackey commented Jan 10, 2020

VSCode's glTF Tools 2.2.8 has been updated to this version of the validator.

@bghgary
Copy link

bghgary commented Jan 10, 2020

I'm trying to use browersify to make the validator work for web like I've done in the past, but I'm getting an error when loading the resulting js:

Uncaught Error: Cannot find module 'url'
at o (gltf_validator.js:formatted:11)
at gltf_validator.js:formatted:20
at Object. (gltf_validator.js:formatted:256)
at Object.3._process (gltf_validator.js:formatted:20007)
at o (gltf_validator.js:formatted:18)
at gltf_validator.js:formatted:20
at Object.4../gltf_validator.dart.js (gltf_validator.js:formatted:20029)
at o (gltf_validator.js:formatted:18)
at gltf_validator.js:formatted:20
at Object.2.gltf-validator (gltf_validator.js:formatted:231)

I'll keep digging, but if anyone happens to know what is causing this, please let me know.

@bghgary
Copy link

bghgary commented Jan 10, 2020

It looks like this comes from dart2js? What changed such that it's trying to access the url module now?

@donmccurdy
Copy link
Contributor

The new validator release is working for me on gltf-viewer.donmccurdy.com/, although I switched from Browserify to ParcelJS recently (so it's using ES modules rather than CommonJS).

@bghgary
Copy link

bghgary commented Jan 10, 2020

Ok, let me try ParcelJS.

@bghgary
Copy link

bghgary commented Jan 11, 2020

I get a very similar error with ParcelJS though I'm not using ES modules. I doubt that makes a difference though since it looks like the validator is trying to access the "url" module which is a node.js thing. Am I missing something?

I'm running this:

parcel build index.js --target browser --out-file gltf_validator.js

Where index.js is:

GLTFValidator = require('gltf-validator');

@bghgary
Copy link

bghgary commented Jan 11, 2020

@donmccurdy Do you happen to have a polyfill for url module?

@bghgary
Copy link

bghgary commented Jan 11, 2020

One more thing I should mention is that I'm running this in a web worker.

@donmccurdy
Copy link
Contributor

Not intentionally, but if I run npm ls url it shows...

Screen Shot 2020-01-10 at 7 42 59 PM

... so I guess ParcelJS is installing a polyfill somehow.

@lexaknyazev
Copy link
Member Author

@bghgary @donmccurdy
This is 100% related to running in a web worker. I'll look into fixing that this weekend.

@bghgary
Copy link

bghgary commented Jan 13, 2020

This is 100% related to running in a web worker. I'll look into fixing that this weekend.

Thanks @lexaknyazev! I'm curious what is going on once you figure it out. I'll try to polyfill url.pathToFileURL manually for now.

@bghgary
Copy link

bghgary commented Jan 13, 2020

Ah, after looking at the code more carefully, I see what you mean by it's related to web worker. I managed to work around the problem by setting an empty window object on the web worker global scope. I'm unblocked but it will still be good to fix it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants