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

Fix: Apply PlayerAttachment stack limits when placing units. #11826

Merged
merged 15 commits into from
Jul 29, 2023

Conversation

asvitkine
Copy link
Contributor

@asvitkine asvitkine commented Jul 26, 2023

Change Summary & Additional Notes

With this change, these restrictions are checked before showing the dialog of which units to place in a territory.
This way, invalid placements will be omitted from the dialog already, rather than showing up and causing an error when selected.

Tested on the "1941 Global Command Decision" map, where all the factory types have a stacking limit of 1 per territory. Before the change, if you build them and try to place units in a territory with a factory already, the factory would show up in the dialog. With the change, it will be omitted. (On this map, the presence of the factory in the list of units to place would also show an increased max for how many units can be placed for non-factory units - e.g. being able to select 3 infantry to place in a territory that only allows 2.)

This change refactors the code to move the stacking limits logic to a new UnitStackingLimitFilter class, combining the logic that was previously split between two places behind a better API that allows simplification of all the call sites.
Additional test coverage is added, both for the new class and for the PlaceDelegate, the latter also covering the sequence of the different calls (i.e. that placement restrictions are checked before stacking limit filtering).

Additionally, one call site no longer needed to do the checks as it was already doing the validation indirectly through another function it was calling.

The cleanup removes several hundred lines of code, although many lines of test code are also added,

Release Note

FIX|Unit placement dialog will now filter out units that can't be placed due to stacking limits coming from the PlayerAttachment.

@asvitkine asvitkine marked this pull request as draft July 27, 2023 03:33
@asvitkine asvitkine marked this pull request as ready for review July 29, 2023 02:41
@asvitkine asvitkine merged commit 00d298a into triplea-game:master Jul 29, 2023
1 check passed
@TheDog-GH
Copy link
Contributor

TheDog-GH commented Jul 29, 2023

@asvitkine
I dont think this working as intended, see below 35 Fighters (max should be 10) & Infantry (max should be 20)
image

image

14 Convoy (max should be 10)
image

@asvitkine
Copy link
Contributor Author

@TheDog-GH thanks for the quick testing.

Will take a look.

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