Add support for campaign status #1681

kubaau · 2024-07-23T12:09:53Z

Adds support for Roman-style (video) campaigns and the world campaign (video).

Fixed world campaign scripts to unlock the same continents as it was in S2.

Related to issues #1639 and #1177

Spikeone · 2024-07-25T15:05:39Z

Either I'm blind or the implementation is somewhere else? What is rttr:SetCampaignChapterCompleted good for?

kubaau · 2024-07-26T22:11:59Z

Either I'm blind or the implementation is somewhere else? What is rttr:SetCampaignChapterCompleted good for?

~~It doesn't make a difference in Roman-style campaigns, but~~ it marks a continent as conquered in the world campaign. It could also be used for other campaign types as well if we wanted to show what was completed, especially if someone designs a campaign whose progression is not linear.

Edit: For roman-style campaigns it shows the "You have successfully completed chapter X." screen after ending the game.

libs/s25main/lua/LuaInterfaceGame.cpp

Flamefire

Awesome, the last bit for campaigns! Review inline.

General hint: Try to make best use of the type system. E.g. "unsigned char" is/should be a (text-)character (although only since C++11 introduced uint8_t) And using this narrow width number-type as parameters likely introduces overhead (registers are wider so often need to be masked) so it is even faster to just use unsigned there.
Similar for string which should be text. It can be stored in the config as a string for brevity but converted during load/save to a more appropriate type, especially when (scoped) enums can be used.

In general I like the changes, thanks! :)

Flamefire · 2024-07-29T10:16:31Z

libs/libGamedata/gameData/CampaignSaveCodes.h

+constexpr auto chapterDisabled = '0';
+constexpr auto chapterEnabled = '1';
+constexpr auto chapterCompleted = '2';
+constexpr auto defaultChaptersEnabled = "1100000000";


What exactly does this mean? Can you add some description?

Flamefire · 2024-07-29T10:18:31Z

libs/libGamedata/gameData/CampaignDescription.h

@@ -23,6 +24,7 @@ struct CampaignDescription
    std::string image;
    unsigned maxHumanPlayers = 0;
    std::string difficulty;
+    std::string defaultChaptersEnabled;


Can you add a docstring what this is and how it should be interpreted? Maybe a string isn't the right type as it isn't exactly text. I guess it is a variable sized array of enum type represented as a string. So maybe an own type and corresponding lua loader is better.

Flamefire · 2024-07-29T10:23:23Z

libs/s25main/CampaignSaveData.cpp

+    std::vector<MissionStatus> ret(saveData.length());
+    for(auto i = 0u; i < saveData.length(); ++i)
+    {
+        ret[i].playable = saveData[i] != CampaignSaveCodes::chapterDisabled;
+        ret[i].conquered = saveData[i] == CampaignSaveCodes::chapterCompleted;
+    }
+
+    ret.resize(campaignDesc.getNumMaps());


I'd say it would be better: Either a range-based for and push_back or use numMaps as the initial size and loop over rttr::Range(min(saveData.length(), ret.length())

Flamefire · 2024-07-29T10:24:35Z

libs/s25main/CampaignSaveData.h

+
+struct CampaignDescription;
+
+bool isChapterEnabled(const CampaignDescription& campaignDesc, unsigned char chapter);


The argument isn't a char but a number:

Suggested change

bool isChapterEnabled(const CampaignDescription& campaignDesc, unsigned char chapter);

bool isChapterEnabled(const CampaignDescription& campaignDesc, unsigned chapter);

Or uint8_t although that is likely even slower so isn't really useful, is it?

Flamefire · 2024-07-29T10:32:15Z

libs/s25main/CampaignSaveData.cpp

+namespace {
+auto getCampaignSaveData(const CampaignDescription& campaignDesc)
+{
+    auto& saveData = SETTINGS.campaigns.saveData;
+
+    if(!saveData.count(campaignDesc.uid))
+        saveData[campaignDesc.uid] = campaignDesc.defaultChaptersEnabled;
+
+    return saveData[campaignDesc.uid];
+}
+} // namespace


For a single function (or even just functions) static is less clutter:

Suggested change

namespace {

auto getCampaignSaveData(const CampaignDescription& campaignDesc)

{

auto& saveData = SETTINGS.campaigns.saveData;

if(!saveData.count(campaignDesc.uid))

saveData[campaignDesc.uid] = campaignDesc.defaultChaptersEnabled;

return saveData[campaignDesc.uid];

}

} // namespace

static auto getCampaignSaveData(const CampaignDescription& campaignDesc)

{

auto& saveData = SETTINGS.campaigns.saveData;

if(!saveData.count(campaignDesc.uid))

saveData[campaignDesc.uid] = campaignDesc.defaultChaptersEnabled;

return saveData[campaignDesc.uid];

}

Also do you intend to return a copy? It looks like it is supposed to be either a reference or a const reference

Flamefire · 2024-07-29T11:11:16Z

tests/s25Main/campaign/testCampaignSaveData.cpp

+
+BOOST_FIXTURE_TEST_CASE(Chapters0and1AreEnabledByDefault, CampaignSaveDataFixture)
+{
+    saveData[uid] = CampaignSaveCodes::defaultChaptersEnabled;


So the default is to enable the first 2 chapters? Why that? Wouldn't (only) the first one be a more sensible default?
I'm also a bit confused as you seem to be using indices and 1-based values for chapters

Maybe stick to indices and use an optional instead of zero

Flamefire · 2024-07-29T11:14:21Z

tests/s25Main/campaign/testCampaignSaveData.cpp

+    BOOST_TEST_REQUIRE(isChapterEnabled(desc, 0) == true);
+    BOOST_TEST_REQUIRE(isChapterEnabled(desc, 1) == false);
+    BOOST_TEST_REQUIRE(isChapterEnabled(desc, 2) == false);
+    BOOST_TEST_REQUIRE(isChapterEnabled(desc, 9) == false);


This should also include a change to saveData to check that the default really is only a default and overwritten by an explicit value.

Flamefire · 2024-07-29T11:15:38Z

tests/s25Main/campaign/testCampaignSaveData.cpp

+    }
+
+    CampaignDescription dsc;
+    CampaignDataLoader loader{dsc, tmp};


Why can't this just use desc from the fixture?

Flamefire · 2024-07-29T11:16:21Z

tests/s25Main/lua/testLua.cpp

+    executeLua("rttr:SetCampaignChapterCompleted('campaign_id', 0)"); // noop - chapters start from 1
+    executeLua("rttr:EnableCampaignChapter('campaign_id', 0)");       // noop - chapters start from 1
+    executeLua("rttr:EnableCampaignChapter('campaign_id', 2)");       // noop - already completed
+    game.executeAICommands();


What is this line for?

Flamefire · 2024-07-29T11:23:01Z

libs/libGamedata/gameData/CampaignDescription.cpp

@@ -12,6 +13,7 @@
 CampaignDescription::CampaignDescription(const kaguya::LuaRef& table)
 {
    CheckedLuaTable luaData(table);
+    luaData.getOrThrow(uid, "uid");


For this you need to Increase CampaignDataLoader::GetVersion()

I'd try to make this backwards compatible: and make the uid optional. Or check the version here (again) and make sure the uid is empty for older versions and non-empty otherwise.

Then you can revert the changes to existing tests

Add support for campaign status

53bef06

kubaau commented Jul 26, 2024

View reviewed changes

libs/s25main/lua/LuaInterfaceGame.cpp Show resolved Hide resolved

Do not enable a campaign chapter that is already completed

2acb961

Flamefire requested changes Jul 29, 2024

View reviewed changes

kubaau added 4 commits December 22, 2024 15:02

Merge branch 'master' into campaign_status

5171c98

Add support for campaign status

679851a

Temporarily enable Roman chapter 8 after completing chapter 6

59d8813

Merge branch 'campaign_status_2' into campaign_status

2ae89be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for campaign status #1681

Add support for campaign status #1681

kubaau commented Jul 23, 2024

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024 •

edited

Loading

Flamefire left a comment

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024

Flamefire Jul 29, 2024


		struct CampaignDescription;

		bool isChapterEnabled(const CampaignDescription& campaignDesc, unsigned char chapter);

Add support for campaign status #1681

Are you sure you want to change the base?

Add support for campaign status #1681

Conversation

kubaau commented Jul 23, 2024

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024 • edited Loading

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubaau commented Jul 26, 2024 •

edited

Loading