Add `try_*` functions that return `Result` instead of `panic`ing

[mthiesmeyer]

There is a pattern in the gltf crate of calling unwrap after calling nth on an iter, one example of which can be found here. My understanding is that the crate is written this way with the expectation that if all validations pass, none of these unwraps should actually panic. However, in our case, we are reporting all validation errors but still attempting a best-effort load of the gltf/glb data that is healthy. Since there is no way to detect that an accessor function will panic before calling it, other than some intensive introspection on the validation errors, we have resorted to using catch_unwind in these scenarios. I believe it would be more rustic, and provide a nicer interface, if these functions each had a corresponding try_* version which returned a Result and allowed the caller to determine if a panic should occur.

If this seems like a reasonable addition, I am happy to take on the work of putting up a PR.

[alteous]

Hi, your understanding is correct, the top-level crate is designed to handle glTF that is known to be valid ahead of time.

other than some intensive introspection on the validation errors

This was the intended purpose of the validation errors though. Is there a way we could make the validation errors more useful to handle these cases?

It is a reasonable request to add fallible versions of the accessor functions. My only concern is whether they would signal to the user that they should use the fallible versions, as if the panicking versions are unreliable, because checking twice for validity defeats the point of the upfront checks.

[mthiesmeyer]

Hi @alteous, thanks for the quick reply, sorry it has taken me so long to get back to this.

Is there a way we could make the validation errors more useful to handle these cases?

It is possible that this is due to my own misunderstanding, but there are a few issues I ran into with the current error propagation mechanism.

1. Errors are propagated as strings, which means that determining which index of which type failed requires expensive and mismatch-prone string matching operations.
2. Once the error information is parsed, performant lookups require creation of a secondary data structure to use while we are processing the non-json types
3. When we are processing the higher-level gltf type (not the json type) the relevant path information isn't directly accessible, meaning that we need to be keeping track of that information as we traverse the gltf data structure.

My only concern is whether they would signal to the user that they should use the fallible versions, as if the panicking versions are unreliable, because checking twice for validity defeats the point of the upfront checks.

I think that's a valid concern. Maybe there's space here for a Validated<X> newtype wrapper which uses the underlying try_ methods from the <X> type and unwraps them? That would, of course, require a whole bunch of boilerplate, which isn't ideal.

[nickbabcock]

Maybe there is an short-term solution that can be split from this?

In the short term, every function that returns an option, yet unwraps an nth(), should be updated to propagate the nth's none.

IMO this seems to maintain semantics while being a backwards compatible change (as these functions aren’t documented as panicking anyways) 🙂