Perf/refactor: use StructureTypes, remove territoryBound#3238
Perf/refactor: use StructureTypes, remove territoryBound#3238DevelopingTom merged 1 commit intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThis refactoring removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
## Description: PR 7/x in effort to break up PR #3220. Follows on already merged #3238. Please see if these can be merged for v30. -**RadialMenuElements**: - _getAllEnabledUnits_: use camelCase instead of PascalCase so change Units into units. - _getAllEnabledUnits_: use StructureTypes to loop through instead of having 6 individual lines of code to check if a structure is enabled. StructureTypes contains and will keep containing the same structures as those that are checked here. PR 3220 will later on also replace the individual lines for the attack type units into a loop with a newly introduced Types array, in the same way as we do in this PR with StructureTypes. - _getAllEnabledUnits_: rename the long named const addStructureIfEnabled to just addIfEnabled, which is clear enough from the context it is used in. -**PlayerImpl** - _buildableUnits_: removed unnecessary "as BuildableUnit" after the in-loop return; the function itself already says it returns BuildableUnit[]. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
Description:
PR 6/x in effort to break up PR #3220. Follows on already merged #3237. Precedes #3239.
Please see if these can be merged for v30.
PlayerImpl: validStructureSpawnTiles did a filter on unit types to get isTerroritoryBound units, on every call again. It read this from unit info in DefaultConfig. While having it centrally in DefaultConfig unitInfo is good for maintainability, other code uses hardcoded StructureTypes and isisStructureType from Game.ts. Which has the same purpose and thus contains the same unit types. StructureTypes and isisStructureType do need manual maintainance outside of DefaultConfig. And are more bug prone/less type safe. But, using them gives more speed compared to getting these unit types out of DefaultConfig unitInfo centrally with some cached function in GameImpl for example (tested with buildableUnits and MIRVPerf.ts). So I went with StructureTypes in validStructureSpawnTiles too.
PlayerExecution: now validStructureSpawnTiles no longer needs isTerritoryBound (see the point above), PlayerExecution is the last place where it was used. Replaced it for isStructureType here too (since it has the same meaning and outcome).
Game.ts and DefaultConfig unitInfo: remove the now unused territoryBound. As it was only used in validStructureSpawnTiles and PlayerExecution and has been replaced in both (see the two points above).
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33