ricochet1k / screeps-client

Custom client for Screeps using PIXI.js
51 stars 13 forks source link

Optimized neighbours function #17

Closed Greg-Tarr closed 1 year ago

Greg-Tarr commented 5 years ago

Reduced calculations required for neighbours()

RiftLurker commented 5 years ago

I personally think it's less readable with those changes, though I'm not working on this project. ;-)

Did you profile the code? I doubt there is much of a performance boost to be gained with those changes.

Nonetheless I'd probably go for an optional range parameter that defaults to 1 (or just inline it to a constant if it's unwanted) and implement the function like this:

export function neighbors(x, y, f, range = 1) {
  const ns = [];

  for (let dx = -range; dx <= range; dx = dx + 1) {
    for (let dy = -range; dy <= range; dy = dy + 1) {
      if (dx === 0 && dy === 0) {
        continue;
      }
      ns.push(f(x + dx, y + dy));
    }
  }

  return ns;
}
Greg-Tarr commented 5 years ago

@PostCrafter having to check if (dx === 0 && dy === 0) every loop is expensive, rather just remove afterwards like so:

export function neighbors(x, y, f, range = 1) {
  const ns = [];

  for (let dx = -range; dx <= range; dx = dx + 1) {
    for (let dy = -range; dy <= range; dy = dy + 1) {
      ns.push(f(x + dx, y + dy));
    }
  }
  ns.splice(Math.floor(ns.length / 2), 1);
  return ns;
}

This removed the center of the array. Saving the if() checks and therefore computation speed. This is my first time realizing this approach so I'm not certain it is worthwhile.

RiftLurker commented 5 years ago

Your approach might be worthwhile for large for-loops (high value range). But since the parameter is mostly 1 the splice is probably more expensive.

That being said I don't think decisions should be made without proper profiling as premature optimization is the root of all evil.

ricochet1k commented 5 years ago

Wait, is this slow? Or a bottleneck somehow? I haven't worked on this project in a while, but I don't remember terrain rendering ever being slower than effectively instant. I'd be extremely surprised if V8 doesn't optimize this into something better than what you wrote anyway.

What's the point of adding a range parameter? This particular function is only used for drawing fancy curves at the corners of terrain walls, and I'm struggling to imagine a case where terrain is ever going to care about something 2 squares away.