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

Proposed by Toni Förster
Status: Merged
Merged at revision: 8961
Proposed branch: lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial
Merge into: lp:widelands
Diff against target: 124 lines (+24/-8)
7 files modified
data/scripting/coroutine.lua (+5/-0)
data/scripting/messages.lua (+8/-0)
data/scripting/ui.lua (+5/-0)
data/scripting/win_conditions/collectors.lua (+1/-3)
data/scripting/win_conditions/territorial_lord.lua (+1/-1)
data/scripting/win_conditions/territorial_time.lua (+1/-1)
data/scripting/win_conditions/win_condition_functions.lua (+3/-3)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial
Reviewer Review Type Date Requested Status
Klaus Halfmann compile, testplay Approve
Toni Förster proof reading Approve
kaputtnik (community) code review Approve
GunChleoc Pending
Review via email: mp+362272@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
kaputtnik (franku) wrote :

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
Revision history for this message
Toni Förster (stonerl) wrote :

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.

Revision history for this message
kaputtnik (franku) wrote :

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

Revision history for this message
Toni Förster (stonerl) wrote :

You're right... fixed it.

8962. By Toni Förster

don't use broadcast() for territorial

Revision history for this message
Toni Förster (stonerl) :
review: Needs Resubmitting
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

8963. By kaputtnik

mention in RST comments not to use some functiones because of desyncs

Revision history for this message
kaputtnik (franku) wrote :

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)
Revision history for this message
kaputtnik (franku) wrote :
Revision history for this message
Toni Förster (stonerl) :
review: Approve (proof reading)
Revision history for this message
Notabilis (notabilis27) wrote :

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.

8964. By kaputtnik

addressed comments review

8965. By kaputtnik

added a note to coroutines.lua

Revision history for this message
kaputtnik (franku) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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)
Revision history for this message
Notabilis (notabilis27) wrote :

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.

Revision history for this message
kaputtnik (franku) wrote :

> 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

Revision history for this message
Toni Förster (stonerl) wrote :

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

Revision history for this message
Toni Förster (stonerl) wrote :

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.

Revision history for this message
Toni Förster (stonerl) wrote :

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?

Revision history for this message
Toni Förster (stonerl) wrote :

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

Revision history for this message
Notabilis (notabilis27) wrote :

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.

Revision history for this message
Toni Förster (stonerl) wrote :

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

This would be great!

Revision history for this message
kaputtnik (franku) wrote :

Klaus, it is called bunnYbot :)

@bunnybot merge

Revision history for this message
Toni Förster (stonerl) wrote :

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

Revision history for this message
kaputtnik (franku) wrote :

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.

Revision history for this message
kaputtnik (franku) wrote :

This is merged into trunk now.

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

Revision history for this message
Notabilis (notabilis27) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/scripting/coroutine.lua'
2--- data/scripting/coroutine.lua 2015-10-31 12:11:44 +0000
3+++ data/scripting/coroutine.lua 2019-01-27 19:08:37 +0000
4@@ -6,6 +6,11 @@
5 -- of coroutines and yielding proper sleeping times to widelands. These
6 -- functions are more specially tailored to widelands and take a lot of
7 -- the awkwardness out of using coroutines directly.
8+--
9+-- .. Note::
10+-- Do not use these functions for multiplayer scripting (scenarios and
11+-- winconditions) in combination with any functions in :ref:`ui.lua`
12+--
13
14 -- =======================================================================
15 -- PUBLIC FUNCTIONS
16
17=== modified file 'data/scripting/messages.lua'
18--- data/scripting/messages.lua 2017-12-10 09:38:31 +0000
19+++ data/scripting/messages.lua 2019-01-27 19:08:37 +0000
20@@ -17,6 +17,10 @@
21 -- the function waits until the player leaves the building mode
22 -- before sending the message
23 --
24+-- Do not use this with 'popup = true' for multiplayer scenarios or
25+-- winconditions, because a game will likely desync then.
26+-- Use :meth:`wl.game.Player.send_message <wl.game.Player.send_message>` instead.
27+--
28 -- :arg player: the recipient of the message
29 -- :arg title: the localized title of the message
30 -- :type title: :class:`string`
31@@ -39,6 +43,10 @@
32 --
33 -- Sends a game status message to all players.
34 --
35+-- Do not use this for multiplayer scenarios or winconditions, because a
36+-- game will likely desync then. Use :meth:`win_condition_functions.broadcast <broadcast>`
37+-- instead.
38+--
39 -- :arg text: the localized body of the message. You can use rt functions here.
40 -- :type text: :class:`string`
41 -- :arg heading: the localized title of the message (optional)
42
43=== modified file 'data/scripting/ui.lua'
44--- data/scripting/ui.lua 2017-11-26 13:34:12 +0000
45+++ data/scripting/ui.lua 2019-01-27 19:08:37 +0000
46@@ -1,12 +1,17 @@
47 include "scripting/coroutine.lua"
48
49 -- RST
50+-- .. _ui.lua:
51+--
52 -- ui.lua
53 -- ---------------
54 --
55 -- This script contains UI related functions like for moving the mouse or the
56 -- view or clicking on fields and UI elements.
57 --
58+-- .. Note::
59+-- Do not use any of these functions for multiplayer scenarios or winconditions,
60+-- because a game will likely desync then.
61
62 -- Sleep until we are done animating.
63 function _await_animation()
64
65=== modified file 'data/scripting/win_conditions/collectors.lua'
66--- data/scripting/win_conditions/collectors.lua 2018-10-13 08:51:51 +0000
67+++ data/scripting/win_conditions/collectors.lua 2019-01-27 19:08:37 +0000
68@@ -191,9 +191,7 @@
69 msg = msg .. vspace(8) .. message
70 end
71
72- for idx, plr in ipairs(plrs) do
73- send_message(plr, game_status.title, msg, {popup = true})
74- end
75+ broadcast(plrs, game_status.title, msg, {popup = true})
76 end
77
78 local function _game_over(plrs)
79
80=== modified file 'data/scripting/win_conditions/territorial_lord.lua'
81--- data/scripting/win_conditions/territorial_lord.lua 2018-12-03 20:30:01 +0000
82+++ data/scripting/win_conditions/territorial_lord.lua 2019-01-27 19:08:37 +0000
83@@ -50,7 +50,7 @@
84 msg = msg .. losing_status_header(plrs) .. vspace(8)
85 end
86 msg = msg .. vspace(8) .. game_status.body .. territory_status(fields, "has")
87- send_message(player, game_status.title, msg, {popup = true})
88+ player:send_message(game_status.title, msg, {popup = true})
89 end
90 end
91
92
93=== modified file 'data/scripting/win_conditions/territorial_time.lua'
94--- data/scripting/win_conditions/territorial_time.lua 2018-12-03 20:30:01 +0000
95+++ data/scripting/win_conditions/territorial_time.lua 2019-01-27 19:08:37 +0000
96@@ -68,7 +68,7 @@
97 :format(remaining_max_minutes))
98 end
99 msg = msg .. vspace(8) .. game_status.body .. territory_status(fields, "has")
100- send_message(player, game_status.title, msg, {popup = true})
101+ player:send_message(game_status.title, msg, {popup = true})
102 end
103 end
104
105
106=== modified file 'data/scripting/win_conditions/win_condition_functions.lua'
107--- data/scripting/win_conditions/win_condition_functions.lua 2018-09-28 17:20:32 +0000
108+++ data/scripting/win_conditions/win_condition_functions.lua 2019-01-27 19:08:37 +0000
109@@ -102,13 +102,13 @@
110 -- .. function:: broadcast(plrs, header, msg[, options])
111 --
112 -- broadcast a message to all players using
113--- :meth:`~wl.game.Player.send_message`. All parameters are passed
114+-- :meth:`wl.game.Player.send_message <wl.game.Player.send_message>`. All parameters are passed
115 -- literally.
116--- Message is delayed while player is in road building mode.
117+
118 function broadcast(plrs, header, msg, goptions)
119 local options = goptions or {}
120 for idx, p in ipairs(plrs) do
121- send_message(p, header, msg, options)
122+ p:send_message(header, msg, options)
123 end
124 end
125

Subscribers

People subscribed via source and target branches

to status/vote changes: