Reduce code duplication#233
Merged
Merged
Conversation
We have a lot of places in the code that calculate direction based on center cell coordinates, but lack a utility function for it similar to other direction methods.
Use ConvertGridNoToCenterCellXY instead
majcosta
approved these changes
Oct 5, 2023
| return(atan8( sXPos2, sYPos2, sXPos, sYPos )); | ||
| } | ||
|
|
||
| INT16 GetDirectionFromCenterCellXYGridNo(INT32 sGridNoDest, INT32 sGridNoSrc) |
| if ( WillExplosiveWeaponFail( pSoldier, pObjHand ) ) | ||
| { | ||
| INT16 sX, sY; | ||
| ConvertGridNoToCenterCellXY(pSoldier->sGridNo, &sX, &sY); |
Contributor
There was a problem hiding this comment.
Having this next to sX and sY declaration is more readable, but are we sure none of the functions between here and the old CenterX/Y calls have any side-effects on pSoldier-sGridNo? (ditto for similar situations below)
Contributor
Author
There was a problem hiding this comment.
IgniteExplosion and AddItemToPool have INT32 sGridNo as a parameter and at that point the functions have their own private copies of pSoldier->sGridNo instead of a reference, right?
Other functions, at least in this case do not use pSoldier->sGridNo at all, but good point. I should probably check if an unintended change can happen elsewhere where I did a similar thing.
ConvertGridNoToCenterCellXY and ConvertMapPosToWorldTileCenter do the exact same thing
Both functions calculate the same thing
The values end up being squared anyways making these unnecessary
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This came about when I was looking at display cover functionality, again, and noticed that in the hot loop, we were using functions CenterX & CenterY to calculate coordinates for X and Y separately, duplicating work.
While going through the code for references to these functions, also noticed a lack of a utility function for calculating direction based on center coordinates, even though there was a clear need for one.
I'd like to chip away at the stupid amount of duplicate code we have and maybe this could serve as a starting point?