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

Change HexGrid::spacing() to accept a single scalar and maintain hexagonal grid #72

Merged
merged 2 commits into from
Dec 26, 2023

Conversation

karliss
Copy link
Contributor

@karliss karliss commented Dec 23, 2023

Keep the hexagons placed within hexagon grid when setting spacing. Previous version placed them in rectangular grid.

Note that it does change the argument type for spacing function arguments from [f64; 2] to f64 it is still possible to set separate spacing for vertical and horizontal spacing using horizontal_spacing and vertical_spacing functions.

If you want to maintain old function signature I don't mind changing using different approach.

  • introduce spacing_uniform method with single argument -> not sure if that's worth it considering that most of the time people will want the uniform spacing
  • keep the [f64; 2] argument, but multiply one of them by cos(30) -> it may be slightly confusing actual vertical/horizontal spacing differs from argument by than nontrivial multiplier, also having grid.vertical_spacing(x).horizontal_spacing(y) different behavior than grid.spacing([x, y]) can be unexpected
  • move the cos(30) multiplication next to the rest of grid calculation, the problem of actual spacing differing from numbers set is still there, but at least separate methods behave same as array version

Keep the hexagons placed within hexagon grid when setting spacing. Previous version placed them in rectangular grid.
@abey79
Copy link
Owner

abey79 commented Dec 24, 2023

This looks good to me, though I'd be interested in @afternoon2's take as they're the initial implementer if this feature.

@abey79 abey79 changed the title Fix hex grid spacing. Change HexGrid::spacing() to accept a single scalar and maintain hexagonal grid Dec 24, 2023
@afternoon2
Copy link
Contributor

lgtm! 🚀

@abey79 abey79 added the whiskers Relates to whiskers or whiskers-derive label Dec 26, 2023
@abey79 abey79 merged commit 1388503 into abey79:master Dec 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whiskers Relates to whiskers or whiskers-derive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants