diasurgical / devilutionX

Diablo build for modern operating systems
Other
7.98k stars 780 forks source link

Refactor missiles.cpp to use Direction enum instead of magic Point/Displacement constants #2260

Open ephphatha opened 3 years ago

ephphatha commented 3 years ago

There are multiple places in missiles.cpp which iterate through tiles adjacent to a given location.

https://github.com/diasurgical/devilutionX/blob/5537fe748e63b06ed2e474f7dea6ebd1216fa3d3/Source/missiles.cpp#L1634 https://github.com/diasurgical/devilutionX/blob/5537fe748e63b06ed2e474f7dea6ebd1216fa3d3/Source/missiles.cpp#L3417 https://github.com/diasurgical/devilutionX/blob/5537fe748e63b06ed2e474f7dea6ebd1216fa3d3/Source/missiles.cpp#L3995 https://github.com/diasurgical/devilutionX/blob/5537fe748e63b06ed2e474f7dea6ebd1216fa3d3/Source/missiles.cpp#L4017 https://github.com/diasurgical/devilutionX/blob/5537fe748e63b06ed2e474f7dea6ebd1216fa3d3/Source/missiles.cpp#L4772

These lists could be directly replaced with a list of the appropriate Direction enum constants. This would retain the same behaviour as existing code while making the intent clearer.

ephphatha commented 3 years ago

There is similar code in player.cpp and towners.cpp (probably plenty of other places). If you were feeling keen and wanted to clean up other files I'd recommend limiting PRs to a single file per commit to make it easier to review 😃

agris-codes commented 3 years ago

Does “good first issue” mean a good issue for a newcomer to the project to tackle, rather than a good writeup of the issue by the poster?

AJenbo commented 3 years ago

Does “good first issue” mean a good issue for a newcomer to the project to tackle, rather than a good writeup of the issue by the poster?

yes https://goodfirstissue.dev/language/cplusplus

agris-codes commented 3 years ago

Does “good first issue” mean a good issue for a newcomer to the project to tackle, rather than a good writeup of the issue by the poster?

yes https://goodfirstissue.dev/language/cplusplus

Thanks, I’ll tackle this issue then if the turn-around time required isn’t < 1 week.

julealgon commented 3 years ago

I imagine some instances could just turn into:

for (auto direction in enum_values<Direction>())
    ...

?

ephphatha commented 3 years ago

Definitely for the lines linked, they only end up being used for CheckMissileCollision and all cases get executed (no early returns) so directions don't need to be tested in a specific order. I'm not sure if other code depends on the list order.

nathanafox-us commented 2 years ago

Hello. I believe I edited the code in missiles.cpp to use the enum directions in those places rather than the magic numbers. I would like to request a push, but this is my first time contributing to a project. How would I go about doing it in this instance?

AJenbo commented 2 years ago

Well actually you create a Pull Request, which we then have to approve. First push your code to your fork on GitHub (Sergentfox234/devilutionX). You then go to https://github.com/diasurgical/devilutionX/pulls where you should see a green button for creating your Pull Request.

nathanafox-us commented 2 years ago

Ok. I pushed to the pull request.

nathanafox-us commented 2 years ago

Ok. It should be good to go now!

AJenbo commented 2 years ago

Thanks, I made a review and added some comments as feedback.