Validation for serialization about tinygltf HOT 14 CLOSED

There is a validator example using json-schema-validator https://github.com/syoyo/tinygltf/tree/master/examples/validator , so you can run it as a postprocess after serialization or in an unit tester.
(Or you could use python script if you don't need to stick with C++11)

Alternatively, I imagine the json parser implements the equals operator, so perhaps we could refactor to make use of that?

Do you mean you'd like to compare input(or serialized) glTF and glTF schema with == operator?

It could be possible and rather easy, but in such a case we need to include/prepare glTF schema files in some way(stringify and embed glTF schema files into .h are one possible solution)

from tinygltf.

In my case I really want to validate the contents of the files actually, e.g. missing parameters that exist in one but not the other, or mismatching values due to forgetting to serialize something. I'm not too worried about the schema, because if the format is not correct that will either result in a crash or show itself otherwise (e.g. through missing parameters or indeed through any other validation methods we use).

from tinygltf.

I already implemented the == operator locally, so the question right now is really just whether you want this functionality in the header or not.

Of course I still need to actually test the serialization.

from tinygltf.

I already implemented the == operator locally

Good! Is this feature only requires current tinygltf dependency(i.e. json.hpp), if so PR is appreciated.

It would also be nice if we could have out-of-bounds check for integer indices(node, texture, target, etc) which cannot be validated using JSON schema.

from tinygltf.

Do you prefer the operator==() declaration and/or implementation inside or outside of the class definition?

class ClassName
{
  bool operator==(const ClassName& other); // ... declaration
}
// ... implementation

class ClassName
{
  bool operator==(const ClassName& other)
  {
    // ... implementation with declaration
  }
}

class ClassName;
bool operator==(const ClassName& one, const ClassName& other); // ... declaration
...
class ClassName
{
}
...
bool operator==(const ClassName& one, const ClassName& other)
{
  // ... implementation
}

from tinygltf.

I think 3 is best since it has less dependency than 1
We cannot use approach 2 when the amount of implementation of == become large.

from tinygltf.

An advantage of 1 is maybe that you'll be reminded to change the operator== if you change the class?

from tinygltf.

An advantage of 1 is maybe that you'll be reminded to change the operator== if you change the class?

I see. PR with either 1 or 3 style is appreciated.

from tinygltf.

It all works and now I'm running into missing serialization; e.g. accessor.name is not serialized

Is it okay if I fix all serialization issues I encounter and make a combined pull request?

from tinygltf.

There are a few issues/concerns, not necessarily a problem:

• Looking at the code, loading embedded buffers seems to take up more memory/cpu, which can be significant for large models. The embedded data is completely stored in the buffer.uri property and then duplicated in the buffer.data property, but for external buffers it's only loaded into buffer.data directly.
• Comparing pre-serialization and post-serialization model data will not work for the buffer property and has to be handled separately if the user wants to verify it, because currently the buffer.uri property is coded to copy the gltf file name and replace the extension instead of keeping the previous buffer uri.
• Every buffer is saved with the same filename, which should either overwrite the previously written buffer or generate an exception.
• Comparing pre-serialization and post-serialization model data will not work for neither buffers nor images if they are embedded, because for obvious reasons the uri changes. This has to be handled separately by the user.

from tinygltf.

Of course the equality comparison for floats and doubles won't work; I'll have to add specific functions for those using some epsilon. This is okay?

Some of the variables are currently floats, like PerspectiveCamera::aspectRatio. Change to double?

from tinygltf.

Of course the equality comparison for floats and doubles won't work; I'll have to add specific functions for those using some epsilon. This is okay?

Yes. You may be interested in floating point comparison technique used in Catch2:

https://github.com/catchorg/Catch2/blob/master/docs/assertions.md#floating-point-comparisons

Some of the variables are currently floats, like PerspectiveCamera::aspectRatio. Change to double?

I think there won't be precision problem for variables in Camera struct, but rewriting it to double type is appreciated for keeping consistency with other variables(e.g. Node::rotation)

For other comments, I will evaluate it(pr comment/review it in github PR thread) with given PR.

from tinygltf.

Closing, as using the equals operator works fine for us.

from tinygltf.

Thank you for PR!

from tinygltf.