flauwekeul / honeycomb

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

Grid's update() doesn't clone grid correctly #68

Closed flauwekeul closed 3 years ago

flauwekeul 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.

Originally posted by @vfonic in https://github.com/flauwekeul/honeycomb/issues/67#issuecomment-812941221

flauwekeul commented 3 years ago

I'll try to come back to this later today.

vfonic commented 3 years ago

Thank you! I really appreciate it!

Take your time. I'm just messing around with the library on a small hobby project.

flauwekeul commented 3 years ago

You found something I hadn't thought of yet (again), so that's very valuable 👍

I've changed update() so that it always returns a grid that iterates over the hexes in its store. I think it's what people (like yourself) would expect.

This demonstrates the changed behaviour:

const sourceGrid = new Grid(hexPrototype, rectangle({ width: 2, height: 1 }))

const updatedGrid = sourceGrid.update((grid) => {
  grid.store = new Map()
})

sourceGrid
  .each((hex) => console.log(hex)) // Hex {q: 0, r: 0}, Hex {q: 1, r: 0}
  .run()
updatedGrid
  .each((hex) => console.log(hex)) // won't be called, because the store is empty
  .run()

I'll release this in the next alpha.

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: