mapbox / earcut.hpp

Fast, header-only polygon triangulation
ISC License
858 stars 133 forks source link

Clean up interface #17

Closed jfirebaugh closed 8 years ago

jfirebaugh commented 9 years ago

The public interface should be:

template <typename Polygon, typename N = uint32_t>
std::vector<N> earcut(const Polygon&);

The Coord type should be deduced, and everything else should be hidden in a detail namespace.

mrgreywater commented 8 years ago

right now this could be easily achieved by adding

    template <typename N = uint32_t, typename Polygon>
    std::vector<N> earcut(const Polygon& poly) {
        mapbox::Earcut<N> earcut;
        earcut.operator()<Polygon>(poly);
        return earcut.indices;
    }

at the end of the mapbox namespace, and putting everything else in an anonymous (or mapbox::detail) namespace. This would of course break the api and therefore it requires the appropriate edits in the tests.

If this is what the maintainers of this project want, I would gladly create a pull request with the required edits.

Usage would be like this:

    using Point = std::array<double, 2>;
    using N = std::uint32_t;
    std::vector<std::vector<Point>> vertices = {
        {
            { 0,0 },
            { 0,1 },
            { 1,1 },
            { 1,0 },
        }
    };
    std::vector<N> indices = mapbox::earcut<N>(vertices);

It would be simpler to use, but earcut would loose the ability to reuse the allocated indices buffer for new calculations (performance) or return additional data like the AABB bounding volume in the future.

mrgreywater commented 8 years ago

Any comment on it? I could also just remove the Earcut class completely, and make it look more like the earcut.js with seperate functions (in a detail/anonymous namespace). But i want to be sure that if I do the work it will actually get merged eventually.

mourner commented 8 years ago

I'm not a C++ developer so I'll wait for @jfirebaugh to weigh in :)

jfirebaugh commented 8 years ago

I think maintaining some state in an Earcut object makes sense, rather than passing state into every helper method. So I like your original suggestion of adding the wrapper and putting everything else in a namespace.