flauwekeul / honeycomb

Create hex grids easily, in node or the browser.
https://abbekeultjes.nl/honeycomb
MIT License
630 stars 57 forks source link

Is skipFirstHex ever valid or needed for a ring traverser? #100

Closed styro closed 1 year ago

styro commented 1 year ago

Hi, I noticed a problem when combining radius style ring traversers in that subsequent traversers after the first were missing their first hex.

This doesn't happen for the start style ring traversers due to start always being present (like most or all all traversers with a start). Because a radius style ring can't accept a start position, you can't hack around that by supplying a dummy one.

const skipFirstHex = !(options as RingOptions).start && cursor

I might be missing something, but I can't think of many cases where you want to chain rings and need to skip the start of the next ring? Especially when the start point is fixed for a radius style ring and always the Eastern direction, you don't get any control where that would be.

Duplicate hexes from multiple rings are already filtered out, so not much is gained from skipFirstHex even if you wanted it.

Would removing the skipFirstHex check break any use cases I can't see? If so, is there a clean way to implement an override?

It seems that the cursor following functionality makes sense for the 1 dimensional or sequential traversers, but probably not so much for the 2 dimensional ones. Apologies if I'm missing the point :)

flauwekeul commented 1 year ago

Thanks for asking your question. I'll get back to you in a couple of days at the latest.

flauwekeul commented 1 year ago

Can you show some example code to clarify the issue? I tried some things, but I don't quite understand what you mean.

It seems that the cursor following functionality makes sense for the 1 dimensional or sequential traversers, but probably not so much for the 2 dimensional ones.

All traversers are sequential, right? What do you mean with 2 dimensional?

styro commented 1 year ago

Thanks for the reply. Here's a contrived/simplified example:

const myHex = defineHex();

const grid = new Grid(myHex, rectangle({ width: 10, height: 10 }))

const ring1 = ring({ center: [1,1], radius: 1 });
const ring2 = ring({ center: [5,5], radius: 1 });

for (const hex of grid.traverse([ring1, ring2])) {
    console.log(hex);
}

output:

M { q: 2, r: 1 }
M { q: 1, r: 2 }
M { q: 0, r: 2 }
M { q: 0, r: 1 }
M { q: 1, r: 0 }
M { q: 2, r: 0 }
M { q: 5, r: 6 }
M { q: 4, r: 6 }
M { q: 4, r: 5 }
M { q: 5, r: 4 }
M { q: 6, r: 4 }

The start of the 2nd ring [6,5] is missing.

After a bit more looking around, I presume the use case for the cursors in the ring traversers is to allow composing with eg repeatWith in spiral. But as I think I see it (still don't fully understand the code), that is using the cursor as an implied start for a standard start type ring rather than a radius type ring. My assumption is that radius rings due to no control over the start point shouldn't bother with cursors and skipping the next start point - the start point is arbitrary on a radius specified ring.

So maybe if skipFirstHex was always false if a radius was specified? That shouldn't interfere with the spiral traverser I don't think?

I could still be missing something though.

All traversers are sequential, right? What do you mean with 2 dimensional?

Seeing the use case for it eg repeatWith in the spiral traverser, made that point a bit irrelevant. But what I meant by 1 dimensional was eg lines or arcs (I see now that a ring skipping the first is really an arc), and 2 dimensional was shapes eg rectangles or full rings. But yeah, you can ignore that point.

Anyway, thanks for a cool library. It's really helping me figure out Typescript.

flauwekeul commented 1 year ago

I see what you mean, thanks for the clear explanation. I agree it makes no sense to ever skip the first hex of rings created with a radius. It will be fixed in v4.1.1.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 4.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: