delaunator-cpp icon indicating copy to clipboard operation
delaunator-cpp copied to clipboard

Experimenting with changes to library layout

Open flippmoke opened this issue 7 years ago • 8 comments

I am going to experiment with a few layout change of the library for a couple of reasons:

  • Break code out to seperate methods make algorithm more readable
  • Change names of variables and methods to make intent more clear
  • Lower amount of allocations performed during algorithm
  • Decrease the amount of memory allocated in the heap at any one period and lower overall memory footprint of algorithm
  • Decrease likelihood of cache misses
  • Migrate away from uses of indices in some portions of the code with a preference for use of pointers instead.
    • Allows for more compiler optimizations
    • Makes code more readable by specifying what index actual references into
    • Reduces memory overhead
  • Allow more generic user defined data types via templates (user can define a data structure that suits their use more easily)

You will likely see these as a series of PRs that I will try to break out into concise updates (not all at once) so that it can be well understood why certain changes are being made and the code more easily reviewed.

flippmoke avatar Oct 08 '18 18:10 flippmoke

Note that Delaunator Rust port has been refactored for better clarity, with extracted methods and renames. I'd urge to not go too far beyond that to keep the ports from diverging too much, so that we can propagate any updates from one port to another easily.

mourner avatar Oct 08 '18 19:10 mourner

Sounds great! Also I 100% agree with @mourner about importance to keep consistency with JS to keep possibility of porting new improvements.

And yes let's do it with a small PR, please!

Looking forward! Thank you @flippmoke

delfrrr avatar Oct 10 '18 07:10 delfrrr

@flippmoke How goes the progress on this? I'd like to jump on board, as I'd like to use this library within Unreal Engine, which basically just boils down to converting all the std containers into the corresponding Unreal containers, at least for now.

My general lack of C++ abilities means I can really only just copy the form of the Rust port onto the C++ port (IE, break out functions and the like), so I'm definitely happy with just starting there at least. Have you done any work already? Looking at your fork but it doesn't look like much has been touched yet.

jeremyabel avatar Oct 26 '18 02:10 jeremyabel

As this is basically a side project for me right now its been hard to carve out enough time to get this done recently. I am about halfway done with my first set of changes here, but need to find some time to finish off the rest.

flippmoke avatar Oct 26 '18 14:10 flippmoke

@jeremyabel could you provide an example layout of how your point data is organized?

flippmoke avatar Oct 26 '18 14:10 flippmoke

Sure thing! The 2D point structure is called FVector2D. Internally, almost every function in there is set to FORCE_INLINE, if that makes any difference for ya. I spent a few minutes last night just working over some of the point class functions in this gist.

Should also mention that FVector2D is all floats rather than doubles. This is fine in my case though, as I'm just making video games, not actually-important mapping software ;)

jeremyabel avatar Oct 26 '18 16:10 jeremyabel

@jeremyabel you should see the branch I am using in #14 if that layout helps you any. I didn't yet make the container for the point type completely customize-able but any container that has an interface like a STD container we could likely just have it work.

flippmoke avatar Oct 30 '18 20:10 flippmoke

Rad, I'll give it a look!

jeremyabel avatar Oct 31 '18 19:10 jeremyabel