[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-08-26 Thread noreply 
The proposal to merge lp:~widelands-dev/widelands/cleanup_playercommand_enums 
into lp:widelands has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/cleanup_playercommand_enums.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-08-26 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 5341. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/576769589.
Appveyor build 5111. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cleanup_playercommand_enums-5111.
-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/cleanup_playercommand_enums.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-08-26 Thread GunChleoc 
@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/cleanup_playercommand_enums.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-08-25 Thread Toni Förster 
Review: Approve


-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/cleanup_playercommand_enums.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-07-27 Thread GunChleoc 
Addressed code review. Thanks for the review

Diff comments:

> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc2019-06-23 11:41:17 +
> +++ src/logic/playercommand.cc2019-06-24 22:00:34 +
> @@ -226,7 +200,7 @@
>  }
>  
>  void CmdBulldoze::serialize(StreamWrite& ser) {
> - ser.unsigned_8(PLCMD_BULLDOZE);
> + write_id(ser);
>   ser.unsigned_8(sender());

Done - I have replaced

void write_id(StreamWrite& ser);
 
by

void write_id_and_sender(StreamWrite& ser);

>   ser.unsigned_32(serial);
>   ser.unsigned_8(recurse);
> @@ -1263,6 +1237,7 @@
>  }
>  
>  void CmdChangeTargetQuantity::serialize(StreamWrite& ser) {
> + // Does not implement id(), so we don't have any id header to serialize

It's a common superclass, so the subclasses take care of it

>   ser.unsigned_8(sender());
>   ser.unsigned_32(economy());
>   ser.unsigned_8(ware_type());
> 
> === modified file 'src/logic/queue_cmd_ids.h'
> --- src/logic/queue_cmd_ids.h 2019-06-09 10:33:51 +
> +++ src/logic/queue_cmd_ids.h 2019-06-24 22:00:34 +
> @@ -33,17 +33,21 @@
>  
>  namespace Widelands {
>  
> -enum class QueueCommandTypes {
> +// The command types are used by the QueueCmdFactory, for network 
> serialization
> +// and for savegame compatibility.
> +// DO NOT change the order
> +// TODO(GunChleoc): Whenever we break savegame compatibility, clean this up.
> +enum class QueueCommandTypes : uint8_t {

I have added a comment

>  
>   /* ID zero is reserved and must never be used */
>   kNone = 0,
>  
>   /* PLAYER COMMANDS BELOW */
>   kBuild,
> - kFlag,
> + kBuildFlag,
>   kBuildRoad,
>   kFlagAction,
> - kStopBuilding,
> + kStartStopBuilding,
>   kEnhanceBuilding,
>   kBulldoze,
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-07-09 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 5260. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/556294766.
Appveyor build 5037. State: failed. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cleanup_playercommand_enums-5037.
-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-06-29 Thread Benedikt Straub 
Looks good to me. Some comments in the diff.

Diff comments:

> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc2019-06-23 11:41:17 +
> +++ src/logic/playercommand.cc2019-06-24 22:00:34 +
> @@ -226,7 +200,7 @@
>  }
>  
>  void CmdBulldoze::serialize(StreamWrite& ser) {
> - ser.unsigned_8(PLCMD_BULLDOZE);
> + write_id(ser);
>   ser.unsigned_8(sender());

These two lines are used pretty much everywhere. Pull out into a parent class 
method?

>   ser.unsigned_32(serial);
>   ser.unsigned_8(recurse);
> @@ -1263,6 +1237,7 @@
>  }
>  
>  void CmdChangeTargetQuantity::serialize(StreamWrite& ser) {
> + // Does not implement id(), so we don't have any id header to serialize

Why doesn´t it implement it? Looks like a bug to me…

>   ser.unsigned_8(sender());
>   ser.unsigned_32(economy());
>   ser.unsigned_8(ware_type());
> 
> === modified file 'src/logic/queue_cmd_ids.h'
> --- src/logic/queue_cmd_ids.h 2019-06-09 10:33:51 +
> +++ src/logic/queue_cmd_ids.h 2019-06-24 22:00:34 +
> @@ -33,17 +33,21 @@
>  
>  namespace Widelands {
>  
> -enum class QueueCommandTypes {
> +// The command types are used by the QueueCmdFactory, for network 
> serialization
> +// and for savegame compatibility.
> +// DO NOT change the order
> +// TODO(GunChleoc): Whenever we break savegame compatibility, clean this up.
> +enum class QueueCommandTypes : uint8_t {

Since you´re already changing it, perhaps we should make it uint16_t so we 
won´t run out of IDs some day

>  
>   /* ID zero is reserved and must never be used */
>   kNone = 0,
>  
>   /* PLAYER COMMANDS BELOW */
>   kBuild,
> - kFlag,
> + kBuildFlag,
>   kBuildRoad,
>   kFlagAction,
> - kStopBuilding,
> + kStartStopBuilding,
>   kEnhanceBuilding,
>   kBulldoze,
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands

2019-06-24 Thread GunChleoc 
GunChleoc has proposed merging 
lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.

Commit message:
Get rid of duplicate listing of player commands. This breaks compatibility for 
replays only.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267

Keeping 2 lists of the same player commands is a bit nuts, so I got rid of one 
of them.

Since replays become incompatible pretty fast anyway, I decided not to add any 
compatibility code.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.
=== modified file 'src/logic/playercommand.cc'
--- src/logic/playercommand.cc	2019-06-23 11:41:17 +
+++ src/logic/playercommand.cc	2019-06-24 22:00:34 +
@@ -74,108 +74,82 @@
 
 }  // namespace
 
-// NOTE keep numbers of existing entries as they are to ensure backward compatible savegame loading
-enum {
-	PLCMD_UNUSED = 0,
-	PLCMD_BULLDOZE = 1,
-	PLCMD_BUILD = 2,
-	PLCMD_BUILDFLAG = 3,
-	PLCMD_BUILDROAD = 4,
-	PLCMD_FLAGACTION = 5,
-	PLCMD_STARTSTOPBUILDING = 6,
-	PLCMD_ENHANCEBUILDING = 7,
-	PLCMD_CHANGETRAININGOPTIONS = 8,
-	PLCMD_DROPSOLDIER = 9,
-	PLCMD_CHANGESOLDIERCAPACITY = 10,
-	PLCMD_ENEMYFLAGACTION = 11,
-	PLCMD_SETWAREPRIORITY = 12,
-	PLCMD_SETWARETARGETQUANTITY = 13,
-	PLCMD_RESETWARETARGETQUANTITY = 14,
-	PLCMD_SETWORKERTARGETQUANTITY = 15,
-	PLCMD_RESETWORKERTARGETQUANTITY = 16,
-	// Used to be PLCMD_CHANGEMILITARYCONFIG
-	PLCMD_MESSAGESETSTATUSREAD = 18,
-	PLCMD_MESSAGESETSTATUSARCHIVED = 19,
-	PLCMD_SETSTOCKPOLICY = 20,
-	PLCMD_SETINPUTMAXFILL = 21,
-	PLCMD_DISMANTLEBUILDING = 22,
-	PLCMD_EVICTWORKER = 23,
-	PLCMD_MILITARYSITESETSOLDIERPREFERENCE = 24,
-	PLCMD_SHIP_EXPEDITION = 25,
-	PLCMD_SHIP_SCOUT = 26,
-	PLCMD_SHIP_EXPLORE = 27,
-	PLCMD_SHIP_CONSTRUCT = 28,
-	PLCMD_SHIP_SINK = 29,
-	PLCMD_SHIP_CANCELEXPEDITION = 30,
-	PLCMD_PROPOSE_TRADE = 31,
-};
-
 /*** class PlayerCommand ***/
 
 PlayerCommand::PlayerCommand(const uint32_t time, const PlayerNumber s)
: GameLogicCommand(time), sender_(s), cmdserial_(0) {
 }
 
+void PlayerCommand::write_id(StreamWrite& ser) {
+	ser.unsigned_8(static_cast(id()));
+}
+
 PlayerCommand* PlayerCommand::deserialize(StreamRead& des) {
-	switch (des.unsigned_8()) {
-	case PLCMD_BULLDOZE:
+	switch (static_cast(des.unsigned_8())) {
+	case QueueCommandTypes::kBulldoze:
 		return new CmdBulldoze(des);
-	case PLCMD_BUILD:
+	case QueueCommandTypes::kBuild:
 		return new CmdBuild(des);
-	case PLCMD_BUILDFLAG:
+	case QueueCommandTypes::kBuildFlag:
 		return new CmdBuildFlag(des);
-	case PLCMD_BUILDROAD:
+	case QueueCommandTypes::kBuildRoad:
 		return new CmdBuildRoad(des);
-	case PLCMD_FLAGACTION:
+	case QueueCommandTypes::kFlagAction:
 		return new CmdFlagAction(des);
-	case PLCMD_STARTSTOPBUILDING:
+	case QueueCommandTypes::kStartStopBuilding:
 		return new CmdStartStopBuilding(des);
-	case PLCMD_SHIP_EXPEDITION:
-		return new CmdStartOrCancelExpedition(des);
-	case PLCMD_SHIP_SCOUT:
-		return new CmdShipScoutDirection(des);
-	case PLCMD_SHIP_EXPLORE:
-		return new CmdShipExploreIsland(des);
-	case PLCMD_SHIP_CONSTRUCT:
-		return new CmdShipConstructPort(des);
-	case PLCMD_SHIP_SINK:
-		return new CmdShipSink(des);
-	case PLCMD_SHIP_CANCELEXPEDITION:
-		return new CmdShipCancelExpedition(des);
-	case PLCMD_ENHANCEBUILDING:
+	case QueueCommandTypes::kEnhanceBuilding:
 		return new CmdEnhanceBuilding(des);
-	case PLCMD_CHANGETRAININGOPTIONS:
+
+	case QueueCommandTypes::kChangeTrainingOptions:
 		return new CmdChangeTrainingOptions(des);
-	case PLCMD_DROPSOLDIER:
+	case QueueCommandTypes::kDropSoldier:
 		return new CmdDropSoldier(des);
-	case PLCMD_CHANGESOLDIERCAPACITY:
+	case QueueCommandTypes::kChangeSoldierCapacity:
 		return new CmdChangeSoldierCapacity(des);
-	case PLCMD_ENEMYFLAGACTION:
+	case QueueCommandTypes::kEnemyFlagAction:
 		return new CmdEnemyFlagAction(des);
-	case PLCMD_SETWAREPRIORITY:
+
+	case QueueCommandTypes::kSetWarePriority:
 		return new CmdSetWarePriority(des);
-	case PLCMD_SETWARETARGETQUANTITY:
+	case QueueCommandTypes::kSetWareTargetQuantity:
 		return new CmdSetWareTargetQuantity(des);
-	case PLCMD_RESETWARETARGETQUANTITY:
+	case QueueCommandTypes::kResetWareTargetQuantity:
 		return new CmdResetWareTargetQuantity(des);
-	case PLCMD_SETWORKERTARGETQUANTITY:
+	case QueueCommandTypes::kSetWorkerTargetQuantity:
 		return new CmdSetWorkerTargetQuantity(des);
-	case PLCMD_RESETWORKERTARGETQUANTITY:
+	case QueueCommandTypes::kResetWorkerTargetQuantity:
 		return new CmdResetWorkerTargetQuantity(des);
-	case PLCMD_MESSAGESETSTATUSREAD:
+
+	case QueueCommandTypes::kMessageSetStatusRead:
 		return new CmdMessageSetStatusRead(des);
-	case PLCMD_MESSAGESETSTATUSARCHIVED:
+	case QueueCommandTypes::kMessageSetStatusArchived:
 		return new CmdMessageSetStatusArchived(des);
-	case PLCMD_SETSTOCKPOLICY: