Skip to content
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

Fix Issue #45 "Reapers don't require tech lab anymore" #46

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

ImpulseCloud
Copy link
Contributor

Removes REAPER from BARRACKSTECHLAB case in API.cpp's tech_requirement workaround.
Tested building Reapers without a BarrackTechLab and it works now.

@@ -209,7 +209,6 @@ sc2::UnitTypeData Observer::GetUnitTypeData(sc2::UNIT_TYPEID id_) const {
break;

case sc2::UNIT_TYPEID::TERRAN_MARAUDER:
case sc2::UNIT_TYPEID::TERRAN_REAPER:
data.tech_requirement = sc2::UNIT_TYPEID::TERRAN_BARRACKSTECHLAB;
Copy link
Owner

@alkurbatov alkurbatov Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this patch is not enough.
You should move the reaper ability from here

case sc2::ABILITY_ID::TRAIN_REAPER:

to here:
case sc2::ABILITY_ID::TRAIN_MARINE:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been done in commit 250f56d and I've reverted the #43 commit, so hopefully this should be good to accept now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
Currently we have a lot of intermediate changes in the history. Please squash the commits and I'll merge the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squashed and force-pushed. :)

point.x = base.x + sc2::GetRandomScalar() * 15.0f;
point.y = base.y + sc2::GetRandomScalar() * 15.0f;

offset.x = sc2::GetRandomScalar() * range; //build in range of StartLoc
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to try the approach used in CommandCenter.
The mineral line in the starting location is enclosed into a rectangle, if a building point is in the rectangle, different value is used. However, such approach requires careful detection of mineral lines which could be done in

struct Expansion {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we can use the placement grid abstraction from the api and mark the related tiles as occupied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look at that. I've reverted that commit, so the Reaper fix can be accepted while I work on this.

@alkurbatov alkurbatov merged commit 292d4c9 into alkurbatov:master Mar 1, 2021
@alkurbatov
Copy link
Owner

Thank you! LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants