All current tests pass but let me know if you want me to add a couple more to make sure

That's awesome, thank you for the PR! I left a few comments to make sure we aren't breaking something across maps by caching data globally. Could you also share the benchmark you used?

Could you also share the benchmark you used?

I used the benchmarkify library with the mock data from the first test in (Radius.test.tsx)

Could you also share the benchmark you used?

I used the benchmarkify library with the mock data from the first test in (Radius.test.tsx)

That's great. Can you also share the code so we can repro it?

That's great. Can you also share the code so we can repro it?

Absolutely, https://github.com/rortan134/athena-crisis/blob/radius-perf-bmark/athena/__tests__/RadiusBenchmark.test.tsx
Just run the test script

Thanks for the updates!

I realized that we can actually avoid all of the extra complexity and don't need to add an LRUCache. The radius cache is specific to a MapData instance, and MapData is immutable. It's just a cache to speed things up, so how about we just add the cache interface to MapData directly? Then it will be garbage collected automatically.

However, this led me to uncover a gap in this optimization. calculateRadius takes four inputs: A map, a unit, a starting Vector and the radius. All four of those can be different at any time, like for example we might want to get a Flamethrower's radius at the center of a desert map, or a Sniper's radius in a volcano map at the left top. However, we might also check different units at one position on the same map, and each unit has different movement costs on the same field, like for example an ocean might be inaccessible to a Flamethrower but a Fighter Jet can just fly over it. This means that caching on a per-map and per-field basis will not work. Let me know if you'd like me to add a bunch of test cases to verify these behaviors.

I see.

Let me know if you'd like me to add a bunch of test cases to verify these behaviors.

A couple test cases would be great

Thanks for the updates, I think we are getting closer to something that can be merged. As for the benchmark, it's not super representative to run it on the same inputs – if that was a common case we could just cache the outputs of the function completely! I added one new test on main that will fail on this branch.

Here is a more representative benchmark that calculates the radius for each unit on the map:

test('benchmark moveable radius', async () => {
  const testMap = startMap.copy({
    units: startMap.units.delete(vec(11, 8)),
  });
  
  const benchmark = new Benchmark('benchmark moveable').printHeader();
  benchmark
    .createSuite('starting benchmark', { time: 10_000 })
    .add('moveable', () => {
      for (const [vector, unit] of testMap.units) {
        moveable(testMap, unit, vector, 7)
      }
    })
    .add('fast moveable', () => {
      for (const [vector, unit] of testMap.units) {
        fastMoveable(testMap, unit, vector, 7)
      }
    })
  await benchmark.run();
});

It might be worth randomizing the order at the beginning of the run, but this should be a good start.

I added the benchmarks right to the repo using pnpm vitest bench: 5d3bc85

Feel free to extend them as well.

@rortan134 are you planning on getting back to this?

Closing due to inactivity. Happy to reopen if it's being picked up again.