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

Improve anti-aliasing algorithm #113

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

Conversation

josieoharrow
Copy link

this fixes the issue w false positives and also a bug for returning true when false should have been returned. In addition, I added conditional handling for the "edge" cases where you variably can accept one or two zeros depending on the line they are on.

@mourner
Copy link
Member

mourner commented Jul 26, 2022

@josieoharrow Thank you so much for the PR! I'm really intrigued by the new anti-aliasing logic — this might address #74. Can you share some example images/diffs with before/after the PR? Also, would you mind fixing the linting errors?

@mourner
Copy link
Member

mourner commented Aug 25, 2022

Hey @josieoharrow, any updates on this?

@josieoharrow
Copy link
Author

Hi, @mourner thanks for the reminder! I will attach the output from before and after below:

I did notice the test script is running slower with my change. The algorithm shouldn't be slower, and maybe it has to do with the errors being thrown since the expected is a bit different, but that is worth looking into.

**AFTER (with my change) **

1diff
2diff
3diff
4diff
5diff
6diff
7diff

**BEFORE (as it was) **

1diff
2diff
3diff
4diff
5diff
6diff
7diff

@paazmaya
Copy link
Contributor

@josieoharrow great work!
Would you be able to continue this PR, as I am eager to test it in use

@josieoharrow
Copy link
Author

@paazmaya I am not sure how to continue this PR. I think the modified js file is the only one which should be changed. I tried un-adding some files which I accidentally included in this PR, but I think the build is failing now because the test cases are inconsistent with the change I made and fixing that involves re-writing the test cases... I don't think that should be done until another person verifies those changes. It has been a while since I looked at this code closely, but I recall seeing definite errors in the logic/math which this PR is aiming to address.

In the interim, I think you can use the code included in this PR on your own fork/locally depending on what you need and it should work well.

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