fananek / hex-grid

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

Hex to screen wrong implementation #7

Closed jhonner closed 2 years ago

jhonner commented 2 years ago

Firstly, thanks for the great work!

I came across this library recently and tried to use it to render a hex grid, but the result was not correct, the hexagons cells were overlapping.

Going back and forth between the doc from redblobgames.com and your source I noticed there is maybe a mix up between the layout size and the hexSize used for calculating hexCornerOffset.

The original doc says : "Note that size.x and size.y are not the width and height of the hexagons." https://www.redblobgames.com/grids/hexagons/implementation.html#hex-to-pixel

Unfortunately, that's exactly what happens in hexCornerOffset, the given hexSize is used as is, so somehow each hexagon gets it's width doubled.

I fixed the rendering of my hexGridView class by updating hexCornerOffset to :

fileprivate static func hexCornerOffset(at index: Int, hexSize: HexSize, orientation: Orientation) -> Point { let angle = 2.0 Double.pi ( OrientationMatrix(orientation: orientation).startAngle + Double(index)) / 6 return Point(x: hexSize.width/2 cos(angle), y: hexSize.height/2 sin(angle)) }

Where hexSize.width and hexSize.height are simply divided by two.

Thanks again. Take care.

fananek commented 2 years ago

Hi, thanks for feedback. Interestingly this division by 2 was part of the original solution while it has been removed within pull request #6. The reason was that hexes were rendered far from each other. See the PR for details. If you wish to elaborate further, any PR is welcome. Otherwise I'll investigate during upcoming days.

jhonner commented 2 years ago

I think the original implementation was good then, because when I revert to it, there is no need for my workarounds anymore. Thanks for the quick answer.

fananek commented 2 years ago

BTW, Would you mind to share your code so it's easier to reproduce the issue? I would like to compare with issue mentioned in PR #6 and ideally find the root cause. Just to make sure when we switch back to original solution it won't cause problems to others.

jhonner commented 2 years ago

Hi yes sure, please find here my modified version of hex-grid library and also the little rendering app using it.

Thanks

Yann Bouschet

Développement iPhone/iPad Design et réalisation web

514-443-8622

http://www.yannbouschet.com

Le ven. 10 déc. 2021 à 04:15, fananek @.***> a écrit :

BTW, Would you mind to share your code so it's easier to reproduce the issue?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fananek/hex-grid/issues/7#issuecomment-990762675, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4VQMHHU33QSSUYQMRM7NDUQHAMHANCNFSM5JVFL67Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

fananek commented 2 years ago

Hi, is there any link in your reply? Can’t see anything.

jhonner commented 2 years ago

Hi,

It was supposed to be attached to the reply, maybe github chopped it off. Here is a link : http://yannbouschet.com/hexGrid/Archive.zip

Yann

Le sam. 11 déc. 2021 à 02:34, fananek @.***> a écrit :

Hi, is there any link in your reply? Can’t see anything.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fananek/hex-grid/issues/7#issuecomment-991512589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4VQMGD5EB2GT57PWNWAB3UQL5G5ANCNFSM5JVFL67Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

fananek commented 2 years ago

Thanks a lot for cooperation. Original solution has been converted back in release 0.4.6.