Load campaign data only once #1710
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Flamefire
force-pushed
the
fix-campaign-load-dsk
branch
4 times, most recently
from
a2b2f01
to
bbf6420
Compare
Flow86
requested changes
Oct 28, 2024
The test doesn't contain on s25Main so we can reduce the amount of compilation required. Also move the campaign test out of the s25Main tests.
Now the image is either a non-empty string or `nullopt`
Load data for all campaigns when opening the capaign selection window. We don't rescan the folders while the window is open so there is no need to load the lua files multiple times. Especially the draw code for the map image loads the current campaign on every draw call to determine what is to be drawn. This also spams the log with messages such as missing languages. If we pass the parsed campaign to the mission selection screen we can also get rid of the invisible column for the filepath. Finally we can load all ressources (images) in one go. Fixes Return-To-The-Roots#1705
Avoid use of sentinel values in presence of C++17 `std::optional`. Makes it more obvious what is returned. Also rename from `getCurrentSelection` for consistency with e.g. `ctrlList` and similar and `setSelection`.
The classes share some code and essentially the same functionality. Especially problematic is the reliance on the "mission map" in the campagin description which may lead to an empty screen if `dskCampaignMissionMapSelection` is instantiated in absence of that. Additionally the difference in naming doesn't make it clear which is which.
Allow to easily identify the expected type for `GetCtrl` calls.
The mission selection map starts with nothing selected so the "Start" button does nothing on click. Disable it until something gets selected for better UX.
Flamefire
force-pushed
the
fix-campaign-load-dsk
branch
from
bbf6420
to
460f124
Compare
Flow86
approved these changes
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #1705 but is a larger rework to make campaign handling easier and more flexible.
The reason for the bug was that the campaign file is loaded repeatedly, especially in the draw call to determine whether to draw the image.
So load data for all campaigns when opening the capaign selection window.
We don't rescan the folders while the window is open so there is no need to load the lua files multiple times.
If we pass the parsed campaign to the mission selection screen we can also get rid of the invisible column for the filepath.
Finally we can load all resources (images) in one go.
The 2 mission selection classes share some code and essentially the same functionality and the difference in naming doesn't make it clear which is which.
Especially problematic is the reliance on the "mission map" in the campaign description which may lead to an empty screen if
dskCampaignMissionMapSelectionis instantiated in absence of that.So just merge them so the campaign selection doesn't need to decide which one to use and the screen can use the representation that is available, i.e. either a map to select an area or the list as a fallback. The result looks exactly the same.
I noticed that we can rather use
std::optionalin 2 places for improved semantics. When modifying the test I noticed that the whole project gets recompiled instead of onlylibGamedata(I moved the test to the corresponding test suite) which was due to some superflous dependencies in CMake.