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

Experimenting with changes to library layout #13

Open
flippmoke opened this issue Oct 8, 2018 · 8 comments
Open

Experimenting with changes to library layout #13

flippmoke opened this issue Oct 8, 2018 · 8 comments

Comments

@flippmoke
Copy link
Contributor

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.

@mourner
Copy link
Contributor

mourner commented Oct 8, 2018

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.

@delfrrr
Copy link
Owner

delfrrr commented Oct 10, 2018

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

@jeremyabel
Copy link

@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.

@flippmoke
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@jeremyabel
Copy link

jeremyabel commented Oct 26, 2018

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 ;)

@flippmoke
Copy link
Contributor Author

@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.

@jeremyabel
Copy link

Rad, I'll give it a look!

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

No branches or pull requests

4 participants