Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

ipadjen
Copy link
Contributor

@ipadjen ipadjen commented Jan 27, 2023

So, I opened the issue #55, but then wrote the thing myself.

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

@Twinklebear
Copy link
Owner

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:

  • looks like build_linux is just failing on a formatting thing
  • build_windows failure is kind of odd to me, I don't see changes here that should effect it and it was passing before
  • nightly_features_linux: maybe we should exclude fp64 from the all features test somehow? It looks like a precision difference is causing this failure

@ipadjen
Copy link
Contributor Author

ipadjen commented Jan 28, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

ipadjen commented Jan 29, 2023

Alright, sounds good!

So, 4c9e45d 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
Copy link
Contributor Author

ipadjen commented Jan 30, 2023

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

@Twinklebear
Copy link
Owner

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

@ipadjen ipadjen changed the title Add double-precision floating point feature 'use_double' Add double-precision floating point feature 'use_f64' Jan 31, 2023
@ipadjen
Copy link
Contributor Author

ipadjen commented Jan 31, 2023

I think the linux test should pass now with the updated formatting in 6cad6b3

@Twinklebear
Copy link
Owner

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

@Twinklebear
Copy link
Owner

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
Copy link
Owner

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.

@Twinklebear Twinklebear linked an issue Feb 2, 2023 that may be closed by this pull request
Change float comparison tests to accomodate f64 option

Change use_double flag to use_f64

Fix formatting
@ipadjen ipadjen force-pushed the feature/f64-option branch from 6cad6b3 to 6acf62a Compare February 2, 2023 07:44
@ipadjen
Copy link
Contributor Author

ipadjen commented Feb 2, 2023

There. Seems ready to merge now!

@Twinklebear
Copy link
Owner

Perfect, thanks @ipadjen !

@Twinklebear Twinklebear merged commit b93b8fb into Twinklebear:master Feb 3, 2023
@Twinklebear
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for double precision
2 participants