Zerg ResourceDepots refactor/training functionality, MobileUnit build time, BWEM initalization

[kdlin10]

Hatchery, Lair, and Hive now inherit from an abstract ZergResourceDepotImpl and can train units. (I know some people dislike exploding number of classes, but thankfully the game isn't going to change much so the scope for growth is limited). Player class canMake() returns true if checking a Hatchery can train something that morphs from a larva (Does not check amount of larva).

MobileUnit can be queried for their remaining build time; returns "Number of frames remaining until the current training unit becomes completed, or the number of frames remaining until the next larva spawns."

BWEM now assigns starting locations as part of initialization

[adakitesystems]

Overall, I like the changes. However, there are some unrelated changes. It seems the main theme of this PR is to introduce ZergResourceDepot. But there is a canMake and BWEM change without any justification.

[adakitesystems]

Since builder is a Unit, you can get rid of the three checks for "is a hatchery or is a lair or is a hive" and just write builder instanceof ZergResourceDepot which reduces the amount of code. Also, what is the purpose of checking whatBuilds for UnitType.Zerg_Larva? Can we not omit that check?

[kdlin10]

The check is intended to allow the case where type builds from Zerg_Larva and the builder is a ZergResourceDepot to pass through without returning false. Removing that check would allow all cases where builder is a ZergResourceDepot to go through without returning false.

Thought I'd keep the checks against UnitType since that seemed to be the style - will revise to instanceof ZergResourceDepot

[adakitesystems]

If I understand correctly, you're saying that self.canMake(myHatchery, UnitType.Zerg_Hydralisk will return true with your change? Then, I assume all other production facilities will need to be considered as well for cases such as self.canMake(myFactory, UnitType.Terran_Goliath)?

Edit: Nevermind that. It probably already returns true.

[kdlin10]

Yes - the conditional is intended to filter out any invalid builder / type combos. Everything else passes through fine, but Zerg MobileUnits are an exception because they are paired with Zerg_Larva and would get filtered out when combined with a ZergResourceDepot

[adakitesystems]

Instead of implementing ResourceDepot, it should implement ZergResourceDepot where ZergResourceDepot is an interface that extends ResourceDepot. Then, users can write unit instanceof ZergResourceDepot instead of instance of ZergResourceDepotImpl.

[kdlin10]

I see your point and can add an interface, but if one wanted to implement shared methods common to Hatchery, Lair, Hive, they'd have to write them separately (assuming you don't keep Hive extending a Lair, Lair extending a Hatchery) or have an abstract class to inherit from, which is what's currently proposed. I think the only functionality really unique to ZergResourceDepot would be getLarva() and researchBurrowing().

[adakitesystems]

There seems to be only formatting changes with this file. This formatting does not match the rest of the codebase.

[kdlin10]

isReadyForResources() was removed and Lair now extends from a ZergResourceDepotImpl abstract. I believe not having Lair extend Hatchery better reflects the game. For example, somebody might want to check if there is a Hatchery to research Burrow and use their Lair for other upgrades - previously, checking instanceof Hatchery on our `Lair would return true.

[adakitesystems]

Yes, sorry my mistake.

[Bytekeeper]

I don't agree here. I'm actually using the Lair is a Hatch mechanic in my bot, as it makes it easier to check if I already have a Lair although it is updated to a Hive already.
As we discussed on Discord, both ways have their pros and contras.

Currently you'll have to check if you supposed Hatchery is actually a Lair or a Hive.
After the change, you'd have to check if you've got a Lair or a Hive. You only move the check to another position.

Unless there's an additional benefit, this is just toggling code back and forth.

[kdlin10]

The proposed scheme would have 4 different types of checks that could be accomplished with a single comparison (If something is a Hatchery, Lair, Hive, or all three) - no second check necessary. If one wants to select two types, that can be accomplished with a second comparison.

With the current arrangement, with a single check, one can filter a Unit down to all a Hatchery || Lair || Hive, a Lair || Hive, or a Hive. Further necking down requires a second check.

More importantly, I think it is not intuitive to check if a Unit is a Lair and have it return true for a Hive. When I ask if a Unit is a Hatchery, it's not obvious at all Lair and Hive qualify as a Hatchery

[Bytekeeper]

We're repeating our discord chat :)
This is not at all an objective argument we're having here. It's just "taste".
I for example find it very intuitive that a Hive isa Lair- it's the same way it is presented to a player in-game.

There's no objective benefit doing it either way. And I believe every change breaking API should have one.

[adakitesystems]

This function will throw an exception if it fails to match suitable base locations to all starting locations. Lots of maps play around with start locations and observer player starting locations. So, this change breaks more than it fixes, IMO. If the intent of this change is so the user is not required to manually invoke this function during the bot's initialization stages, the thrown exception needs to be removed or dealt with. As something to consider, BWEM generates a list of starting locations not assigned to bases if the function fails.

[kdlin10]

Understood, I'll withdraw this proposal. Was just struck by how user-unfriendly it was to have an initialize() then have to manually do something that seems like core functionality, not an option.

[Bytekeeper]

As you mentioned I'm not really happy about this approach: This is the first of many many interfaces:
TerranResourceDepot, ZergFlyer, ProtossSpellCaster, ZergSpellCaster, TerranSpellCaster.
Since Hive extends Lair extends Hatchery - you can already just do a instanceof Hatchery. What does the new interface improve?

(canMake is another issue (#53) and is ambiguous anyways.)

[adakitesystems]

I agree that TerranResourceDepot, ZergFlyer, ProtossSpellCaster etc. could be too much. However, since all of the Zerg resource depots are almost the same, I like the ZergResourceDepot. I also agree instanceof Hatchery works well too but I would argue this check is slightly disingenuous if the user merely wants to check if the building/unit is resource depot. I, personally, consider it a hackish type of check. The code instanceof Hatchery insinuates the author wants to check for Hatchery but the actual underlying purpose of instanceof Hatchery could be to check whether it is just a generic resource depot.

Edit: I guess the user should use instanceof ResourceDepot in that case then? Hmm.

This might be a somewhat related or unrelated question: can a worker return resources to an infested Command Center? If so, it would also be considered a ZergResourceDepot, no?

[kdlin10]

I'm also wary of combinatoric explosion of interfaces, but happily, game mechanics are set so the potential for growth there is limited. I would argue that the interfaces should be limited to things that provide unique functionality, so something like ZergFlyer or ProtossSpellCaster wouldn't qualify, since the only thing flying Zerg units share is that they fly and are Zerg. ZergResourceDepot however, share larva spawning and selecting and passing train commands, so it's a meaningful distinction.

And I don't believe resources can be returned to infested CCs