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

#3959 Refactored stockpile limits into unit custom params #3994

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ryanbennitt
Copy link

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

@sprunk
Copy link
Collaborator

sprunk commented Dec 4, 2024

@ryanbennitt
Copy link
Author

Refactored these too

luarules/gadgets/unit_stockpile_limit.lua Outdated Show resolved Hide resolved
Copy link
Member

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

Copy link
Author

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.

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?

Copy link
Author

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.

Copy link
Collaborator

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

Copy link
Member

@WatchTheFort WatchTheFort Dec 5, 2024

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.

Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

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.

3 participants