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

Resolves #73 #82

Merged
merged 11 commits into from
Jul 17, 2024
Merged

Resolves #73 #82

merged 11 commits into from
Jul 17, 2024

Conversation

RickFqt
Copy link

@RickFqt RickFqt commented Mar 19, 2024

Resolves #73. When complete, this pull request should have:

  • Implementation of ProtossState, TerranState, and ZergState classes.
  • Implementation of unit tests for these classes.

@RickFqt
Copy link
Author

RickFqt commented Mar 19, 2024

@alvarofpp For the implementation of TerranState class, I used Simple64GridState class from the original sc2 state classes from urnai as a base. It basically stores information from the player (number of units, barracks and minerals, for example), and also builds a 4x4 grid that stores the location of enemy units. Do you think there is a better choice? I considered choosing UnitStackingState as a base since it is pretty well commented, but it seemed to me that it is too specific for a general purpose class.

Also, I implemented the methods get_my_units_amount and get_units_by_type at the end of the file. Is that right?

urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
@alvarofpp
Copy link
Member

@RickFqt Please mark as solved the modifications that you have already solved.

@RickFqt RickFqt marked this pull request as ready for review July 2, 2024 17:41
Copy link
Member

@alvarofpp alvarofpp left a comment

Choose a reason for hiding this comment

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

Many parts of the code have "magic numbers" (values without an explicit definition). I listed a few in the review, but I think you should create a file with constants to map them all.

urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
@RickFqt
Copy link
Author

RickFqt commented Jul 5, 2024

Many parts of the code have "magic numbers" (values without an explicit definition). I listed a few in the review, but I think you should create a file with constants to map them all.

Should this new file be on the path sc2/states/ as well?

@alvarofpp
Copy link
Member

Should this new file be on the path sc2/states/ as well?

I don't think so. Constants are generally used in many parts of the project, so it is recommended to apply them to a single file or use Enum to give them more context.

@RickFqt
Copy link
Author

RickFqt commented Jul 8, 2024

I don't think so. Constants are generally used in many parts of the project, so it is recommended to apply them to a single file or use Enum to give them more context.

In that case, is the path urnai/constants.py a good choice? On the previous version, it was located on urnai/utils/constants.py, but I think we are not using utils anymore, right?

@alvarofpp
Copy link
Member

In that case, is the path urnai/constants.py a good choice?

Yes.

urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
urnai/sc2/states/states_utils.py Outdated Show resolved Hide resolved
tests/units/sc2/states/test_aux_methods_sc2_state.py Outdated Show resolved Hide resolved
tests/units/sc2/states/test_sc2_state.py Outdated Show resolved Hide resolved
Copy link
Member

@alvarofpp alvarofpp left a comment

Choose a reason for hiding this comment

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

The ProstossState, TerranState and ZergState classes have a lot of repeated code. I think it would be better to create a StarCraft2State class with this repeated code and have the race classes inherit from it.

urnai/sc2/states/terran_state.py Outdated Show resolved Hide resolved
@alvarofpp alvarofpp merged commit 42e5ce7 into v2 Jul 17, 2024
1 check passed
@alvarofpp alvarofpp deleted the issue-73 branch July 17, 2024 12:39
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