search

fananek / hex-grid

HexGrid library provides easy and intuitive way of working with hexagonal grids.
MIT License
45 stars 8 forks source link

added a CubeCoordinates initializer that only takes x and y values. #15

Closed mgrider closed 2 years ago

fananek commented 2 years ago

What is the purpose of this change? Careful with this one. Values x, y and z are more like vectors, not a cartesian coordinates. It will work for sure but probably in a different way people might expect.

Coordinates with only two components are in fact Axial Coordinates, which are already in palce.

To be honest I'm thinking about major refactoring. Naming x, y, z turn out to be quite misleading.

mgrider commented 2 years ago

I am curious about what kind of refactor you are considering.

I'll admit I'm not 100% sure about the difference between Axial and Cube. (My understanding is that they are effectively the same.) I do like how redblobgames calls these coordinates q, r, s instead of x, y, z, because it's obvious right away that they aren't 3D coordinates. I see that is how it is in AxialCoordinates, but not in CubeCoordinates, and that does confuse me.

Mainly I just think this is cleaner at the call site:

                tiles.insert(CubeCoordinates(x: x, y: y))

than this:

                tiles.insert(try (CubeCoordinates(x: x, y: y, z: -x-y)))

But I don't feel strongly about it at all. I mostly threw it up because I've been working on a branch that allows you to "shift" all the coordinates in the grid, and this was the only change to the core package in that branch (the rest of it is in a HexGrid extension). So I was just doing some cleanup.

fananek commented 2 years ago

One thing is that Cube coordinate system requires sum of all three values to be zero. It doesn’t necessarily has to be z axis.

// All of these are perfectly valid cube coordinates 
CubeCoordinates(x: x, y: y, z: -x-y)
CubeCoordinates(x: x, y: -x-z, z: z)
CubeCoordinates(x: -y-z, y: y, z: z)

Second is data consistency. With only two values, it’s impossible make any validation because the third value is automatically calculated. Therefore an instance of coordinates is considered as valid, even if you pass some nonsense. Imagine you have a grid generated by some algorithm. Debugging such code could easily become a nightmare.

As for refactoring, I’m referring exactly to the discrepancy you mentioned. Ideally x, y and z should become q, r and s. Unfortunately it’s not as simple as renaming them. From a long term perspective, I believe it’s worth the effort.

mgrider commented 2 years ago

I'll just close this for now. NBD.