search

Hexworks / mixite

A GUI agnostic hexagonal grid library. Supports a multitude of grid layouts including hexagonal, triangular, rectangular and more.
Apache License 2.0
191 stars 31 forks source link

General optimizations #34

Closed HoldYourWaffle closed 7 years ago

HoldYourWaffle commented 7 years ago

Hereby the PR you asked for by mail.

It contains basically 2 things:

adam-arold commented 7 years ago

Did you run the checkstyle plugin? It says there are some checkstyle violations. I'll check out your code and look into it. Checkstlye can be a nuisance sometimes. Thanks for your PR though I'll review it ASAP.

HoldYourWaffle commented 7 years ago

Oops I completely forgot tests... :blush:

adam-arold commented 7 years ago

I think it did not run the tests because Checkstyle failed the build before it got there. :D

adam-arold commented 7 years ago

So I checked your PR and fixed the checkstyle problems. There is something which I'd like to clarify: which parts are the optimization?

I mean I'd like to know exactly what resulted in a huge speedup for you because there are some redundancies (pointList vs pointArray for example). I'd like to remove one of them but keep the stuff which is faster.

Another thing which is a bit puzzling: why vertices is useful? From what I understand it is simply a denormalized list of points so it contains all the points flattened like this: (x, y, x, y, etc...). Would you kindly clarify these?

If we fix this then I can release a new version to Maven and you can use that for your project.

The code looks good btw, thanks!

HoldYourWaffle commented 7 years ago

which parts are the optimization?

Well what I mainly did was pre-calculating some things on every hexagon-instantiation instead of when a method was invoked, wich creates a general speed boost (altough not that big). Also it just makes more sense.

I mean I'd like to know exactly what resulted in a huge speedup for you

Well I didn't really test the pre-calculations individually but I guess the pre-calculations of the points (HexagonImpl.calcPoints()) had a big part in it and not having to call List.toArray every frame could also have helped (It pre-calculates these now).

because there are some redundancies (pointList vs pointArray for example)

Well I made changes over the cours of weeks and pushed them waaay later so I don't remember clearly why they're both in here. What I can see now is that getPointList() is just a "workaround" for the "general" type List of getPoints() because my mind derped and didn't really figure out I could just edit the Hexagon interface to change it. That could be easily changed.

why vertices is useful? From what I understand it is simply a denormalized list of points so it contains all the points flattened like this: (x, y, x, y, etc...). Would you kindly clarify these?

It's exactly what you understood it is, it's useful for rendering (at least in my GDX-implementation). You could remove it, I guess I didn't fully qualify it as GDX-exclusive in my head.

The code looks good btw, thanks!

Well that's the first time in forever that I've heard that :joy:

adam-arold commented 7 years ago

Cool, then I'll leave everything in place but I'll factor the getPointArray out since an ArrayList is backed by a real array and it does not have any overhead whatsoever. Is this OK for you? I tend not to use plain arrays because they don't add anything compared to a List and using a List isn't noticeably slower.

HoldYourWaffle commented 7 years ago

Is this OK for you?

Sure it's your project :man_shrugging:

adam-arold commented 7 years ago

Cool, I've released a new version. Related reddit post here.