Improved error output about cgltf HOT 9 OPEN

I think this is a good thing to discuss, and it's worth mentioning that the Filament client deals with this by calling cgltf_validate in its debug build, which provides reasonable feedback when CGLTF_VALIDATE_ENABLE_ASSERTS is enabled.

from cgltf.

My 2 cents would be that it's only meaningful with human-readable error output if the user can reasonably be expected to fix the error. If the glTF is referring to a missing external file or runs out of memory while processing - fine. If the JSON is malformed or there is a data integrity issue, I'm less sure. Isn't it better if the user reports the issue to the exporting application devs then?

from cgltf.

I believe it would be great to have better error reporting! I'd also love it to be optional. 👍

@pezcode Would cgltf_validate() help you as well? Could the error reporting functionality be added to that?

@LxLasso Generally, you're right. But you may still have expert users who just want to find out what's wrong with their glTF file and fix it. Error messages could very well help in that case. Maybe you don't have to throw the error at everyone, but hide them behind some "more details" button. Also, the file might not come from some proper exporting application, but it might be generated or hand-crafted even. Or maybe someone is writing a new exporter and would like help from cgltf to get it up and running?

from cgltf.

Fair enough, I've been there myself. I used https://github.khronos.org/glTF-Validator/ then though.

from cgltf.

The cases we've been running into with @pezcode (in Magnum's cgltf-based importer) were mostly due to various syntax or out-of-bounds errors, which caused the initial cgltf_parse() to fail. Which means we get nothing except for the error code, so there's nothing cgltf_validate() could attach to.

But of course because cgltf uses pointers for references to meshes etc. (and not indices like for example tiny_gltf), it has to do this checking during the initial parsing already, there's no way around that. It would just be great to get more context for the error state. Optionally of course, I'm well aware of the use cases where binary size matters most.

As @jkuhlmann says, we fall into the "expert users" category where we hand-craft or generate glTFs ourselves and having to use an external validator every time cgltf says "nah" is a productivity killer :)

As far as I can see, these are the main cases:

1. A glTF syntax error (such as a number where an array is expected). This seems to be mostly guarded by CGLTF_CHECK_TOKTYPE() and related macros:

   cgltf/cgltf.h

   Lines 2361 to 2363 in dd70e93

   	#define CGLTF_CHECK_TOKTYPE(tok_, type_) if ((tok_).type != (type_)) { return CGLTF_ERROR_JSON; }
   	#define CGLTF_CHECK_TOKTYPE_RETTYPE(tok_, type_, ret_) if ((tok_).type != (type_)) { return (ret_)CGLTF_ERROR_JSON; }
   	#define CGLTF_CHECK_KEY(tok_) if ((tok_).type != JSMN_STRING || (tok_).size == 0) { return CGLTF_ERROR_JSON; } /* checking size for 0 verifies that a value follows the key */

   If these would be guarded in an #ifndef CGLTF_CHECK_TOKTYPE etc, the user would have a possibility to override it and print things like "Invalid primitive token at 55467, expected array":

   #define CGLTF_CHECK_TOKTYPE(tok_, type_) \ 
       if(tok_.type != type_) { \
           printError("Invalid {} token {} at {}, expected {}", \
               TokenNames[tok_.type], tok_.start, TokenNames[type_]); \
           return CGLTF_ERROR_JSON; \
       }
   #include <cgltf.h>

   If the user code would need to do something advanced like figuring out line/column or a JSON path from token position, then it has to do that via some global state -- I don't think it's worth to complicate cgltf by passing some user_state pointer to each and every macro.

2. A glTF OOB error (such as a mesh reference out of bounds), which are checked by CGLTF_PTRFIXUP() and related macros:

   cgltf/cgltf.h

   Lines 2366 to 2367 in dd70e93

   	#define CGLTF_PTRFIXUP(var, data, size) if (var) { if ((cgltf_size)var > size) { return CGLTF_ERROR_JSON; } var = &data[(cgltf_size)var-1]; }
   	#define CGLTF_PTRFIXUP_REQ(var, data, size) if (!var || (cgltf_size)var > size) { return CGLTF_ERROR_JSON; } var = &data[(cgltf_size)var-1];

   Same case, if these would be guarded in an #ifdef CGLTF_PTRFIXUP etc, with some effort the user could provide a message such as "Index 67 out of bounds for 15 buffer_views":

   #define CGLTF_PTRFIXUP(var, data, size) \
       if(var) { \
           if((cgltf_size)var > size) { \
              printError("Index {} out of bounds for {} {}", \
                  var - 1, size, #data + sizeof("data->") - 1); \
              return CGLTF_ERROR_JSON; \
           } \
           var = &data[(cgltf_size)var-1]; \
       }
   #include <cgltf.h>

   Ideally it would also say where the index comes from, but I don't see an easy way to do that without saving a token position to each cgltf_buffer_view etc, and then passing extra context to each macro. This is good enough.

3. A JSON syntax error (such as a stray comma), where jsmn parsing fails already. This currently results in cgltf_result_invalid_json here (and a similar case before that):

   cgltf/cgltf.h

   Lines 5919 to 5923 in dd70e93

   	if (token_count <= 0)
   	{
   	options->memory.free(options->memory.user_data, tokens);
   	return cgltf_result_invalid_json;
   	}

   The jsmn parser seems to remember the last token position when it fails, which could provide at least some context for the user. So I'm thinking about putting the above in a macro guarded by #ifndef again:

   #ifndef CGLTF_CHECK_JSON_PARSED
   #define CGLTF_CHECK_JSON_PARSED(parser, tokens, token_count, memory) \
       if (token_count <= 0) \
       { \
           memory.free(memory.user_data, tokens); \
           return cgltf_result_invalid_json; \
       }
   #endif

   And then the user could again override this to produce an error message, possibly peeking into the tokens array before it gets freed to provide more context:

   #define CGLTF_CHECK_JSON_PARSED(parser, tokens, token_count, memory) \
       if(token_count <= 0) { \
           printError("Invalid JSON at {}", parser->pos); \
           memory.free(memory.user_data, tokens); \
           return cgltf_result_invalid_json; \
       }
   #include <cgltf.h>

4. Various other cases of return CGLTF_ERROR_JSON, such as arrays having wrong size or a JSON map having two keys of the same name. There doesn't seem to be that many variants but I'm not sure what to do here yet. Probably something similar to the CGLTF_CHECK_JSON_PARSED() macro suggested above, having each variant go through a macro that gets the relevant context (token position, array actual vs expected size, which key is duplicated, ...). I'd defer this to later.

Regarding cgltf_validate(), we experimented with making CGLTF_VALIDATE_ENABLE_ASSERTS output human-readable but ultimately ended up replicating its internals on our side. With more verbose error reporting and additional checks for vendor extensions etc it doesn't handle, and we defer the checks to a point when a particular mesh / animation / ... gets loaded instead of doing everything upfront. So on that side we don't need any changes.

Huh, sorry for such a lengthy post. The TL;DR variant of it is that the initial version of this would only need a few tiny changes with no negative impact for existing users or maintainers -- a couple of #ifndefs to allow overriding macros, and one new CGLTF_CHECK_JSON_PARSED() macro to give us a possibility to peek into jsmn's state on error. If you think this is reasonable, we can submit a PR with these changes.

from cgltf.

In general, I like the approach. There are two things, however, that I think could use a little more work:

1. Your overrides duplicate the code that is already there and this might break if the code in cgltf would ever change.
2. For the places, where we don't use macros yet, it adds more macro usage and makes it harder to read.

Do you think it would make sense to keep the functionality in the macros and instead just let the user add the error handling?

Something like this:

#define CGLTF_REPORT_ERROR_PTRFIXUP(var, data, size) \
    printError("Index {} out of bounds for {} {}", \
               var - 1, size, #data + sizeof("data->") - 1); \

#ifndef CGLTF_REPORT_ERROR_PTRFIXUP
#define CGLTF_REPORT_ERROR_PTRFIXUP(var, data, size)
#endif

#define CGLTF_PTRFIXUP(var, data, size) \
    if (var) { \
        if ((cgltf_size)var > size) { \
            CGLTF_REPORT_ERROR_PTRFIXUP(var, data, size); \
            return CGLTF_ERROR_JSON; \
        } \
        var = &data[(cgltf_size)var-1]; \
    }

from cgltf.

Do you think it would make sense to keep the functionality in the macros and instead just let the user add the error handling?

Yes, that would be even better, good idea :) I was just trying to come up with a minimally invasive change that would enable us to do what we need. But you have a valid point in that it'd make the overrides rather brittle and cgltf's internals harder to read.

Should I then prepare a PR with changes that follow your suggestion?

from cgltf.

That would be great and very much appreciated!

from cgltf.

Just FYI, and sorry for the very late comment -- as our use case ended up diverging even further from cgltf's goals, we ended up making our own parser from scratch. Thus we no longer have a need for the changes proposed here, and I'm afraid I won't find time to PR the implementation.

Feel free to close the issue. Thanks for all your work nevertheless -- cgltf made us realize that efficient glTF parsers can exist :)

from cgltf.