-
Notifications
You must be signed in to change notification settings - Fork 303
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
#3959 Refactored stockpile limits into unit custom params #3994
base: master
Are you sure you want to change the base?
#3959 Refactored stockpile limits into unit custom params #3994
Conversation
Refactored these too |
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.
Even though it's unchanged, the if statement on lines 46-50 can be fixed.
if ud.customParams and ud.customParams.stockpileLimit then
isStockpilingUnit[udid] = tonumber(ud.customParams.stockpileLimit)
elseif ud.customParams and ud.customParams.stockpilelimit then
isStockpilingUnit[udid] = tonumber(ud.customParams.stockpilelimit)
end
Both branches of the if statement are identical. As well, customParams
is guaranteed not to be nil, so no nil check is required.
if ud.customParams.stockpileLimit then
isStockpilingUnit[udid] = tonumber(ud.customParams.stockpileLimit)
end
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.
Even though it's unchanged, the if statement on lines 46-50 can be fixed.
if ud.customParams and ud.customParams.stockpileLimit then isStockpilingUnit[udid] = tonumber(ud.customParams.stockpileLimit) elseif ud.customParams and ud.customParams.stockpilelimit then isStockpilingUnit[udid] = tonumber(ud.customParams.stockpilelimit) endBoth branches of the if statement are identical. As well,
customParams
is guaranteed not to be nil, so no nil check is required.
Alas the case is not identical in both branches, the L/l is upper case/lower case. There are some uses in code of the lower case which I could refactor, but any external use of this outside of the repo will not be updated. Is there a deprecation process or should I just proceed?
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's some scavenger code removed, I'm assuming when scavenger units are created that they copy all the customParams, so they'll have the stockpile limits already.
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.
Yea scavs copy customparams. Making customparams have consistent case sounds like a separate task, imo keep PRs small and focused
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.
But the two conditions are identical, only the first branch will ever run, the elseif
is no-op.
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.
no they aren't, the second one checks for lowercase
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.
Work done
Refactored stockpile limits into each unit's custom params section instead.
Some values remain as cannot find these unit defs here, are they stored elsewhere or are they vestigial and can be removed?
Addresses Issue(s)
Move default stockpile limits to customparams #3959
#3959
Setup
Just a regular game, some units changed require evolved commanders, legion, raptors
Test steps
Construct any unit containing a limited stockpile of e.g. missiles, confirm that it stops building when its stockpile limit is reached