Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" #1683

kubaau · 2024-07-23T12:12:02Z

Fixes map "The snake" WITHOUT changing the map. You can now use the original S2 map, but I needed to implement some additional features, like placing HQs and overriding the number of players from LUA scripts. For example Roman chapter 1 does not show 7 players anymore and chapter 7 has the player 3 HQ set from LUA (video). This also makes it possible to fix all other maps where the positions are not set in the map but only in RTX files (like half the FANpaign maps - example video).

Fixes #1624

data/RTTR/campaigns/roman/MISS206.lua

Spikeone · 2024-07-25T15:15:01Z

This is gerat, but I'd still like to have some feature to "fix" map 6, where they failed when converting from WORLD.DAT to WLD :D

kubaau · 2024-07-26T22:23:13Z

This is gerat, but I'd still like to have some feature to "fix" map 6, where they failed when converting from WORLD.DAT to WLD :D

What do you mean by this?

Spikeone · 2024-07-27T08:18:51Z

This is gerat, but I'd still like to have some feature to "fix" map 6, where they failed when converting from WORLD.DAT to WLD :D

What do you mean by this?

On map IX they changed the map between the original version and gold/mission CD. For one instance they removed the harbors completly, this means there is no more seafaring. They also changed the terrain for the second AI, which had nearly no way of farming or producing wood since they used a non farmable terrain which simply stopped that AI from doing anything usefull.

You can compare that version: Campaign_OriginalRoman/MISS208.WLD with the one in your game directory, I've created it by reading the WORLD.DAT file of the original campaign and replacing the terrain with the correct one and re adding the harbors

kubaau · 2024-07-27T11:26:53Z

On map IX

Ah yes, that one. Yeah this one is messed up in mission CD/gold. The RTX file is also wrong in a couple of places resulting in NO STRING WITH THIS ID errors and instead of placing an activated gate in the right place they place a rock in the middle of water north of your HQ... I think the only way to handle being given the original file by the user is to add code that detects that this is indeed The Gray Island and adds/modifies some data it reads internally (I don't know if we can add such a modified WLD file in the game files itself?).

If you create an issue for this I would very much like to take a look at this once I finish all the stuff I currently have open. While this does actually change an original map (which I don't like in principle), I think it is safe to assume the map was not supposed to be played as it was post mission CD.

Flamefire

I hope this does not break something later on, but looks good for now :)

If you put two HQs for a player from LUA then only the first one will be placed. This needs documenting.

Indeed.

libs/s25main/GamePlayer.cpp

libs/s25main/network/GameClient.cpp

libs/s25main/world/MapLoader.cpp

Flamefire

I added some comments and questions.

We also need to increase the version in unsigned LuaInterfaceGameBase::GetFeatureLevel() due to the added functions such that scripts can check for that. Also the 2 changed mission scripts should require that new version too

Flamefire · 2024-08-08T11:02:31Z

libs/s25main/GamePlayer.cpp

@@ -376,6 +377,19 @@ void GamePlayer::RemoveBuildingSite(noBuildingSite* bldSite)
    buildings.Remove(bldSite);
 }

+bool GamePlayer::IsHQTent() const


What is the reason for adding this here? It isn't used (yet)

Flamefire · 2024-08-08T11:15:09Z

libs/s25main/GamePlayer.cpp

+    return false;
+}
+
+void GamePlayer::SetHQIsTent(bool isTent)


This and GetHQ is basically the code moved from ModifyHQ. What was the motivation?

I'm thinking about the case where there are multiple HQs (e.g. the S2 cheats had it) and one might want to specify which HQ to change. Putting the code in the caller uses the world to get a specific HQ and changes that.

Although we already have GetHQPos which returns the position of "a" HQ, IIRC the last one added that still exists.

Flamefire · 2024-08-08T11:17:20Z

libs/s25main/GamePlayer.cpp

+
+void GamePlayer::SetHQIsTent(bool isTent)
+{
+    if(nobHQ* hq = const_cast<nobHQ*>(GetHQ()))


I missed that GetSpecObj (in GetHQ) can return a non-const pointer. So we don't need the const cast at all.

If you make GetHQ non-const then this can be removed

Flamefire · 2024-08-08T11:24:09Z

libs/s25main/desktops/dskGameLobby.cpp

+        {
+            if(const auto num = lua->GetNumPlayersFromScript())
+            {
+                GAMESERVER.SetNumPlayers(num);


This won't work for savegames, will it? Because I think for a savegame the player needs to be already initialized.
Or does that work still?

Flamefire · 2024-08-08T11:25:16Z

libs/s25main/lua/LuaInterfaceSettings.cpp

+
+unsigned LuaInterfaceSettings::GetNumPlayersFromScript()
+{
+    kaguya::LuaRef func = lua["getNumPlayers"];


Please add documentation for this new function.

Flamefire · 2024-08-08T11:33:35Z

libs/s25main/lua/LuaPlayer.cpp

+    const MapPoint mp{x, y};
+    constexpr auto checkExists = false;
+    world.DestroyNO(mp, checkExists);
+    BuildingFactory::CreateBuilding(world, BuildingType::Headquarters, mp, player.GetPlayerId(), player.nation);


Nit: Better use pos(ition) as in other places

Suggested change

const MapPoint mp{x, y};

constexpr auto checkExists = false;

world.DestroyNO(mp, checkExists);

BuildingFactory::CreateBuilding(world, BuildingType::Headquarters, mp, player.GetPlayerId(), player.nation);

const MapPoint pos{x, y};

constexpr auto checkExists = false;

world.DestroyNO(pos, checkExists);

BuildingFactory::CreateBuilding(world, BuildingType::Headquarters, pos, player.GetPlayerId(), player.nation);

Flamefire · 2024-08-08T11:38:40Z

libs/s25main/network/GameClient.cpp

+        if((!mapinfo.luaFilepath.empty() && !loader.LoadLuaScript(*game, *this, mapinfo.luaFilepath))
+           || !loader.Load(mapinfo.filepath)) // Do not reorder: load lua first, load map second.
+                                              // If the map is loaded first and it does not have a player HQ set, it
+                                              // will not load correctly, even though the HQ may be set using LUA.


I still find it hard to understand why it will "not load correctly" and what loading the script first changes. What about:

Suggested change

if((!mapinfo.luaFilepath.empty() && !loader.LoadLuaScript(*game, *this, mapinfo.luaFilepath))

|| !loader.Load(mapinfo.filepath)) // Do not reorder: load lua first, load map second.

// If the map is loaded first and it does not have a player HQ set, it

// will not load correctly, even though the HQ may be set using LUA.

// Load (and verify) Lua script before the map.

// If a valid script exists loading the map will not abort when HQ positions in the map are missing

// as HQs might be placed by the script when starting the game.

if((!mapinfo.luaFilepath.empty() && !loader.LoadLuaScript(*game, *this, mapinfo.luaFilepath))

|| !loader.Load(mapinfo.filepath))

Flamefire · 2024-08-08T11:46:23Z

libs/s25main/network/GameServer.cpp

@@ -526,6 +526,11 @@ bool GameServer::assignPlayersOfRandomTeams(std::vector<JoinPlayerInfo>& playerI
    return playerWasAssigned;
 }

+void GameServer::SetNumPlayers(unsigned num)
+{
+    playerInfos.resize(num);


We should add a check (state == ServerState::Config) before doing this. Maybe:

Suggested change

playerInfos.resize(num);

if(state != ServerState::Config)

throw std::logic_error("Changing the number of players must only be done during game configuration");

if(num < 1 || num > MAX_PLAYERS)

throw std::invalid_argument("Invalid number of players");

playerInfos.resize(num);

Not sure about the 2nd check as the value is likely from a lua script. So maybe rather return false here and abort the game in the caller.

Flamefire · 2024-08-08T11:47:11Z

libs/s25main/network/GameServer.h

@@ -52,6 +52,8 @@ class GameServer :
    /// Assign players that do not have a fixed team, return true if any player was assigned.
    static bool assignPlayersOfRandomTeams(std::vector<JoinPlayerInfo>& playerInfos);

+    void SetNumPlayers(unsigned num);


Maybe document as something like:

Suggested change

void SetNumPlayers(unsigned num);

/// Change number of players (after loading the map but before starting the game)

void SetNumPlayers(unsigned num);

Flamefire · 2024-08-08T11:55:46Z

libs/s25main/world/MapLoader.cpp

@@ -421,8 +421,9 @@ bool MapLoader::PlaceHQs(GameWorldBase& world, std::vector<MapPoint> hqPositions
        // Does the HQ have a position?
        if(i >= hqPositions.size() || !hqPositions[i].isValid())
        {
-            LOG.write(_("Player %u does not have a valid start position!")) % i;
-            return false;
+            LOG.write(_("Player %u does not have a valid start position!\n")) % i;


This needs an update to the translations. I used a quick regex-search-replace in that repo. Please update the submodule external/languages to the current master (revision fa7f546)

- document PlaceHQ - document why LUA should be loaded before the map

Spikeone reviewed Jul 25, 2024

View reviewed changes

data/RTTR/campaigns/roman/MISS206.lua Outdated Show resolved Hide resolved

Flamefire requested changes Jul 29, 2024

View reviewed changes

libs/s25main/GamePlayer.cpp Outdated Show resolved Hide resolved

libs/s25main/network/GameClient.cpp Outdated Show resolved Hide resolved

libs/s25main/world/MapLoader.cpp Outdated Show resolved Hide resolved

kubaau requested a review from Flamefire August 5, 2024 17:01

Flamefire changed the title ~~Fix map "The snake" and enable overriding crucial map settings from LUA~~ Lua: Allow setting number of players, and placing HQs. Then fix the map "The snake" Aug 8, 2024

Flamefire changed the title ~~Lua: Allow setting number of players, and placing HQs. Then fix the map "The snake"~~ Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" Aug 8, 2024

Flamefire requested changes Aug 8, 2024

View reviewed changes

kubaau added 5 commits December 13, 2024 22:44

Fix map "The Snake"

7625b14

Enabling setting number of players from LUA

d2e885c

Apply review remarks:

dbd105d

- document PlaceHQ - document why LUA should be loaded before the map

Apply review remark: don't const_cast inside GamePlayer::GetHQ

47b869f

Remove IsHQTent tests (require features from another branch)

9ea584a

kubaau force-pushed the lua_fixing_wld branch from dd32aa1 to 9ea584a Compare December 13, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" #1683

Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" #1683

kubaau commented Jul 23, 2024 •

edited by Flamefire

Loading

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024

Spikeone commented Jul 27, 2024

kubaau commented Jul 27, 2024 •

edited

Loading

Flamefire left a comment

Flamefire left a comment •

edited

Loading

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

Flamefire Aug 8, 2024

-    playerInfos.resize(num);
+    if(state != ServerState::Config)
+        throw std::logic_error("Changing the number of players must only be done during game configuration");
+    if(num < 1 || num > MAX_PLAYERS)
+        throw std::invalid_argument("Invalid number of players");
+    playerInfos.resize(num);

	void SetNumPlayers(unsigned num);
	/// Change number of players (after loading the map but before starting the game)
	void SetNumPlayers(unsigned num);

Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" #1683

Are you sure you want to change the base?

Lua: Allow setting number of players, and placing HQs. Then fix the mission on map "The snake" #1683

Conversation

kubaau commented Jul 23, 2024 • edited by Flamefire Loading

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024

Spikeone commented Jul 27, 2024

kubaau commented Jul 27, 2024 • edited Loading

Flamefire left a comment

Choose a reason for hiding this comment

Flamefire left a comment • edited Loading

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 23, 2024 •

edited by Flamefire

Loading

kubaau commented Jul 27, 2024 •

edited

Loading

Flamefire left a comment •

edited

Loading