Twinklebear / tobj

Tiny OBJ Loader in Rust
MIT License
235 stars 47 forks source link

Add double-precision floating point feature 'use_f64' #56

Closed ipadjen closed 1 year ago

ipadjen commented 1 year ago

So, I opened the issue https://github.com/Twinklebear/tobj/issues/55, but then wrote the thing myself.

It's a feature 'use_double' that enables switching the float type from f32 to f64.

Twinklebear commented 1 year ago

Thanks for adding this feature @ipadjen ! Maybe I'm remembering another project or it was this one, but I think there had been some discussion about adding fp64 support by using some templating. I think it was actually this PR in arcball camera: https://github.com/Twinklebear/arcball/pull/3/files . I'm not sure if I want to add cgmath as a dependency here since apps using this library may be using their own math library, and I don't want to dictate/push one on them. However it looks like cgmath is pretty widely used, and we'd just be using the BaseFloat type from it, so maybe that's ok? Then you'd be able to load fp32 and fp64 OBJ files in the same codebase (might not be a use case anyone really has). Let me know what you think, since you'd be the user of the fp64 functionality.

For the CI tests:

ipadjen commented 1 year ago

I tried to keep it similar to tinyobjloader where they set the precision as a cmake option.

Just like you said, I don't have a use case for loading both f32 and f64 in the same codebase. I deal mostly with geospatial data, so a roundoff error can be pretty significant with single-precision. I personally don't know of a case where someone would benefit from both?

As for CI tests, I have no idea what's going on with windows. I can take a look into features test in the next few days.

Twinklebear commented 1 year ago

Cool, yeah keeping it as a build option is fine with me, it will help avoid adding breaking changes to the API too.

I'll take a look at the windows failure, it's pretty odd. Maybe something changed w/ cfg if or some other package.

ipadjen commented 1 year ago

Alright, sounds good!

So, https://github.com/Twinklebear/tobj/pull/56/commits/4c9e45d2fa91a83aaa7423ec5d596a9fe2e8e1ea is my try to accommodate the tests. I added the float_eq crate with a relative tolerance (r2nd here) of 1e-7 and changed all floating point assertions with it. I figured 7 decimal points would keep all the tests within the single-precision accuracy. I tried fiddling with the test values a bit, I think it works fine.

ipadjen commented 1 year ago

Also, maybe 'use_f64' would sound more Rust than 'use_double'?

Twinklebear commented 1 year ago

I agree on use_f64, that matches the Rust type name so it'll be more what people expect 👍

ipadjen commented 1 year ago

I think the linux test should pass now with the updated formatting in https://github.com/Twinklebear/tobj/pull/56/commits/6cad6b3b7ee3fef8b3bfdf3e1328cfc67ebb00c1

Twinklebear commented 1 year ago

Cool! I think this looks good, I'll make some time to look into the windows/cfg-if specific issue

Twinklebear commented 1 year ago

Looks like the issue was that we actually were setting that split debug info flag in the Cargo.toml, and it seems to now be unstable/giving an error on windows when I don't think it was in the past. I've fixed that on master so I'll squash/rebase/merge this PR.

Thanks again @ipadjen !

Twinklebear commented 1 year ago

Actually, after doing that manual squash/merge/rebase github doesn't recognize this PR as being merged :/ . Would you mind squashing the commits down? Then I can just rebase it on to master w/ the GH UI.

ipadjen commented 1 year ago

There. Seems ready to merge now!

Twinklebear commented 1 year ago

Perfect, thanks @ipadjen !

Twinklebear commented 1 year ago

This is now released in 3.2.4: https://github.com/Twinklebear/tobj/releases/tag/3.2.4