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

Switch to tesselated fill rendering with earcut #1606

Merged
merged 4 commits into from
May 17, 2016
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Oct 8, 2015

A fresh recut of #948. Had to squish into one commit because all previous branches were a big mess. cc @jfirebaugh @ansis @kkaefer

fixes #2269
fixes #682

@mourner mourner added performance ⚡ Speed, stability, CPU usage, memory usage, or power usage rendering labels Oct 8, 2015
@mourner mourner mentioned this pull request Oct 8, 2015
11 tasks
@mourner
Copy link
Member Author

mourner commented Oct 8, 2015

All good except one fill pattern render test! Rendering is super-blazing-fast.

At first I thought some sliver lines I saw were earcut artifacts, but they turn out to be present on master as well:
image

@mourner
Copy link
Member Author

mourner commented Oct 8, 2015

The fill-pattern/zoomed test looks very good actually, and there is definitely something fishy about the diff algorithm. Should we add it to ignore list @jfirebaugh? Otherwise the PR looks great.

@jfirebaugh
Copy link
Contributor

We should update the expected rendering for all of the fill tests.

@lucaswoj
Copy link
Contributor

lucaswoj commented Oct 9, 2015

@jfirebaugh @mourner Great work! Do you have an estimate on how long it will be until this can land in a release? It resolves #930, which is blocking a client of ours.

@jfirebaugh
Copy link
Contributor

The blocker for adopting earcut is getting confidence that the tessellation is good enough. This is a function of both the quality of our vector tile generation pipeline, and the robustness of earcut itself, and myself, @mourner, @lbud, @flippmoke, and @springmeyer are all pushing on this at the moment. We should have a better estimation next week.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 2, 2015

Rebased this on master (with #1645) and pushed to earcut-final-final.

@jfirebaugh
Copy link
Contributor

@lucaswoj Looks like earcut-final-final is still pointing at the same commit as earcut-final. Feel free to just force push earcut-final.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 2, 2015

Ok. Force pushed here and deleted earcut-final-final. (Just in case, previous SHA was 44ce255)

@mourner
Copy link
Member Author

mourner commented Nov 11, 2015

@lucaswoj let's rebase on master to make sure it's in mergeable state

@lucaswoj
Copy link
Contributor

Done @mourner 🚀 (previous SHA was 494bb19)

@lucaswoj lucaswoj force-pushed the earcut-final branch 2 times, most recently from 47d8795 to 0a7ad6a Compare November 16, 2015 20:09
@lucaswoj
Copy link
Contributor

We can close #930 when this lands

@averas
Copy link
Contributor

averas commented Jan 8, 2016

Do you have a fresh estimate on when this could be merged? I'm making heavy use of fill-layers and this branch is much more performant than master.

@lucaswoj lucaswoj force-pushed the earcut-final branch 2 times, most recently from fe743fe to 094b35f Compare May 13, 2016 22:44
} else {
console.log({bad: '@@GOOD@@', zoom: this.zoom, length: layer.length, coord: this.coord});
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Did you commit this unintentionally?

Copy link
Contributor

@lucaswoj lucaswoj May 16, 2016

Choose a reason for hiding this comment

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

@mourner Intentionally (though perhaps not not in good practice). I was debugging some serverside problems with @springmeyer.

@lucaswoj
Copy link
Contributor

lucaswoj commented May 16, 2016

This is ready to ship pending

cc @mourner @jfirebaugh @jakepruitt @ansis @springmeyer

@jfirebaugh
Copy link
Contributor

Let's eliminate convertCoords, rewriting classifyRings and the code that generates the flattened array for earcut in terms of the {x: x, y: y} point-geometry objects. This will both simplify the code and avoid GC churning a large number of 2-element arrays.

@mourner
Copy link
Member Author

mourner commented May 17, 2016

@jfirebaugh yep, done.

@mourner
Copy link
Member Author

mourner commented May 17, 2016

The test failed because of some really weird Git/NPM issue — tried rebuilding without cache and it still failed with some "git bad object" errors. Not sure whether this is something temporary or broken in one of the subdeps.

@mourner
Copy link
Member Author

mourner commented May 17, 2016

Ah, it failed because the test suite ref it pointed to no longer exists for some reason.

@mourner
Copy link
Member Author

mourner commented May 17, 2016

OK, rebased again with updated test-suite and it passes.

@jfirebaugh
Copy link
Contributor

👍 🚢 🎉

@mourner mourner merged commit b3f1bef into master May 17, 2016
@mourner mourner deleted the earcut-final branch May 17, 2016 16:17
@springmeyer
Copy link
Contributor

\o/ Thank you everyone!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
8 participants