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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions data/RTTR/campaigns/roman/MISS200.lua
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ rttr:RegisterTranslations(
},
})

function getNumPlayers()
return 1
end

-- format mission texts
-- BUG: NewLine at the end is wrongly interpreted, adding 2x Space resolves this issue
function MissionText(e)
Expand Down Expand Up @@ -205,13 +209,6 @@ function onSettingsReady()

rttr:GetPlayer(0):SetNation(NAT_ROMANS) -- nation
rttr:GetPlayer(0):SetColor(0) -- 0:blue, 1:red, 2:yellow,

rttr:GetPlayer(1):Close()
rttr:GetPlayer(2):Close()
rttr:GetPlayer(3):Close()
rttr:GetPlayer(4):Close()
rttr:GetPlayer(5):Close()
rttr:GetPlayer(6):Close()
end

function getAllowedChanges()
Expand Down
8 changes: 6 additions & 2 deletions data/RTTR/campaigns/roman/MISS206.lua
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,12 @@ function addPlayerBld(p, onLoad)
rttr:GetPlayer(p):DisableBuilding(BLD_SHIPYARD, false)
rttr:GetPlayer(p):DisableBuilding(BLD_HARBORBUILDING, false)

if(p == 2) then
if onLoad then return end
if onLoad then return end

if(p == 1) then
rttr:GetPlayer(p):PlaceHQ(112, 82) -- !SET_HOUSE 24, 112, 82
elseif(p == 2) then
rttr:GetPlayer(p):PlaceHQ(40, 54) -- !SET_HOUSE 24, 40, 54
rttr:GetPlayer(p):AIConstructionOrder(43, 59, BLD_FORTRESS)
end
end
Expand Down
4 changes: 4 additions & 0 deletions doc/lua/functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,10 @@ Ignored if the current player is not controlled by AI.
**GetHQPos()**
Return x,y of the players HQ

**PlaceHQ(x, y)**
Places this player's headquarters at map location {x, y}.
Does nothing if the player already has a valid headquarters position (e.g. set by the map or using a previous PlaceHQ call).
Copy link
Member

Choose a reason for hiding this comment

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

See below.

Suggested change
Does nothing if the player already has a valid headquarters position (e.g. set by the map or using a previous PlaceHQ call).
Does nothing if the player already has an existing headquarter (e.g. set by the map or using a previous PlaceHQ call).

Maybe even remove the part in the parentheses


**Surrender(destroyBuildings)**
Let the player give up either keeping its buildings or destroying them.
Can be called multiple times e.g. to destroy buildings later.
Expand Down
5 changes: 5 additions & 0 deletions libs/s25main/GameLobby.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,8 @@ unsigned GameLobby::getNumPlayers() const
{
return players_.size();
}

void GameLobby::setNumPlayers(unsigned num)
{
players_.resize(num);
}
1 change: 1 addition & 0 deletions libs/s25main/GameLobby.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class GameLobby
const JoinPlayerInfo& getPlayer(unsigned playerId) const;
const std::vector<JoinPlayerInfo>& getPlayers() const { return players_; }
unsigned getNumPlayers() const;
void setNumPlayers(unsigned num);

GlobalGameSettings& getSettings() { return ggs_; }
const GlobalGameSettings& getSettings() const { return ggs_; }
Expand Down
20 changes: 20 additions & 0 deletions libs/s25main/GamePlayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "WineLoader.h"
#include "addons/const_addons.h"
#include "buildings/noBuildingSite.h"
#include "buildings/nobHQ.h"
#include "buildings/nobHarborBuilding.h"
#include "buildings/nobMilitary.h"
#include "buildings/nobUsual.h"
Expand Down Expand Up @@ -411,6 +412,19 @@ void GamePlayer::RemoveBuildingSite(noBuildingSite* bldSite)
buildings.Remove(bldSite);
}

bool GamePlayer::IsHQTent() const
Copy link
Member

Choose a reason for hiding this comment

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

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

{
if(const nobHQ* hq = GetHQ())
return hq->IsTent();
return false;
}

void GamePlayer::SetHQIsTent(bool isTent)
Copy link
Member

Choose a reason for hiding this comment

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

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.

{
if(nobHQ* hq = const_cast<nobHQ*>(GetHQ()))
Copy link
Member

Choose a reason for hiding this comment

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

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

hq->SetIsTent(isTent);
}

void GamePlayer::AddBuilding(noBuilding* bld, BuildingType bldType)
{
RTTR_Assert(bld->GetPlayer() == GetPlayerId());
Expand Down Expand Up @@ -1400,6 +1414,12 @@ void GamePlayer::TestDefeat()
Surrender();
}

const nobHQ* GamePlayer::GetHQ() const
{
const MapPoint& hqPos = GetHQPos();
return hqPos.isValid() ? GetGameWorld().GetSpecObj<nobHQ>(hqPos) : nullptr;
}

void GamePlayer::Surrender()
{
if(isDefeated)
Expand Down
5 changes: 5 additions & 0 deletions libs/s25main/GamePlayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class noShip;
class nobBaseMilitary;
class nobBaseWarehouse;
class nobHarborBuilding;
class nobHQ;
class nobMilitary;
class nofCarrier;
class nofFlagWorker;
Expand Down Expand Up @@ -91,6 +92,9 @@ class GamePlayer : public GamePlayerInfo
const GameWorld& GetGameWorld() const { return world; }

const MapPoint& GetHQPos() const { return hqPos; }
bool IsHQTent() const;
void SetHQIsTent(bool isTent);

void AddBuilding(noBuilding* bld, BuildingType bldType);
void RemoveBuilding(noBuilding* bld, BuildingType bldType);
void AddBuildingSite(noBuildingSite* bldSite);
Expand Down Expand Up @@ -426,6 +430,7 @@ class GamePlayer : public GamePlayerInfo
bool FindWarehouseForJob(Job job, noRoadNode* goal) const;
/// Prüft, ob der Spieler besiegt wurde
void TestDefeat();
const nobHQ* GetHQ() const;

//////////////////////////////////////////////////////////////////////////
/// Unsynchronized state (e.g. lua, gui...)
Expand Down
8 changes: 8 additions & 0 deletions libs/s25main/desktops/dskGameLobby.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "ingameWindows/iwMsgbox.h"
#include "lua/LuaInterfaceSettings.h"
#include "network/GameClient.h"
#include "network/GameServer.h"
#include "ogl/FontStyle.h"
#include "gameData/GameConsts.h"
#include "gameData/const_gui_ids.h"
Expand Down Expand Up @@ -102,6 +103,13 @@ dskGameLobby::dskGameLobby(ServerType serverType, std::shared_ptr<GameLobby> gam
RTTR_Assert(gameLobby_->isHost());
LOG.write(_("Lua was disabled by the script itself\n"));
lua.reset();
} else
{
if(const auto num = lua->GetNumPlayersFromScript())
{
GAMESERVER.SetNumPlayers(num);
Copy link
Member

Choose a reason for hiding this comment

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

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?

gameLobby_->setNumPlayers(num);
}
}
if(!lua && gameLobby_->isHost())
lobbyController->RemoveLuaScript();
Expand Down
10 changes: 10 additions & 0 deletions libs/s25main/lua/LuaInterfaceSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,13 @@ bool LuaInterfaceSettings::IsMapPreviewEnabled()
}
return true;
}

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

Choose a reason for hiding this comment

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

Please add documentation for this new function.

if(func.type() == LUA_TFUNCTION)
{
return func.call<unsigned>();
}
return 0;
}
1 change: 1 addition & 0 deletions libs/s25main/lua/LuaInterfaceSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class LuaInterfaceSettings : public LuaInterfaceGameBase
/// Get addons that are allowed to be changed
std::vector<AddonId> GetAllowedAddons();
bool IsMapPreviewEnabled();
unsigned GetNumPlayersFromScript();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unsigned GetNumPlayersFromScript();
/// Get number of players as set in the script. Return zero to use the value set in the map/save file.
unsigned GetNumPlayersFromScript();

Or maybe returning an optional is more clear and disallow zero for lua files.


private:
IGameLobbyController& lobbyServerController_;
Expand Down
23 changes: 16 additions & 7 deletions libs/s25main/lua/LuaPlayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "ai/AIPlayer.h"
#include "buildings/nobBaseWarehouse.h"
#include "buildings/nobHQ.h"
#include "factories/BuildingFactory.h"
#include "helpers/EnumRange.h"
#include "helpers/toString.h"
#include "lua/LuaHelpers.h"
Expand Down Expand Up @@ -46,6 +47,7 @@ void LuaPlayer::Register(kaguya::State& state)
.addFunction("GetNumPeople", &LuaPlayer::GetNumPeople)
.addFunction("GetStatisticsValue", &LuaPlayer::GetStatisticsValue)
.addFunction("AIConstructionOrder", &LuaPlayer::AIConstructionOrder)
.addFunction("PlaceHQ", &LuaPlayer::PlaceHQ)
.addFunction("ModifyHQ", &LuaPlayer::ModifyHQ)
.addFunction("GetHQPos", &LuaPlayer::GetHQPos)
.addFunction("IsDefeated", &LuaPlayer::IsDefeated)
Expand Down Expand Up @@ -259,15 +261,22 @@ bool LuaPlayer::AIConstructionOrder(unsigned x, unsigned y, lua::SafeEnum<Buildi
return true;
}

void LuaPlayer::PlaceHQ(MapCoord x, MapCoord y)
{
// Ignore if there is an HQ set in the map file.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually care about the map file directly. A script might (attempt to) set the HQ multiple times. Or re-add it if destroyed.

Suggested change
// Ignore if there is an HQ set in the map file.
// Ignore if an HQ already exists

if(player.GetHQPos().isValid())
return;

GameWorld& world = player.GetGameWorld();
const MapPoint mp{x, y};
constexpr auto checkExists = false;
world.DestroyNO(mp, checkExists);
BuildingFactory::CreateBuilding(world, BuildingType::Headquarters, mp, player.GetPlayerId(), player.nation);
Comment on lines +271 to +274
Copy link
Member

Choose a reason for hiding this comment

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

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);

}

void LuaPlayer::ModifyHQ(bool isTent)
{
const MapPoint hqPos = player.GetHQPos();
if(hqPos.isValid())
{
auto* hq = player.GetGameWorld().GetSpecObj<nobHQ>(hqPos);
if(hq)
hq->SetIsTent(isTent);
}
player.SetHQIsTent(isTent);
}

bool LuaPlayer::IsDefeated() const
Expand Down
2 changes: 2 additions & 0 deletions libs/s25main/lua/LuaPlayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "gameTypes/BuildingType.h"
#include "gameTypes/GoodTypes.h"
#include "gameTypes/JobTypes.h"
#include "gameTypes/MapCoordinates.h"
#include "gameTypes/PactTypes.h"
#include "gameTypes/StatisticTypes.h"
#include <map>
Expand Down Expand Up @@ -50,6 +51,7 @@ class LuaPlayer : public LuaPlayerBase
unsigned GetNumPeople(lua::SafeEnum<Job> job) const;
unsigned GetStatisticsValue(lua::SafeEnum<StatisticType> stat) const;
bool AIConstructionOrder(unsigned x, unsigned y, lua::SafeEnum<BuildingType> bld);
void PlaceHQ(MapCoord x, MapCoord y);
void ModifyHQ(bool isTent);
bool IsDefeated() const;
void Surrender(bool destroyBlds);
Expand Down
6 changes: 4 additions & 2 deletions libs/s25main/network/GameClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,10 @@ void GameClient::StartGame(const unsigned random_init)
gameWorld.GetPlayer(i).MakeStartPacts();

MapLoader loader(gameWorld);
if(!loader.Load(mapinfo.filepath)
|| (!mapinfo.luaFilepath.empty() && !loader.LoadLuaScript(*game, *this, mapinfo.luaFilepath)))
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.
Comment on lines +326 to +329
Copy link
Member

Choose a reason for hiding this comment

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

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))

{
OnError(ClientError::InvalidMap);
return;
Expand Down
5 changes: 5 additions & 0 deletions libs/s25main/network/GameServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,11 @@ bool GameServer::assignPlayersOfRandomTeams(std::vector<JoinPlayerInfo>& playerI
return playerWasAssigned;
}

void GameServer::SetNumPlayers(unsigned num)
{
playerInfos.resize(num);
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

/**
* startet das Spiel.
*/
Expand Down
2 changes: 2 additions & 0 deletions libs/s25main/network/GameServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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);


private:
bool StartGame();

Expand Down
5 changes: 3 additions & 2 deletions libs/s25main/world/MapLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -409,8 +409,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;
Copy link
Member

Choose a reason for hiding this comment

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

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)

if(!world.HasLua()) // HQ can be placed in the script, so don't signal error
return false;
}

BuildingFactory::CreateBuilding(world, BuildingType::Headquarters, hqPositions[i], i,
Expand Down
Loading