Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands
A branch with the requested lua method is up: https://code.launchpad.net/~widelands-dev/widelands/bug-1814372-lua-gametype/+merge/362638 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
This is merged into trunk now. New bug: https://bugs.launchpad.net/widelands/+bug/1814372 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
It won't merge since travis through an error. But if notabilis implements the single- multiplayer?-method we wouldn't need this branch here... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Klaus, it is called bunnYbot :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
>I can take a look at it in the next days. This would be great! -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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..
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.
___
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/bug-1811583-desync-with-territorial into lp:widelands
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? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.
___
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/bug-1811583-desync-with-territorial into lp:widelands
>> 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? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
> 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 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Review: Approve compile, testplay 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 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Review: Approve proof reading -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
I have removed the link to bug 1721126, because of https://bugs.launchpad.net/widelands/+bug/1721126/comments/27 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Review: Approve code review 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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Review: Resubmit -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
You're right... fixed it. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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 :-) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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/bug-1811583-desync-with-territorial into lp:widelands
Review: Needs Fixing 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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial. ___ 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
