smogon / pokemon-showdown

Pokémon battle simulator.
https://pokemonshowdown.com
MIT License
4.72k stars 2.76k forks source link

ActiveMove is problematic #5415

Open scheibo opened 5 years ago

scheibo commented 5 years ago

5389 made ActiveMove a class, which brought some concerns:

Hm I don't think I like the idea of ActiveMove being its own class. Especially since it doesn't actually extend Move, it actually makes move properties non-readonly.

_Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5389#discussion_r271868604_

and importantly also caused some crashes. It also presents some challenges for serialization in #5270, as I need to include a _class_ tag:

  "lastMove": {
    "_class_": "ActiveMove",
    "move": "[Move:willowisp]",
    "hit": 1,
    "moveHitData": {
      "crit": {},
      "typeMod": {},
      "zBrokeProtect": {}
    },
    "totalDamage": 0
  }

However, I'm not sure the previous approach (before it was made it a class) was any better, as we'd often simply cast arbitrary objects to /** @type {ActiveMove} */, and simply cloning + adding crap to a Data.Move is not ideal for performance, as thanks to the monomorphism optimizations we would ideally be assigning everything on creation in the literal/constructor. From a serialization point of view, detecting an ActiveMove without the constructor becomes more difficult (looking for a hit field or similar is somewhat error prone, so I'd probably end up not being able to collapse the attributes cloned from Move).

interface ActiveMove {
  move: Data.Move;
  activeCrap: ...
}

To be clear, I think 'simplicity' and 'readability' trump 'serialization/performance concerns', but even with the old approach we played fast and loose with casts and I don't think the ActiveMove concept was optimal.

Zarel commented 5 years ago

The previous approach is at least not actively buggy. :/

scheibo commented 5 years ago

Yeah, I should mention that fixing the crashes should take precedence over trying to decide what the ideal long term solution should be.