syoyo / tinygltf

Header only C++11 tiny glTF 2.0 library
MIT License
2k stars 404 forks source link

Validations on glTF Models #11

Open brycehutchings opened 7 years ago

brycehutchings commented 7 years ago

I used tinygltf in a project, and one thing I think most will need are sanity checks done on the loaded tinyglt::Model object, beyond the basics already provided. A few things which would be useful:

  1. Verify that a buffer view does not exceed the bounds of the buffer that it references. Similarly, that an accessor does not exceed the buffer view bounds, if it references a buffer view.
  2. Check for illegal loops in the node hierarchy ("no node may be a direct or indirect descendant of more than one node").
  3. Check the various index references into other sections of the glTF document are not out of range (e.g. if there are 10 nodes and a scene references node 11, that is wrong). I mention these because they could protect against potentially malicious models. Of course glTF a complex format and the number of validations could go into the hundreds I'm sure.

Now this project is self-described as "tiny" so one question I have is where does this functionality belong? Possible locations:

  1. Built into TinyGLTFLoader class.
  2. Same header, separate class.
  3. Separate header file.
  4. Completely separate project.

To me option three feels right by keeping the functionality completely optional but readily available. I'd be interested in getting the ball rolling on this. Thoughts?

syoyo commented 7 years ago

Thank you for using tinygltf and the proposal!

Validating the loaded model is important, but at the moment I have no enough dev resource for data validation so the contribution is welcome!

Sorry for late response. For some reason, there is no e-mail notification so I've missed this issue for a week.

Option 3 or 4 looks good for me. The code would be something like this:

#include "tinygltf.h"
#include "tinygltf-validator.h" // tinygltf_validator::Validator

tinygltf::Model model = ...;

tinygltf_validator::Validator validator;

std::string err;
bool ret = validator.Validate(model, &err);
if (!err.empty()) {
  std::cerr << err << std::endl;
}
...
AurL commented 6 years ago

Good point @brycehutchings, and the issues you mention are already well detected by the official glTF validator: https://github.com/KhronosGroup/glTF-Validator, you can have an overview of the detected issues here.

To be honest, I'm wondering if validation should be part of tinygltf, I guess this responsibility should be given to a dedicated tool. We are using it for glTF import and export on Sketchfab but our validation is different on each process. Tinygltf performs a validation for glTF import/processing, and for export we validate output files with the official glTF validator: https://github.com/KhronosGroup/glTF-Validator. The glTF-Validator is updated very quickly after any change in the specification, and it's a very useful tool to assert that the glTF you read or write is sane.

So I would say 4) with the official and up-to-date glTF-Validator.

@syoyo what's your opinion on this ?

syoyo commented 6 years ago

Thank you for the comment, @AurL !

So I would say 4) with the official and up-to-date glTF-Validator.

It looks gTF-Validator is now very mature, so I would also like to go with 4) + use gltf-Validator for a while.

I have a plan to rewrite tinygltf parser/writer using a code generator from JSON spec https://github.com/syoyo/tinygltf/issues/10 which can also be used for creating validator program especially suited for TinyGLTF as a derivative(approach 3), but I cannot promise when I could finish implementing #10 ...

brycehutchings commented 6 years ago

Absolutely glTF-Validator should be used to validate glTF files when possible. My thinking was more about consuming glTF files from unknown sources, potentially malicious sources. For example, I made a glTF file with circular node references. When I upload and view it on BablyonJS's Sandbox, I get the error "Unable to load from 'Cube.gltf': Maximum call stack size exceeded". To me, it would be nice to have glTF parsers catch issues like this, which would remove the burden of doing checks at a later stage. glTF-Validator can't be used in many cases at runtime, especially for native applications. To me this falls into a set of functionality that TinyGLTF doesn't expose but I assume many people need, like parsing accessors into more structured data (e.g. primitive POSITION attribute into vector of Vec3). These are both kinds of optional things which could be built on top of TinyGLTF and consumers could find useful.

syoyo commented 6 years ago

I made a glTF file with circular node references

For detecting circular node references, we could implement DFA(Data Flow Analysis) e.g. using topological sort.

Maximum call stack size exceeded

I guess this is an error in JavaScript runtime layer, not in JavaScript program itself. In C++, it is difficult to detect out-of-memory, stack-overflow, etc in pure C++ programming level without specialized compiler options and runtime library(e.g. AddressSanitizer, MemorySanitizer, VC's CRT stack protection mechanism).

Another way is to introduce secure tiny VM layer e.g. using duktape https://github.com/svaarala/duktape . This kind of approach would be better when we load unknown, malcious glTF model in Mobile environment(Android, iOS) but may be a bit overkill for TinyGLTF library...

syoyo commented 6 years ago

https://github.com/syoyo/tinygltf/tree/validator/examples/validator

I have created validator example using https://github.com/pboettch/json-schema-validator in validator branch.

Currently it only tests the syntax of glTF, but it would be a good starting point of the validation of glTF files in a C++ program.

syoyo commented 6 years ago

And now validator has been merged into devel

syoyo commented 5 years ago

devel is now master.