flauwekeul / honeycomb

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

Grid is not calling `.map()` #67

Closed vfonic closed 3 years ago

vfonic commented 3 years ago

Here's a test I wrote that is not throwing an error, but it should:

describe('rectangle()', () => {
  test('returns a new grid', () => {
    const grid = new Grid(createHexPrototype(), rectangle({ width: 2, height: 2 })).map((h) => h).run()
    const newGrid = grid.map((h) => {
      throw new Error('')
    })
    grid.run()
    newGrid.run()
  })
})

This test throws an error, as expected:

describe('rectangle()', () => {
  test('returns a new grid', () => {
-    const grid = new Grid(createHexPrototype(), rectangle({ width: 2, height: 2 })).map((h) => h).run()
+    const grid = new Grid(createHexPrototype(), rectangle({ width: 2, height: 2 })).map((h) => h) //.run()
+    grid.run()
    const newGrid = grid.map((h) => {
      throw new Error('')
    })
    grid.run()
    newGrid.run()
  })
})

version: 4.0.0-alpha.3

flauwekeul commented 3 years ago

Yes! Thanks for finding this 🏆

The bug is that after run() is called, the grid's internal hexes are cleared. So when you call any iterator method after run(), it won't iterate over any hexes.

I wanted to prevent that the iterators before the run() call would be run again, but it shouldn't also clear all hexes. I've made a fix and probably make a new release in a couple of days (with other changes).

vfonic commented 3 years ago

Thank you!!!

I'll just pull the last commit and use it directly from there. :)

I switched from .map() back to .each(). I'll see if I can switch back to .map().

I'm afraid to use grid.hexes() or grid.map() as I've had a bad experience doing so. I'm really abusing the library at this point unfortunately. :/ I call .each() once and then push all the hexes to an array and then access the hexes from the array. :/

I'll see if I can make it work by not fighting the library, and if I find a bug if I can write a reproducible test for it.

flauwekeul commented 3 years ago

Could you tell me what you're trying to achieve? Or are you just trying to find bugs / unexpected behaviour?

vfonic commented 3 years ago

I'm building a board game solver of sorts for Cryptid game. I'm really just trying to use the library and not to think about it too much. Maybe I should've gone with the v3.

Here's the code: https://vfonic.github.io/honeycomb/ https://github.com/vfonic/honeycomb/tree/gh-pages

tl;dr game rules You need to guess a hex that fits all the players. You can ask players and, based on their response, you can eliminate squares. I need an algorithm that's going to iterate over all of the hexes (immediately, no lazily) and try to guess which hex is the one.

I noticed that the rendering was taking couple of seconds each time. This was because calling .each() was rendering/adding DOM/SVG elements one by one. I've since changed that to use the good ol' .innerHTML and just append all of the hexes at once in a last step: https://github.com/vfonic/honeycomb/blob/gh-pages/playground/render.ts#L13-L19 This made the grid render much faster.

vfonic commented 3 years ago

The idea is to just build this solver and try it out when playing with friends. After that I'll abandon the project. It doesn't have to be perfect code, just a working proof-of-concept.

vfonic commented 3 years ago

Here's another grid bug that I just encountered:

  test(`can mutate hexes inside update function`, () => {
    const hexPrototype = createHexPrototype<TestHex>()
    const hex = createHex(hexPrototype, { q: 0, r: 0 })
    hex.test = 1
    const newHex = cloneHex(hex)
    newHex.test = 2
    const callback = jest.fn((grid) => {
      grid.store.set('0,0', newHex)
    })
    const grid = new Grid(hexPrototype, () => [hex])
    const newGrid = grid.update(callback)

    expect(newGrid.store.get('0,0')!.test).toBe(2)
    expect(newGrid).not.toBe(grid)
    newGrid
      .each((hex) => {
        expect(hex.test).toEqual(newHex.test)
      })
      .run()
  })

I tried updating a hex as per the readme instructions, using grid.update() and within the update() method I called grid.store.set(). This works, if you're calling grid.store.get() to get the new hex. However, if you try using grid.each(), you get the old hex returned.

vfonic commented 3 years ago

For some reason, I cannot use map() here: https://github.com/vfonic/honeycomb/blob/gh-pages/playground/index.ts#L74-L79

I'm also using hexagonsOrdered array instead of doing grid.each().run(), as I've been having issues using that.

Regarding my last comment about grid.update(). I've also tried this, but it doesn't seem to work: https://github.com/vfonic/honeycomb/blob/gh-pages/playground/index.ts#L124-L127

I've really struggling to use this library. I thought it would be cool to use a TypeScript version, but maybe I was too early to use it or maybe I'm just not using it correctly?

flauwekeul commented 3 years ago

As you've seen, I've made 2 separate issues.

I've really struggling to use this library. I thought it would be cool to use a TypeScript version, but maybe I was too early to use it or maybe I'm just not using it correctly?

For you it may have been easier to use v3 (it also has typings, so it can be used with TypeScript), but for me it's very good you're using v4-alpha. It gives me a very valuable insight in how other people use my library. Maybe you're not using v4 as I intend to, but that's because there's not much documentation yet. And even if there were plenty of documentation and examples, people write code as they're used to and that might be different from how I'd like them to.

I'll look into your code and either make changes in Honeycomb or I could give some advice how to better use Honeycomb (if you want).

vfonic commented 3 years ago

I could give some advice how to better use Honeycomb (if you want).

That would be amazing!

Glad I can be a guinea pig. :)

flauwekeul commented 3 years ago

As I said in the other issue: it's probably better if you could give me isolated pieces of code. Code snippets that show that you're expecting a certain behaviour but get a different result. Or specific functionality you're missing. For example, I saw you added hexesInRange() from v3 yourself. I can add that function (or rather something similar search for hexesInRange) with higher priority.

flauwekeul commented 3 years ago

I've thought some more about the API and that you have trouble using it. I've never used the API myself and I have my doubts about some things (that it's lazy, that most grid methods return a new grid, that the store is just a Map).

So I'll finish a feature I started yesterday (a ring traverser) and then I start to actually use my library by making a game or something. Unfortunately, I expect to have little spare time the coming months because I started a new job...

vfonic commented 3 years ago

it's probably better if you could give me isolated pieces of code

I understand. That's why I always try to write a failing test for the bug I find.

You asked how I'm using the library so I shared. :)

I saw you still didn't add hexesInRange and, since I'm not familiar with pointy grid and not very familiar with all the intricacies of various coordinate systems, I thought I'll try to port hexesInRange suitable for my use case, instead of putting more weight on your shoulders.

by making a game or something

Write a solver for Cryptid and then we can see who came up with the better algorithm. :)

vfonic commented 3 years ago

@flauwekeul this is probably not the best place to post this, but I didn't want to open another issue for this.

I've finished the Cryptid solver and I'll try it out tomorrow.

Here's the code and the app: https://vfonic.github.io/honeycomb/ https://github.com/vfonic/honeycomb/tree/gh-pages

flauwekeul commented 3 years ago

I just released alpha.4. It contains a ring() and spiral() traverser and I've tried to fix the duplicate hexes that chaining traversers caused. I'm looking forward to your opinion (of course it's fine if you don't want to).

I took a quick look at your app, it looks nice and clean. Unfortunately, I don't know the rules of the game, so I have no idea how to play it. But any game that uses hexagons is worth playing, so I'll learn this when I have more time.

flauwekeul commented 3 years ago

Closing this due to inactivity.

flauwekeul commented 2 years ago

:tada: This issue has been resolved in version 4.0.0-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: