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

incircle predicate produces results that differ from original C code #48

Closed
bmmeijers opened this issue Sep 2, 2019 · 4 comments
Closed

Comments

@bmmeijers
Copy link
Contributor

While verifying the exact predicates implementation in Rust, I found that your
incircle code produces results that differ from the original predicates code in
C.

The problem lies in the following lines in exactpred.rs (lines 337-343):

        let mut aytcc = [0f64; 8];
        let aytcclen = scale_expansion_zeroelim(&cc, adytail, &mut aytcc);
        let temp16blen = scale_expansion_zeroelim(&aytcc[..aytcclen], cdx, &mut temp16b);

        let mut aytbb = [0f64; 8];
        let aytbblen = scale_expansion_zeroelim(&bb, adytail, &mut aytbb);
        let temp16clen = scale_expansion_zeroelim(&aytbb[..aytbblen], -bdx, &mut temp16c);

These lines should be changed as follows (variables named 'cc' have been swapped with 'bb' and vice versa):

        let mut aytbb = [0f64; 8];
        let aytbblen = scale_expansion_zeroelim(&bb, adytail, &mut aytbb);
        let temp16blen = scale_expansion_zeroelim(&aytbb[..aytbblen], cdx, &mut temp16b);

        let mut aytcc = [0f64; 8];
        let aytcclen = scale_expansion_zeroelim(&cc, adytail, &mut aytcc);
        let temp16clen = scale_expansion_zeroelim(&aytcc[..aytcclen], -bdx, &mut temp16c);

Note, the following are the original lines in predicates.c (lines 2839-2843):

    aytbblen = scale_expansion_zeroelim(4, bb, adytail, aytbb);
    temp16blen = scale_expansion_zeroelim(aytbblen, aytbb, cdx, temp16b);

    aytcclen = scale_expansion_zeroelim(4, cc, adytail, aytcc);
    temp16clen = scale_expansion_zeroelim(aytcclen, aytcc, -bdx, temp16c);

After this change, the Rust incircle code produces identical results to the
original C code.

bmmeijers added a commit to bmmeijers/spade that referenced this issue Sep 2, 2019
This change fixes the differences in produced results
between the Rust implementation and the original implementation
in C.
@Stoeoef
Copy link
Owner

Stoeoef commented Sep 2, 2019

Woah, thanks for finding this. I wonder how many other errors may have been introduced during the manual translation. Maybe it's time to do this properly with a C to rust converter. I know they're error prone, but since the underlying C files are really simple, they may actually work in this case.

Related to this issue on geo

Stoeoef added a commit that referenced this issue Sep 2, 2019
Fix for #48: incircle predicate
@bmmeijers
Copy link
Contributor Author

As far as I can tell, the orient2d and incircle code are now correct.

I've been running quite some random calculations (either point close to nearly straight line for orient2d, or close to unit circle for incircle) and I've not observed any differences between the results from predicates.c and the Rust code.

@Stoeoef
Copy link
Owner

Stoeoef commented Sep 2, 2019

As far as I can tell, the orient2d and incircle code are now correct.

I've been running quite some random calculations (either point close to nearly straight line for orient2d, or close to unit circle for incircle) and I've not observed any differences between the results from predicates.c and the Rust code.

That sounds promising, thanks for doing this and sharing the results.
I'll close the issue once I've published a new version.

@Stoeoef
Copy link
Owner

Stoeoef commented Jan 29, 2022

The predicates have been moved to crate robust. Closing.

@Stoeoef Stoeoef closed this as completed Jan 29, 2022
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

No branches or pull requests

2 participants