flauwekeul / honeycomb

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

Are there more docs/examples? #66

Closed vfonic closed 3 years ago

vfonic commented 3 years ago

I'm having a hard time figuring out how to use this library.

I tried both master (3.1.8) and next version, but it's difficult to understand how to properly use the library.

Are there any more examples?

I have a flat grid (odd-q) and I'm trying to create 12x9 grid and set each of the hexagons one by one to their respective values (each hexagon stores some value in my application).

I have no clue how to achieve this. I've spent several hours now going between master and next, reading documentation, reading readblobgames, etc.

It would really help to have more examples or at least some documentation/explanation of what each function does.

Thanks!

vfonic commented 3 years ago

For example:

Here's the code that renders 3 hexagons:

const grid = new Grid(hexPrototype, rectangle({ width: 12, height: 9 }))
  .traverse([at({ q: 0, r: 0 }), move(Compass.SE), move(Compass.NE)])
  .filter(inStore)
  .each(render)
grid.run()

And here's the code that doesn't render anything:

const grid = new Grid(hexPrototype, rectangle({ width: 12, height: 9 }))

grid
  .traverse([at({ q: 0, r: 0 }), move(Compass.SE), move(Compass.NE)])
  .filter(inStore)
  .each(render)

grid.run()

I can't figure out why?

vfonic commented 3 years ago

I just realized that the first example in the readme doesn't work:

This doesn't output anything:

import { createHexPrototype, Grid, rectangle } from 'honeycomb-grid'

// 1. Create a hex prototype. This is an object (literally as a JS prototype) that's used by all hexes in the grid:
const hexPrototype = createHexPrototype({ dimensions: 30 })

// 2. Create a grid with this hex prototype and also pass a "traverser" for a rectangular-shaped grid:
const grid = new Grid(hexPrototype, rectangle({ width: 10, height: 10 }))

// 3. Iterate over the grid to pass each hex to a render function (that you have to supply yourself (for now)):
grid.each((hex) => render(hex))

// 4. The above won't do anything yet, that's because grid methods are executed lazily.
//    You need to call its run() method in order to execute the each() call (and most other method calls):
grid.run()

Seems like grid.each doesn't get invoked at all:

// this doesn't log anything
grid.each((hex) => {
  render(hex)
  console.log(hex)
})
flauwekeul commented 3 years ago

I tried both master (3.1.8) and next version, but it's difficult to understand how to properly use the library.

v3 of the lib should have a decent amount of documentation and examples. But that may not be sufficient still. I'm planning on adding a lot more examples and recipes for v4. Currently, v4 lacks quite some functionality (compared to v3, but also compared to what I'm planning). I'd say it's about 70% feature complete.

You're absolutely right the first(!) example doesn't work and I feel pretty stupid about it đŸ¤Ļ

The reason it doesn't work is because grid.each() returns a new grid and doesn't mutate grid. So when grid.run() is called, grid doesn't "contain" the each() call. Something like this would work:

// 3. Iterate over the grid to pass each hex to a render function (that you have to supply yourself (for now)):
const grid2 = grid.each((hex) => render(hex))

// 4. The above won't do anything yet, that's because grid methods are executed lazily.
//    You need to call its run() method in order to execute the each() call (and most other method calls):
grid2.run()

But that's a pretty shitty API ☚ī¸ Making this mistake myself proves it's not very intuitive or convenient. My apologies that you had to spend so much time on this. And thanks for opening an issue and calmly explain the problem! 🙏

Maybe the most obvious solution would be to just mutate the grid in-place and return that (instead of returning a new grid each time). But if a grid instance is used in multiple places in the code, calling a method in one place would mutate the grid in other place(s) too. This is the reason I'm returning new instances in the first place! It's also a notorious problem of Moment.js:

As an example, consider that Moment objects are mutable. This is a common source of complaints about Moment. We address it in our usage guidance but it still comes as a surprise to most new users.

The only other solution I can think of is a completely different API (inspired actually by Moment.js's best competitor: date-fns). I've had this idea for a while and this might be a good reason to implement it. Something like this:

import { createHexPrototype, each, filter, Grid, rectangle, traverse } from 'honeycomb-grid'

const hexPrototype = createHexPrototype()

// a grid would simply be a container for a hex prototype and a store and only have some methods that work with those
const grid = new Grid(hexPrototype, rectangle({ width: 10, height: 10 }))

// methods that would previously return a new grid are now functions that return a function that accept a grid:
each((hex) => render(hex))(grid)
// same as:
const run = each((hex) => render(hex))
run(grid)

// these functions can be composed using a common utility function called pipe:
const run = pipe(
  traverse([
    at({ q: 0, r: 0 }),
    move(Compass.E, 4),
    move(Compass.SW, 4),
    move(Compass.NW, 3),
  ]),
  filter(({ q, r }) => q > 0 && r > 0),
  each((hex) => console.log(hex)), // logs: { q: 3, r: 1 }, { q: 2, r: 2 }, { q: 1, r: 3 }
)
run(grid)

I believe this is a beautiful API with numerous advantages:

  1. more separation of concerns: Grid is responsible for maintaining state of hexes and the separate functions (each(), filter(), traverse(), etc) are responsible for "transforming" grids (without mutating the state in the grid instance!)
  2. lazyness is built-in: nothing happens until the returned function (called run() in this example) is called with a grid
  3. better tree shaking because all functions that aren't imported don't need to be bundled
  4. Honeycomb would probably become a lot simpler

There's one big disadvantage (which is also the reason I haven't build the API like this already): I think most of my users will be put off by this functional programming "wizardry" đŸ˜ĸ Regardless, of one thing I'm pretty sure: the current API where new grids are created with each method call needs to change...

flauwekeul commented 3 years ago

But that's a pretty shitty API ☚ī¸

So, maybe it's not that bad 😅 (a good night's sleep helps seeing things in perspective)

Most Array methods actually have the same behavior:

const array = [1, 2, 3]
array.map((item) => item * 2)
array.forEach((item) => console.log(item)) // logs: 1, 2, 3; not 2, 4, 6

So, I just leave the API as it is for now. I've updated the examples though 👍 I would still like to hear your opinion on this btw.

vfonic commented 3 years ago

Hey @flauwekeul! Thanks for the detailed reply!

I love the v4 API! It makes a lot of sense to me.

I'd just never assume that .each method would modify/return new instance. If you're aiming for similarity to Array methods, and I think that's a good idea, I'd allow .each or .forEach to simply iterate over all of the hexes. That method shouldn't return anything (or perhaps if will, it could return the original, unchanged, grid instance).

I'd suggest that you add .map method, which would return a new grid, just like .filter does:

const array = [1, 2, 3]
// array.map((item) => item * 2) // `.map` returns new instance, not `.forEach`
array.forEach((item) => item * 2).filter(item => item % 2 === 0) // throws because `.forEach` returns `undefined`

This simple change would make things much clearer for me.

I love immutability so the rest of the API would work well for me I believe.

flauwekeul commented 3 years ago

That's a good idea 😃 , however, there's a problem 😞

Because grid's are lazy, I need to store all the information (callbacks, traversers) passed to the methods and store that information in a grid instance. When run() is called, all that information is "applied". With the current "chain" API, there are some possibilities (the code is simplified for brevity):

  1. Current API: always return a new grid instance:

    const grid = new Grid()
    const grid2 = grid.each(...)
    grid.run() // each() isn't applied ❌
    grid2.run() // each() is applied ✅
  2. Make each() mutate the grid instance in-place:

    const grid = new Grid()
    const grid2 = grid.each(...) // 👈 modifies grid
    grid === grid2 // true
    grid.run() // each() is applied ✅
  3. Same as 1, but rename each() to map() to communicate a new instance is returned:

    const grid = new Grid()
    const grid2 = grid.map(...)
    grid.run() // map() isn't applied ❌
    grid2.run() // map() is applied ✅
    // note that it would be a bad idea to have an each() method return nothing:
    grid.each(...).run() // error: Cannot read property run of undefined

Do you have any other ideas? For now I think option 3 is the best.

Btw, note that this always works (regardless of whatever solution above we pick):

const grid = new Grid()
  .each(...)
  .filter(...)
  .traverse(...)
  .each(...)

// later:
grid.run()

The code below shows how people should ideally use the v4 API. I've used an example of a game with tanks that can shoot at each other on a grid map:

const grid = new Grid(hexPrototype, rectangle(...)) // or whatever initial state they want

class Tank {
  searchEnemy() {
    return grid
      .traverse(spiral({ start: this.coordinates, radius: this.range })) // spiral() is on the backlog
      .filter(inStore) // I've already added the inStore function btw
      .takeWhile((hex) => hex.hasEnemy) // assumes hex has a custom hasEnemy prop
      .toArray() // this is on the backlog too
  }

  canShoot(target: HexCoordinates) {
    return grid
      .traverse(between(this.coordinates, target)) // between() is on the backlog; it will have similar behaviour to v3's grid.hexesBetween()
      .takeWhile((hex) => !hex.blocked) // blocked would be true for hexes with walls or something
      .some((hex) => hex.equals(target)) // some() is on the backlog
  }
}

P.S. I've updated this comment several times, but I'm finished now 😅

vfonic commented 3 years ago

I'm on my phone, but yeah, Option 3, with a documented difference between forEach and each, is the best IMO.

Could you return the same grid when each is called and a new grid when map is called?

How can someone change the grid when each is called? Someone can modify hex objects? Well, isn't it the same for forEach? forEach disregards the changes. You can still call forEach and modify each of array's objects.

On Mon, Mar 29, 2021, 13:20 Abbe Keultjes @.***> wrote:

That's a good idea 😃 , however, there's a problem 😞

Because grid's are lazy, I need to store all the information (callbacks, traversers) passed to the methods and store that information in a grid instance. When run() is called, all that information is "applied". With the current "chain" API, there are some possibilities (the code is simplified for brevity):

  1. Current API: always return a new grid instance:

const grid = new Grid() const grid2 = grid.each(...) grid.run() // each() isn't applied ❌ grid2.run() // each() is applied ✅

  1. Make each() mutate the grid instance in-place:

const grid = new Grid() const grid2 = grid.each(...) // 👈 modifies grid grid === grid2 // true grid.run() // each() is applied ✅

  1. Same as 1, but rename each() to map() to communicate a new instance is returned:

const grid = new Grid() const grid2 = grid.map(...) grid.run() // map() isn't applied ❌ grid2.run() // map() is applied ✅ // note that it would be a bad idea to have an each() method return nothing: grid.each(...).run() // error: Cannot read property run of undefined

Do you have any other ideas? For now I think option 3 is the best.

Btw, note that this always works (regardless of whatever solution above we pick):

const grid = new Grid()

.each(...)

.filter(...)

.traverse(...)

.each(...)

// later: grid.run()

The code below shows how people should ideally use the v4 API. I've used an example of a game with tanks that can shoot at each other on a grid map:

const baseGrid = new Grid(hexPrototype, rectangle(...)) // or whatever initial state they want

class Tank {

hexesInRange() {

return baseGrid.traverse([

])

} }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flauwekeul/honeycomb/issues/66#issuecomment-809299275, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQO3MBZXKIPQ762TKTRPLTGBWBDANCNFSM4Z5652PQ .

flauwekeul commented 3 years ago

Option 3, with a documented difference between forEach and each, is the best IMO.

To be clear: I'm was planning (continue reading for my new plan 🤓) on renaming each() to map(). There won't be an each() nor a forEach().

Could you return the same grid when each is called[...]?

Not without mutating the grid.

Update: new plan! I'll keep each() the same as it's now (always returning a new grid instance) and add a map() (that also always returns a new grid instance). This map() is different from each() in that it's callback is called with a cloned hex so that users may mutate that hex however they want without mutating the hexes in the source grid. Like so:

// create a grid with a single hex
const grid = new Grid(hexPrototype, at({ q: 0, r: 0 }))

const eachGrid = grid.each((hex) => {
  // hex can still be mutated, but that's up to the user. This method should be used for side-effects
  hex.a = 'EACH'
})

const mapGrid = grid.map((hex) => {
  // hex can be mutated without the hex in the source grid being mutated
  hex.a = 'MAP'
  // it's not required to return the hex, the (mutated) clone is used if no hex is returned
})

eachGrid.run()
grid.getHex({ q: 0, r: 0 }) // { q: 0, r: 0, a: 'EACH' } // hex is mutated
eachGrid.getHex({ q: 0, r: 0 }) // { q: 0, r: 0, a: 'EACH' } // of course, eachGrid also has the mutated hex

mapGrid.run()
grid.getHex({ q: 0, r: 0 }) // { q: 0, r: 0, a: 'EACH' } // `a` still has the same value from calling each()
mapGrid.getHex({ q: 0, r: 0 }) // { q: 0, r: 0, a: 'MAP' } 👈 a is set to the expected value

You may wonder why I don't pass a cloned hex to the callbacks of the other methods (filter(), takeWhile(), etc) as well. That's because of performance. And I don't expect people to go mutating hexes all over the place, if they do, it's at their own peril.

flauwekeul commented 3 years ago

I've already made the planned changes from my previous comment 😎 . And added some more examples in the readme. I'll try to release the next alpha later today.

I'll close this issue. Thanks for making Honeycomb better by being my rubber duck đŸĻ†

vfonic commented 3 years ago

You're welcome! :)

Here's my point of view on keeping .each() and adding .map(). I think it's a good idea. I'd try to make it more familiar to people by making it as close as possible to Array's behavior. Pretty much everyone is familiar with Array's .filter(), .forEach() and .map() methods.

The way your new .map() method behaves is different from Array's .map() method. What if I want to create a new grid with hexes "cloned", but I have my own "cloning" function (maybe there's some magic happening in the constructor that I need to call)? What do I do now? 1) I can create a new grid and use .each() to manually add new hexes that I create off of existing hexes. - Doesn't sound ideal 2) I can use .map(), which seems like the right function to call, but now I know I'll have your code clone the hex and then I'll call something like hex = new MagicHex(hex), but it won't even work, because .map() ignores returned values and just creates a new grid out of cloned hexes. 🤷‍♂ī¸

I'll stop here. My point is, I never considered that .each() modifies the grid, just like how I'd think that .map()'s return value is what new grid is made out of...

flauwekeul commented 3 years ago

What if I want to create a new grid with hexes "cloned", but I have my own "cloning" function

That's exactly why it's possible to create your own hex prototype. You can make your own clone() method:

import { cloneHex } from 'honeycomb-grid'

const hexPrototype = createHexPrototype({
  clone(newProps) {
    // do your magic
    return cloneHex(this, newProps)
  }
})

That clone() method is called whenever Honeycomb is creating a new hex (even when hexes are created for the first time!). I haven't documented this yet, I'll add it soon.

Moving on, my code comment in the map() example from my previous comment says:

it's not required to return the hex, the (mutated) clone is used if no hex is returned

So if a hex is returned from the map() callback, that hex will be used. This provides another way to control how hexes are created/cloned.

vfonic commented 3 years ago

Ah, interesting! I missed the part where you can return but don't have to. Thanks for explanation!