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

Skip processing of child paths in mergeEraser #921

Merged

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Nov 8, 2019

Resolves

Resolves #621
Resolves #120
Resolves #119

Proposed Changes

This PR modifies the shape Boolean processing loop in Blobbiness.mergeEraser to skip processing paths that are children of compound paths.

Reason for Changes

Both child paths and their parents are included in the list of items that mergeEraser will attempt to merge with. If child paths are not specifically excluded, mergeEraser will attempt to operate on them twice: once with their parent path, and one with them on their own.

Fixing this greatly improves the reliability of eraser operations:

Before After
mergeeraser-before mergeeraser-after
mergeeraser-before-2 mergeeraser-after-2

@adroitwhiz adroitwhiz force-pushed the mergeeraser-skip-child-paths branch from c773a65 to 1952b90 Compare January 9, 2020 20:00
@adroitwhiz adroitwhiz force-pushed the mergeeraser-skip-child-paths branch from 1952b90 to 41e0765 Compare January 9, 2020 20:00
@fsih
Copy link
Contributor

fsih commented Jan 9, 2020

Wow, that looks great!

@adroitwhiz adroitwhiz requested a review from fsih January 28, 2020 17:24
@fsih fsih self-assigned this Feb 6, 2020
@fsih fsih added this to the February 2020 milestone Feb 6, 2020
Copy link
Contributor

@fsih fsih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so good!! Thanks for wading through the blob code to find this!

// If a path is part of a compound path, that parent path will later be processed.
// Skip processing the child path so as not to double-process it.
if (isCompoundPathChild(items[i])) continue;

// TODO handle compound paths
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's what this comment was referring to? =_=;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that... is it okay to remove those comments then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're handling them now, so I guess so!

@fsih fsih merged commit 7ffe87e into scratchfoundation:develop Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants