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

Resource loader cleanup #8561

Merged
merged 8 commits into from
Jan 2, 2021
Merged

Resource loader cleanup #8561

merged 8 commits into from
Jan 2, 2021

Conversation

DanVanAtta
Copy link
Member

@DanVanAtta DanVanAtta commented Jan 1, 2021

Overview

Several cleanup iterations to ResourceLoader to simplify. The code remains quite complex after this and still is a bit hacky. Notably the 'getGameEngineAssetLoader' method which uses an empty map name (that turns out to resolve the map folder to be the downloads folder which is then never going to be used as it is not expected to contain sound, so we wind up with a misleading extra search path).

Commits

commit 93a29f9

Move method 'getMapPrefix' to be colocated with its single usage

commit cecb0a5

Add comment explaining why an additional assets folder is being loaded in ResourceLoader

commit 41762ce

Change parameterization of resource discovery, pass game engine asset folder
and map folder parameters separately.

commit 0f692e1

Inline ResourceLoader factory method logic into constructor.

commit ba24d64

Remove ResourceLoader factory method, use constructor directly instead.

commit 88b5d18

Simplify ResourceLoader, remove unnecessary conversions between String and File

commit bfcf998

Avoid optional local variable, use 'orElseThrow'

commit 1ecf69f

Simplify, move variable closer to usage and clarify comment

Testing

Did some smoke testing launching a local game and walking through the code in a debugger.

Screens Shots

Additional Notes to Reviewer

Release Note

@codecov
Copy link

codecov bot commented Jan 1, 2021

Codecov Report

Merging #8561 (1ecf69f) into master (a7a7576) will increase coverage by 0.02%.
The diff coverage is 21.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8561      +/-   ##
============================================
+ Coverage     24.83%   24.86%   +0.02%     
- Complexity     7304     7307       +3     
============================================
  Files          1265     1265              
  Lines         79592    79618      +26     
  Branches      11085    10991      -94     
============================================
+ Hits          19765    19795      +30     
- Misses        57754    57758       +4     
+ Partials       2073     2065       -8     
Impacted Files Coverage Δ Complexity Δ
...c/main/java/org/triplea/domain/data/LobbyGame.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...trategy/engine/auto/update/EngineVersionCheck.java 50.00% <ø> (-1.73%) 8.00 <0.00> (-1.00)
...ames/strategy/engine/auto/update/UpdateChecks.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ava/games/strategy/engine/chat/ChatController.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...trategy/engine/chat/MessengersChatTransmitter.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../strategy/engine/data/BombingUnitDamageChange.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...c/main/java/games/strategy/engine/data/Change.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...s/strategy/engine/data/ChangeAttachmentChange.java 74.28% <ø> (ø) 5.00 <0.00> (ø)
...in/java/games/strategy/engine/data/GamePlayer.java 60.49% <0.00%> (ø) 26.00 <0.00> (ø)
...main/java/games/strategy/engine/data/GameStep.java 45.65% <0.00%> (-11.11%) 10.00 <0.00> (ø)
... and 416 more

Continue to review full report at Codecov.

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

@RoiEXLab RoiEXLab merged commit 3daac13 into master Jan 2, 2021
@RoiEXLab RoiEXLab deleted the resource-loader-cleanup branch January 2, 2021 11:29
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