Merge lp:~widelands-dev/widelands/safeguard-multiplayer-messages into lp:widelands

Proposed by Toni Förster
Status: Merged
Merged at revision: 8973
Proposed branch: lp:~widelands-dev/widelands/safeguard-multiplayer-messages
Merge into: lp:widelands
Diff against target: 73 lines (+8/-14)
4 files modified
data/scripting/messages.lua (+1/-9)
data/scripting/ui.lua (+4/-2)
data/scripting/win_conditions/collectors.lua (+2/-2)
data/scripting/win_conditions/win_condition_functions.lua (+1/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/safeguard-multiplayer-messages
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+362942@code.launchpad.net

Commit message

Safeguard to avoid desyncs when using send_message() from ui.lua

Description of the change

This should make sure that we don't have desyncs when in multiplayer and sending messages. Could potentially solve some desyncs in smugglers multiplayer campaign.

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

Code LGTM :)

review: Approve
8974. By Toni Förster

use send_message() from ui.lua

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

Thanks.

And BTW: Welcome back GunChleoc

@bunnybot merge

Revision history for this message
kaputtnik (franku) wrote :

What is the reason for the change in win_condition_functions.lua?

wait_for_roadbuilding() acts now different when in multiplayer: The sentence "Sleeps while player is in roadbuilding mode." is now only valid in singleplayer games. How should this be documented?

I fear we get things messsed up if we have such constructions, at least for people doing scripting it get much more difficult.

Just my opinion :-)

Revision history for this message
GunChleoc (gunchleoc) wrote :

Add the info to the documentation, like for the send_message function which now states that it isn't multiplayer-safe.

Revision history for this message
kaputtnik (franku) wrote :

Sorry if my sentences sounds too hard...

> Add the info to the documentation, like for the send_message function which now states that it isn't multiplayer-safe.

This statement is not valid anymore, if this branch get merged. Or do i miss some thing?

Tony, this is nothing against your merge request, nor against you. But this branch solves an issue which is not really there. Or do i miss anything again? IMHO this should be targeted to build21.

Once we had a feature freeze, but now we get far away from that :(

8975. By Toni Förster

change documentation for send_message()

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

@kaputtnik

Neither do your sentences sound to hard nor rude. :-) Please let me explain why I think the changes are necessary for build_20.

send_message() from messages.lua is also used in mp_scenarios and some other win_conditions. We could either change the method calls there as well and change them back for build_21, or we fix it now.

We have a hodgepodge methods we use for sending messages ATM. It would be sensible to unify them, and use the same method for the same kind of message.

We shouldn't forget that the win_conditions are also used in single player and there it is absolutely okay to wait for a message to pop-up. The changes we did in the other branch were a hot-fix, specifically for multiplayer. But being able to distinguish between single- and multiplayer, and making this method multiplayer-aware is the way to go IMHO.

I don't think that this change interferes with the feature freeze. We could have searched through all the lua files and locate where the aforementioned method is used (there are still many left) and replace it with something like this: "player:send_message(HEADER, BODY)". But after the release of build_20, we then would have to revert all back to use the method from messages.lua.

@GunChleoc & @kaputtnik

I changed the documentation as well.

The changes in win_condition_functions just restore the old behaviour; using the method from messages.lua.

I hope my explanations sound sensible.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4444. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/491032831.
Appveyor build 4232. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_safeguard_multiplayer_messages-4232.

Revision history for this message
GunChleoc (gunchleoc) wrote :

This has a chance of fixing some desyncs for us, which is why I want it for Build 20. We do need to get rid of those desyncs.

@bunnybot merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/scripting/messages.lua'
2--- data/scripting/messages.lua 2019-01-27 10:10:29 +0000
3+++ data/scripting/messages.lua 2019-02-09 18:50:14 +0000
4@@ -15,11 +15,7 @@
5 -- Sends a message to the player.
6 -- If the popup parameter is true and the player is in building mode,
7 -- the function waits until the player leaves the building mode
8--- before sending the message
9---
10--- Do not use this with 'popup = true' for multiplayer scenarios or
11--- winconditions, because a game will likely desync then.
12--- Use :meth:`wl.game.Player.send_message <wl.game.Player.send_message>` instead.
13+-- before sending the message (only in singleplayer)
14 --
15 -- :arg player: the recipient of the message
16 -- :arg title: the localized title of the message
17@@ -43,10 +39,6 @@
18 --
19 -- Sends a game status message to all players.
20 --
21--- Do not use this for multiplayer scenarios or winconditions, because a
22--- game will likely desync then. Use :meth:`win_condition_functions.broadcast <broadcast>`
23--- instead.
24---
25 -- :arg text: the localized body of the message. You can use rt functions here.
26 -- :type text: :class:`string`
27 -- :arg heading: the localized title of the message (optional)
28
29=== modified file 'data/scripting/ui.lua'
30--- data/scripting/ui.lua 2019-01-27 18:57:30 +0000
31+++ data/scripting/ui.lua 2019-02-09 18:50:14 +0000
32@@ -181,8 +181,10 @@
33 -- Sleeps while player is in roadbuilding mode.
34 --
35 function wait_for_roadbuilding()
36- _await_animation()
37- while (wl.ui.MapView().is_building_road) do sleep(2000) end
38+ if wl.Game().type == "singleplayer" then
39+ _await_animation()
40+ while (wl.ui.MapView().is_building_road) do sleep(2000) end
41+ end
42 end
43
44
45
46=== modified file 'data/scripting/win_conditions/collectors.lua'
47--- data/scripting/win_conditions/collectors.lua 2019-02-09 07:51:55 +0000
48+++ data/scripting/win_conditions/collectors.lua 2019-02-09 18:50:14 +0000
49@@ -194,10 +194,10 @@
50 local lost_or_won = 0
51 if (info[2] < win_points) then
52 lost_or_won = 0
53- player:send_message(lost_game_over.title, lost_game_over.body)
54+ send_message(player, lost_game_over.title, lost_game_over.body)
55 else
56 lost_or_won = 1
57- player:send_message(won_game_over.title, won_game_over.body)
58+ send_message(player, won_game_over.title, won_game_over.body)
59 end
60 if (count_factions(plrs) > 1) then
61 if (player.team == 0) then
62
63=== modified file 'data/scripting/win_conditions/win_condition_functions.lua'
64--- data/scripting/win_conditions/win_condition_functions.lua 2019-02-09 07:51:55 +0000
65+++ data/scripting/win_conditions/win_condition_functions.lua 2019-02-09 18:50:14 +0000
66@@ -108,7 +108,7 @@
67 function broadcast(plrs, header, msg, goptions)
68 local options = goptions or {}
69 for idx, p in ipairs(plrs) do
70- p:send_message(header, msg, options)
71+ send_message(p, header, msg, options)
72 end
73 end
74

Subscribers

People subscribed via source and target branches

to status/vote changes: