kvark / obj

Basic Wavefront OBJ loader
Apache License 2.0
29 stars 12 forks source link

Removing Panics On Failed Parse #11

Closed cwfitzgerald closed 4 years ago

cwfitzgerald commented 4 years ago

Hey kvark, I swear you're everywhere 😄

I am looking at using this library to convert from OBJ to another silly model format, and I need to deal with invalid inputs because users are handing me this directly. It seems like everything in the library returns results with the exception of the unexpected token branches of the mtl and the obj parser.

Would you be open to a patch that turns those panics into a Err? There are a couple ways I could try to do this depending how much you care about backwards compatibility. Some ideas off the top of my head:

Let me know if you've had any thoughts about this in the past and what requirements you would want for something like this.

kvark commented 4 years ago

@cwfitzgerald fwiw, this library is not mine. I just inherited it after @csherratt deleted their account at some point (you can see it from the commits). @csherratt maybe we could transfer it back to you, if you are interested in maintaining it?

Would you be open to a patch that turns those panics into a Err?

Absolutely yes!

Adding load_err would be good for compatibility reasons, but I don't think we have that many users. Let's just have a breaking version release and change the result type.

cwfitzgerald commented 4 years ago

Sounds good! I'm also going to play around with some mtl loading things that might make sense to break api for (in a different PR) so look out for that too.