(Improvement) Add additional calculation methods to RoomXY

[jciskey]

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

[khoover]

Strongly disagree with these impls existing, considering even in-game objects disagree on being row or column major (which this implicitly enforces).

[jciskey]

This was copied over from Position, purely because it allows for usage as a key in things like BTreeMap and that seems like a reasonable thing to keep parity with. If we want to tear it out here, we should tear it out there as well.

[khoover]

Honestly, yes. Position is even worse, since on top of picking row/column major, you also have to decide if you're doing a lex order on the (room name, room xy) pair or treating the whole position as a single world xy.

[khoover]

For position, there's really 6 different ways you could arrange the components into a 4-tuple and use the lex ordering to come up with a sensible Position ordering:

• (room_name.x, room_xy.x, room_name.y, room_xy.y)
• (room_name.y, room_xy.y, room_name.x, room_xy.x) <- the one Position implements
• (room_name.x, room_name.y, room_xy.x, room_xy.y)
• (room_name.x, room_name.y, room_xy.y, room_xy.x)
• (room_name.y, room_name.x, room_xy.x, room_xy.y)
• (room_name.y, room_name.x, room_xy.y, room_xy.x)

[jciskey]

I can tear this out if we really want to, and you can open up a PR for tearing out the Position implementation, but I'm not comfortable doing the latter in this PR. It feels out of scope for the intended goal of bringing RoomXY into approximate API parity with Position.

[khoover]

This is a bit of a weird one; wouldn't you want a float in [0,1] for self*x + target*(1-x)?

[jciskey]

I'm not entirely sure what you're asking here. Can you clarify a bit more?

[khoover]

It seems like this is supposed to be some sort of interpolation method, where you get a point on the line segment between self and target.

[jciskey]

Ah, yes, this is a directional interpolation method. I'm not sure that returning the interpolation multiplier (or even vector) is all that helpful, though, since the user would still need to determine what RoomXY corresponds to that interpolation multiplier.

As with PartialOrd, this was copied from Position for API parity.

[khoover]

Left replies in discussions

[shanemadden]

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]

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]

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]

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]

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.