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

Add a biome selector to the map generation dialog #3085

Merged
merged 8 commits into from Jan 4, 2024
Merged

Conversation

BlackYps
Copy link
Collaborator

Requires FAForever/Neroxis-Map-Generator#382 to be merged and released to actually be able to load the biomes list

grafik

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (2b15d9b) 59.02% compared to head (9805ecb) 59.10%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #3085      +/-   ##
=============================================
+ Coverage      59.02%   59.10%   +0.08%     
- Complexity      4408     4415       +7     
=============================================
  Files            565      565              
  Lines          20086    20122      +36     
  Branches         989      990       +1     
=============================================
+ Hits           11856    11894      +38     
+ Misses          7713     7710       -3     
- Partials         517      518       +1     
Files Coverage Δ
...om/faforever/client/game/CreateGameController.java 70.58% <100.00%> (+0.10%) ⬆️
...m/faforever/client/game/GenerateMapController.java 83.65% <100.00%> (+2.03%) ⬆️
...m/faforever/client/preferences/GeneratorPrefs.java 87.50% <100.00%> (+0.68%) ⬆️
...ever/client/map/generator/MapGeneratorService.java 50.51% <0.00%> (-3.93%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b15d9b...9805ecb. Read the comment docs.

@BlackYps
Copy link
Collaborator Author

Now that the text for random style and biome is the same, should the two constants GENERATOR_RANDOM_STYLE and GENERATOR_RANDOM_BIOME be consolidated into one?

@@ -358,6 +373,20 @@ protected void setStyles(List<String> styles) {
mapStyleLabel.setVisible(true);
}

protected void setBiomes(List<String> biomes) {
biomes.add(0, MapGeneratorService.GENERATOR_RANDOM_BIOME);
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this now this and set styles really should make a copy of the list that is guaranteed to be mutable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how do I do that and why is that desirable?

Copy link
Member

Choose a reason for hiding this comment

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

It is desirable because this method could be passed an immutable list so the add function would fail.

Or if a mutable list is passed this then modifies that list in ways that the caller may not be expecting so it breaks something higher up.

You would implement it by just creating a new list and then adding this list to that one.

@Sheikah45 Sheikah45 merged commit 5067d5e into develop Jan 4, 2024
5 checks passed
@Sheikah45 Sheikah45 deleted the biome-selector branch January 4, 2024 22:59
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.

None yet

2 participants