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

Rough fix for issue96 #9

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

Conversation

stefannikolei
Copy link
Contributor

@stefannikolei stefannikolei commented Feb 4, 2025

This is just a rough fix for the last test failure of the js generic tests where we have a mismatch in the count of points.

This is also done in the rust version. mod.rs L:116

Do you see a better way how to handle this? Perhaps change the return value of ConnectEdge to Contour[] and then do the ordering?

When this is done only 2 tests of the js test suite are failing. And those faile because of probably some rounding problems

@JimBobSquarePants
Copy link
Member

@stefannikolei I'll have a look at this asap. I've been preoccupied looking at some gnarly ImageSharp issues.

As a side, I've been porting more of the Rust version code into a local branch. There's a bit of difference in that codebase around segment comparing plus some intersection calculation changes. It doesn't fix things yet but it may make a good starting point to getting everything working so will push the branch tomorrow.

@stefannikolei
Copy link
Contributor Author

@JimBobSquarePants I looked further into the remaining failing tests. I get a difference in the connectEdge logic. Our implementation for example return 8 as the nextPos. But the Rust implementation returns 9.

The rust implementation is using an precalculated iteration_map. I think our focus should be on this one.

I will wait for your remaining rust ports and then look further into it.

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.

2 participants