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

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

Closed
styro opened this issue May 11, 2023 · 5 comments
Closed

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

styro opened this issue May 11, 2023 · 5 comments
Labels

Comments

@styro
Copy link

styro commented May 11, 2023

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
Copy link
Owner

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

@flauwekeul
Copy link
Owner

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
Copy link
Author

styro commented May 15, 2023

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
Copy link
Owner

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 pushed a commit that referenced this issue May 16, 2023
# [v4.1.1](v4.1.0...v4.1.1) (2023-05-16)

## 🐛 Bug Fixes
- [`00b2567`](00b2567)  Rings created with a radius never skip the first hex (Issues: [`#100`](#100))

## [4.1.1](v4.1.0...v4.1.1) (2023-05-16)
@github-actions
Copy link

🎉 This issue has been resolved in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants