Merge lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands

[Toni Förster]

- fix broadcast()
- fix send_message() for territorial

The win conditions territorial, woodgnome and collectors should not desync anymore.

[kaputtnik (franku)]

Please pay attention for messages where each player get a different message (e.g. different land size). In think this is the case in territorial_time and territorial_lord. Using broadcast() will send the same message to all players.

We should also take care of the RST comments... i could do that if you don't want to touch them.

review: Needs Fixing

[Toni Förster (stonerl)]

Are you sure. I thought that the status messages are the same for all players. A message usually contains the sizes for all players. But I could be wrong.

If you would take care of the RST that would be nice.

[kaputtnik (franku)]

Yes, i am, just look at the code. E.g. for territorial_lord they are:

For the currently winning player: "You own more than half of the map’s area. Keep it for %i more minute to win the game."

For the currently loosing player: "Playername owns more than half of the map’s area. You’ve still got %i minute to prevent a victory."

I will look at the RST's tomorrow :-)

[Toni Förster (stonerl)]

You're right... fixed it.

[Toni Förster (stonerl)]

review: Needs Resubmitting

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4415. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/484807553.
Appveyor build 4205. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1811583_desync_with_territorial-4205.

[kaputtnik (franku)]

LGTM :-)

Approve this, but i want to have GunChleoc to take a look at my glorious English wording ;)

Of course some one else with knowledge of English can do this also.

review: Approve (code review)

[kaputtnik (franku)]

I have removed the link to bug 1721126, because of https://bugs.launchpad.net/widelands/+bug/1721126/comments/27

[Toni Förster (stonerl)]

review: Approve (proof reading)

[Notabilis (notabilis27)]

I haven't tested it but the code is looking good, thanks.
Regarding the documentation: Please update the documentation of win_condition_functions::broadcast() since it no longer waits for roadbuilding. Also, could you add a comment in coroutines.lua (similar to ui.lua)? In multiplayer, these methods should either not been used or used for all players at the same time.

[kaputtnik (franku)]

Oh, thanks for the hint regarding broadcast(), just forgot it.

About coroutines.lua: I do not know which function is NOT used for all players at the same time.

I guess those functions are the functions called when a particular user action is triggered, and i think that are all the functions in ui.lua. So i added a comment that is according to my guess to coroutines.lua.

If there are more functions which are critical for user actions, or other things to consider, please let me know.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4418. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/485117314.
Appveyor build 4208. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1811583_desync_with_territorial-4208.

[Klaus Halfmann (klaus-halfmann)]

We played this to the End on a 3vs3 Game on some
large Map and had no problems whatsover.

This way I finally learned Tonis game-name.

Traviss had an error with apt.llvm.org port one build only.

@bunnbot merge

review: Approve (compile, testplay)

[Notabilis (notabilis27)]

Sorry for the late response. Currently, these functions are indeed called for all players in parallel within the win condition scripts. But if this would change for whichever reason, the game would desync. A point where this happened are the functions within ui.lua which call the sleep() function in coroutines.lua for only some players. It could happen that a similar construct is created at some time in the future again.
I think the comment in coroutines.lua should be something like:

Do not use these functions in multiplayer scripting (scenarios and
winconditions) for only some players of a game. Make sure they are
called for none or all players in parallel, otherwise the games will
desynchronize.

[kaputtnik (franku)]

> But if this would change for whichever reason, the game would desync.

Thats the reason why i think this is just a workaround. A proper fix would be to make the problematic functions, like wait_for_roadbuilding(), work for multiplayer games too. Don't know if this is feasible though.

Your suggestion for the note in coroutines.lua is too abstract for me, and i guess for other people also.

> Make sure they are called for none or all players in parallel,

I think most people (including me) do not now how to achieve that :-S

[Toni Förster (stonerl)]

>> Make sure they are called for none or all players in parallel,

>I think most people (including me) do not now how to achieve that :-S

Maybe an example would help?

[Toni Förster (stonerl)]

I don't think we can fix wait_for_roadbuilding(). What we could do is either remove it from send_message() so that it looks like this:

function send_message(player, title, body, parameters)
   player:send_message(title, body, parameters)
end

Or, every send_message() call needs to be called from within a coroutine:

function broadcast(plrs, header, msg, goptions)
   local options = goptions or {}
   for idx, p in ipairs(plrs) do
      run(function()
         send_message(p, header, msg, options)
      end)
   end
end

The problem here is that the send_time will differ. E.g. the message is scheduled for 30 minutes. The message would be: "Game ends in 3 hours 30 minutes" and send_time would be 30 minutes. If the User has the road building menu open, this message would be delayed until the menu is closed. Let's say the user closes the menu at the 40 minutes mark (I know, nobody would let the road building menu open for 10 minutes) the content of the message would be wrong, because the remaining time after 40 minutes is 3 hours 20 minutes.

The 3rd option is what we do right now.

[Toni Förster (stonerl)]

Fourth option would be to check for multiplayer.

Here is some pseudo-code

function wait_for_roadbuilding()
  if not multiplayer then
   _await_animation()
   while (wl.ui.MapView().is_building_road) do sleep(2000) end
  end
end

Is there a function to check for multiplayer?

[Toni Förster (stonerl)]

I have an idea but no clue how to implement this:

the lua part:

function wait_for_roadbuilding()
  local game_type = wl.Game().game_type
  if game_type = singleplayer then
   _await_animation()
   while (wl.ui.MapView().is_building_road) do sleep(2000) end
  end
end

The c++ part I got from src/wui/tribal_encyclopedia.cc

if (game->game_controller()->get_game_type() == GameController::GameType::kSingleplayer) {
    cr->push_arg("singleplayer");
} else {
    cr->push_arg("multiplayer");
}

So if someone with more knowledge could do this..

[Notabilis (notabilis27)]

You are right, my note is most likely to abstract. If I wouldn't be knowing the code I possibly wouldn't understand it either.
I like the idea of checking for single-/multiplayer game. Since I wasn't able to find a lua method for this, I guess someone has to add one for it. I can take a look at it in the next days.

[Toni Förster (stonerl)]

>I can take a look at it in the next days.

This would be great!

[kaputtnik (franku)]

Klaus, it is called bunnYbot :)

@bunnybot merge

[Toni Förster (stonerl)]

It won't merge since travis through an error. But if notabilis implements the single- multiplayer?-method we wouldn't need this branch here...

[kaputtnik (franku)]

Looks like bunnybot is in winter sleep ...

The travis error is about curl, and i would wonder if the changes would affect building widelands at all, since only some lua code is changed.

> But if notabilis implements the single- multiplayer?-method we wouldn't need this branch here...

I think we need this change in this branch to get closer to the next official release. And i think the changes are straightforward and do not change anything for the players.

Implementing a single/multiplayer switch does not affect this branch, except the documentation. It would 'only' harden the codebase against possible mistakes when using the wrong functions, imho.

I will create a new bug report to make the single/multiplayer mode accessible in lua, and merge this branch by hand.

[kaputtnik (franku)]

This is merged into trunk now.

New bug: https://bugs.launchpad.net/widelands/+bug/1814372

[Notabilis (notabilis27)]

A branch with the requested lua method is up:
https://code.launchpad.net/~widelands-dev/widelands/bug-1814372-lua-gametype/+merge/362638

=== modified file 'data/scripting/coroutine.lua'
--- data/scripting/coroutine.lua	2015-10-31 12:11:44 +0000
+++ data/scripting/coroutine.lua	2019-01-27 19:08:37 +0000
@@ -6,6 +6,11 @@
 -- of coroutines and yielding proper sleeping times to widelands. These
 -- functions are more specially tailored to widelands and take a lot of
 -- the awkwardness out of using coroutines directly.
+--
+-- .. Note:: 
+--    Do not use these functions for multiplayer scripting (scenarios and
+--    winconditions) in combination with any functions in :ref:`ui.lua`
+--
 
 -- =======================================================================
 --                             PUBLIC FUNCTIONS

=== modified file 'data/scripting/messages.lua'
--- data/scripting/messages.lua	2017-12-10 09:38:31 +0000
+++ data/scripting/messages.lua	2019-01-27 19:08:37 +0000
@@ -17,6 +17,10 @@
 --    the function waits until the player leaves the building mode
 --    before sending the message
 --
+--    Do not use this with 'popup = true' for multiplayer scenarios or
+--    winconditions, because a game will likely desync then.
+--    Use :meth:`wl.game.Player.send_message <wl.game.Player.send_message>` instead.
+--
 --    :arg player: the recipient of the message
 --    :arg title: the localized title of the message
 --    :type title: :class:`string`
@@ -39,6 +43,10 @@
 --
 --    Sends a game status message to all players.
 --
+--    Do not use this for multiplayer scenarios or winconditions, because a
+--    game will likely desync then. Use :meth:`win_condition_functions.broadcast <broadcast>`
+--    instead.
+--
 --    :arg text: the localized body of the message. You can use rt functions here.
 --    :type text: :class:`string`
 --    :arg heading: the localized title of the message (optional)

=== modified file 'data/scripting/ui.lua'
--- data/scripting/ui.lua	2017-11-26 13:34:12 +0000
+++ data/scripting/ui.lua	2019-01-27 19:08:37 +0000
@@ -1,12 +1,17 @@
 include "scripting/coroutine.lua"
 
 -- RST
+-- .. _ui.lua:
+--
 -- ui.lua
 -- ---------------
 --
 -- This script contains UI related functions like for moving the mouse or the
 -- view or clicking on fields and UI elements.
 --
+-- .. Note::
+--    Do not use any of these functions for multiplayer scenarios or winconditions,
+--    because a game will likely desync then.
 
 -- Sleep until we are done animating.
 function _await_animation()

=== modified file 'data/scripting/win_conditions/collectors.lua'
--- data/scripting/win_conditions/collectors.lua	2018-10-13 08:51:51 +0000
+++ data/scripting/win_conditions/collectors.lua	2019-01-27 19:08:37 +0000
@@ -191,9 +191,7 @@
          msg = msg .. vspace(8) .. message
       end
 
-      for idx, plr in ipairs(plrs) do
-         send_message(plr, game_status.title, msg, {popup = true})
-      end
+      broadcast(plrs, game_status.title, msg, {popup = true})
    end
 
    local function _game_over(plrs)

=== modified file 'data/scripting/win_conditions/territorial_lord.lua'
--- data/scripting/win_conditions/territorial_lord.lua	2018-12-03 20:30:01 +0000
+++ data/scripting/win_conditions/territorial_lord.lua	2019-01-27 19:08:37 +0000
@@ -50,7 +50,7 @@
                msg = msg .. losing_status_header(plrs) .. vspace(8)
             end
             msg = msg .. vspace(8) .. game_status.body .. territory_status(fields, "has")
-         send_message(player, game_status.title, msg, {popup = true})
+            player:send_message(game_status.title, msg, {popup = true})
          end
       end
 

=== modified file 'data/scripting/win_conditions/territorial_time.lua'
--- data/scripting/win_conditions/territorial_time.lua	2018-12-03 20:30:01 +0000
+++ data/scripting/win_conditions/territorial_time.lua	2019-01-27 19:08:37 +0000
@@ -68,7 +68,7 @@
                   :format(remaining_max_minutes))
             end
             msg = msg .. vspace(8) .. game_status.body .. territory_status(fields, "has")
-            send_message(player, game_status.title, msg, {popup = true})
+            player:send_message(game_status.title, msg, {popup = true})
          end
       end
 

=== modified file 'data/scripting/win_conditions/win_condition_functions.lua'
--- data/scripting/win_conditions/win_condition_functions.lua	2018-09-28 17:20:32 +0000
+++ data/scripting/win_conditions/win_condition_functions.lua	2019-01-27 19:08:37 +0000
@@ -102,13 +102,13 @@
 -- .. function:: broadcast(plrs, header, msg[, options])
 --
 --    broadcast a message to all players using
---    :meth:`~wl.game.Player.send_message`. All parameters are passed
+--    :meth:`wl.game.Player.send_message <wl.game.Player.send_message>`. All parameters are passed
 --    literally.
---    Message is delayed while player is in road building mode.
+
 function broadcast(plrs, header, msg, goptions)
    local options = goptions or {}
    for idx, p in ipairs(plrs) do
-      send_message(p, header, msg, options)
+      p:send_message(header, msg, options)
    end
 end