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

Fix bug #74 #91

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix bug #74 #91

wants to merge 6 commits into from

Conversation

finscn
Copy link

@finscn finscn commented Dec 9, 2017

Only water-huge2 failed .
the log says :

not ok 18 4464 triangles when expected 4461

I think maybe something is wrong in the test-case.

Only `water-huge2` failed .
the log says : 
```
not ok 18 4464 triangles when expected 4461
```

I think maybe  something is wrong in the test-case.
@finscn
Copy link
Author

finscn commented Dec 9, 2017

With this PR , it could solve the problem in #74 :

image

@mourner
Copy link
Member

mourner commented Dec 10, 2017

Cool! Can you describe the logic in more detail here?

@finscn
Copy link
Author

finscn commented Dec 10, 2017

the key code is :

        var toRemove = false;

        if (!p.steiner) {
            if (equals(p, p.next) || equals(p.prev, p)) {
                toRemove = true;
            } else if (area(p.prev, p, p.next) === 0) {
                // If `p.prev, p & p.next` are on holes (not outer edge) ,
                // And `p.prev & p` are on the same hole ,
                // Then do NOT remove `p` .
                if (prevHole && nextHole && prevHole !== nextHole && prevHole === currentHole) {
                    toRemove = false;
                } else {
                    toRemove = true;
                }
            }
        }

But there is something I can't understand about

            if (equals(p, p.next) || equals(p.prev, p)) {
                toRemove = true;

if no || equals(p.prev, p) , the test-case in #74 will be failed.

@finscn
Copy link
Author

finscn commented May 6, 2018

Is there any news about this pr & issue ?

@finscn
Copy link
Author

finscn commented Jun 7, 2018

Is there any news about this pr & issue ???

@mourner
Copy link
Member

mourner commented Jun 9, 2018

@finscn sorry, I didn't have the chance to thoroughly review it yet. It's on my list. Feel free to use your forked version in the mean time.

@stefnotch
Copy link

I don't mean to add useless noise, but I'd be interested if there is a chance of this PR getting merged anytime soon.

JaffaKetchup added a commit to JaffaKetchup/dart_earcut that referenced this pull request Jan 31, 2024
Updated LICENSE to be in-line with Mapbox's ISC requirements
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.

3 participants