rustyscreeps / screeps-game-api

Typed bindings to the Screeps in-game API for WASM Rust AIs
MIT License
138 stars 44 forks source link

(Improvement) Add additional calculation methods to RoomXY #521

Closed jciskey closed 2 months ago

jciskey commented 3 months ago

This PR adds calculation methods to RoomXY that give it approximate parity to Position.

shanemadden commented 2 months ago

Back from a trip and catching up on this - the changes look good to me, though it sounds like whether to implement Ord and PartialOrd is a point of contention - or I should say still a point of contention, as the discussion around implementing it for Position and RoomName was contentious before my time in #226. My inclination is to say that since those game types have Ord based on the outcome of that discussion then RoomXY might as well, until/unless we revisit that decision to have Ord on those?

jciskey commented 2 months ago

That's pretty much where I'm sitting for the Ord/PartialOrd dispute. If Position has it, RoomXY should have it. If we want to ditch it entirely, that's a separate discussion.

khoover commented 2 months ago

If we're going for consistency, then sure (although I'd also check that we used the same of column or row major here as we do for Position, away from an actual computer right now). I do think we'd be better served with something like pub struct ColumnMajor(pub RoomXY/Position/RoomName); pub struct RowMajor... instead of forcing a default choice.

EDIT: I do think you can have a default PartialOrd for RoomXY. It'd look something like this

impl PartialOrd for RoomXY {
  fn partial_cmp(&self, other: &Self) -> Optional<Ordering> {
    match (self.x.cmp(&other.x), self.y.cmp(&other.y)) {
      (ord, Equal) | (Equal, ord) => Some(ord),
      (a, b) if a == b => Some(a),
      _ => None
    }

    // To make the point clearer, and actually be more efficient
    // according to godbolt:
    let row_major = RowMajor(*self).cmp(&RowMajor(*other));
    let col_major = ColMajor(*self).cmp(&ColMajor(*other));
    (row_major == col_major).then_some(row_major)
  }
}

This partial ordering would be extendible into either row or column major ordering, i.e. if a.partial_cmp(&b).is_some() then RowMajor(a).cmp(&RowMajor(b)) == ColMajor(a).cmp(&ColMajor(b)) == a.partial_cmp(&b).unwrap().

jciskey commented 2 months ago

I'd also check that we used the same of column or row major here as we do for Position

It's the same ordering. y gets checked first, then x.

https://github.com/rustyscreeps/screeps-game-api/blob/main/src/local/position.rs#L376

shanemadden commented 2 months ago

Got #523 opened for switching over to a struct-wrapped implementation for all of these ordered types when someone has a chance to implement that.