-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Zerg ResourceDepots refactor/training functionality, MobileUnit build time, BWEM initalization #63
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!canMake(type) || !builder.isA(type.whatBuilds().getFirst())) { | ||
return false; | ||
} | ||
if (!canMake(type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
import org.openbw.bwapi4j.type.UpgradeType; | ||
import org.openbw.bwapi4j.unit.UnitImpl.TrainingSlot; | ||
|
||
public abstract class ZergResourceDepotImpl extends BuildingImpl implements Organic, ResourceDepot, ResearchingFacility, Morphable, TrainingFacility { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
import static org.openbw.bwapi4j.type.UnitCommandType.Morph; | ||
import static org.openbw.bwapi4j.type.UnitType.Zerg_Hive; | ||
|
||
import org.openbw.bwapi4j.type.UnitType; | ||
import org.openbw.bwapi4j.type.UpgradeType; | ||
public class Lair extends ZergResourceDepotImpl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be only formatting changes with this file. This formatting does not match the rest of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry my mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -155,7 +155,13 @@ public void initialize(final boolean enableTimer) { | |||
logger.info("Map::createBases: " + timer.elapsedMilliseconds() + " ms"); | |||
timer.reset(); | |||
} | |||
|
|||
|
|||
assignStartingLocationsToSuitableBases(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
As you mentioned I'm not really happy about this approach: This is the first of many many interfaces: ( |
I agree that Edit: I guess the user should use 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? |
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 And I don't believe resources can be returned to infested CCs |
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