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

Restructure #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Restructure #14

wants to merge 3 commits into from

Conversation

flippmoke
Copy link
Contributor

This is still a work in progress ⚠️

This is a large amount of changes that was hinted at in #13. I probably put in more then I wanted to in this PR, but I like the way the library is laid out more personally. There is not a large wrapping class here but a series of small structures that all operate inside a single method. The return from this method is just the halfedges and triangles indexes.

auto result = delaunator(coords);

What you input as coords is now changed too, it is a templated point type that you can assign a custom getter and setter. This means that any custom point types now work. Example below:

struct my_fancy_point {
    int64_t my_x;
    int64_t my_y;
    int64_t some_z;
};

template <>
double delaunator::get_x_value(my_fancy_point const& pt) {
    return static_cast<double>(pt.my_x);
}

template <>
double delaunator::get_y_value(my_fancy_point const& pt) {
    return static_cast<double>(pt.my_y);
}

std::vector<my_fancy_point> coords = get_coords();

auto result = delaunator::delaunator(coords);

Internally there are lots of other changes, the hash is now its own struct and the finding items in the cache is pulled out of the main loop into a method. The hull is also pulled out into its own structure as well.

The current performance bottleneck is in the legalize() method. I have commented the biggest culprit of that is caused by a cache miss. This isn't any different in the previous implementation which has a big cache miss here as well when it travels to the coords in that previous version to get the x,y values for the in_circle() function.

screen shot 2018-10-30 at 4 22 04 pm

We might be able to optimize this away further if we wanted to store the x,y values in the triangle index structure, but this would likely be a potentially massive allocation as the number of triangles is rather large.

@flippmoke
Copy link
Contributor Author

Some more debugging screenshots to share context:

legalize() currently takes up the vast majority of processing time.
screen shot 2018-10-30 at 4 26 54 pm

After that the main loop takes up the second most amount of time, and then finally the sorting of the points is the 3rd.

@mourner
Copy link
Contributor

mourner commented Oct 31, 2018

@flippmoke awesome! Did you manage to get some benchmarks running? I'm very curious to see how much improvement the change to pointers brought.

@flippmoke
Copy link
Contributor Author

@mourner I am not sure it does currently provide an improvement yet. Still blocked primarily in performance in the single section above, need to analyze current master branch as well for what is the blocker.

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.

None yet

2 participants