search

Blizzard / s2client-api

StarCraft II Client - C++ library supported on Windows, Linux and Mac designed for building scripted bots and research using the SC2API.
MIT License
1.66k stars 281 forks source link

Terran upgrade: RESEARCH_MAGFIELDLAUNCHERS #230

Open DonThompson opened 6 years ago

DonThompson commented 6 years ago

RESEARCH_MAGFIELDLAUNCHERS is in the ABILITY_ID list as ID 766. This appears to be an older upgrade that is no longer in the current game. However, ID 766 is actually now RESEARCH_SMARTSERVOS, an entirely different upgrade.

I am unclear if this is a documentation error (the ID was never 766 and someone input it wrong), or if the actual game client actually re-used the ID and changed it when they implemented the "Smart Servos" upgrade. I do not know how to find this answer.

I added RESEARCH_SMARTSERVOS in PR https://github.com/Blizzard/s2client-api/issues/228 but left RESEARCH_MAGFIELDLAUNCHERS with the same ID so as not to break anyone who had used that ID in their code.

KevinCalderone commented 6 years ago

I think there is a bug in the game that can cause IDs of research abilities to get reused.

I believe MAGFIELDLAUNCHERS was removed from the game before any public version of the API was released, so probably no one is depending on this enum value?

Also in C++ I believe you can have multi enum entries that have the same value. Having both there may be the safest option.

DonThompson commented 6 years ago

It is safe to repeat an enum definition, however you can't use it in a case statement... so (after PR 228 gets merged)

switch(abilityID) {
case RESEARCH_SMARTSERVOS:  /766
  break;
case RESEARCH_MAGFIELDLAUNCHERS:  //766
  break;
}

would result in a compiler error. This probably isn't the end of the world, but does require all future bot builders figure this out on their own if they're trying to build out mappings.

2 alternatives: 1) Remove RESEARCH_MAGFIELDLAUNCHERS entirely. Would break anyone who has pulled and used that upgrade ID already. 2) change the value from 766 to INVALID? Seems like this would be somewhat self-documenting that it's no longer a valid upgrade, work in any switch statements it happened to be in (at least until another INVALID upgrade is found), and would not break any existing code.

N00byEdge commented 6 years ago

If we would have to choose between those 2, I'd say that removing the value entirely is the way to go. If a bot uses that somewhere in its logic and it no longer is available, it should probably be removed.