[Widelands-dev] [Merge] lp:~widelands-dev/widelands/collectors_notification into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/collectors_notification into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
I have wanted this unified for a long time. Thanks for taking it on! -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Just a Homebrew download issue. @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Well then :-) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Not sure whether you are waiting for me with this. As far as I am concerned, your (new) changes are fine and this can be merged. -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Continuous integration builds have changed state: Travis build 4398. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/478872308. Appveyor build 4189. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_collectors_notification-4189. -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Continuous integration builds have changed state: Travis build 4390. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/477473643. Appveyor build 4181. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_collectors_notification-4181. -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
I'm going to add more info to the notification_remaining_time() function.
Since I wanted this to be in place for build 20, I'm going to address
count_factions() later. I'd like to unify more code, were possible. But this
would also involve some changes to strings.
Diff comments:
> === modified file 'data/scripting/win_conditions/collectors.lua'
> --- data/scripting/win_conditions/collectors.lua 2018-10-13 08:51:51
> +
> +++ data/scripting/win_conditions/collectors.lua 2019-01-02 13:47:40
> +
> @@ -28,7 +28,11 @@
>
> -- set the objective with the game type for all players
> broadcast_objective("win_condition", wc_descname, wc_desc)
> -
> +
> + -- set the maximum game time of 4 hours
> + local max_time = 4 * 60
> +
> + local game = wl.Game()
> local plrs = wl.Game().players
I thought about this but the same variable is used in wood gnome & territorial,
so I'd like to keep this as uniform as possible.
> local teams = {}
> for idx,plr in ipairs(plrs) do
> @@ -221,10 +199,14 @@
> lost_or_won = 1
> player:send_message(won_game_over.title, won_game_over.body)
> end
> - if (player.team == 0) then
> -wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> + if (count_factions(plrs) > 1) then
Going to do this in the next iteration.
> +if (player.team == 0) then
> + wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> +else
> + wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[3],
> team_score=info[2]}))
> +end
> else
> -wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[3],
> team_score=info[2]}))
> +wl.game.report_result(player, lost_or_won)
> end
>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 +
> +++ data/scripting/win_conditions/win_condition_functions.lua 2019-01-02
> 13:47:40 +
> @@ -237,3 +237,59 @@
> table.sort(ranked_players_and_teams, function(a,b) return a["points"] >
> b["points"] end)
> return ranked_players_and_teams
> end
> +
> +-- RST
> +-- .. function:: format_remaining_time(remaining_time)
> +--
> +--return a message that contains the remaining game time
> +--to be used when sending status meassages
> +--
> +--:arg remaining_time:The remaining game time in minutes
> +function format_remaining_time(remaining_time)
> + local h = 0
> + local m = 60
> + local time = ""
> + set_textdomain("win_conditions")
> +
> + if (remaining_time ~= 60) then
> + h = math.floor(remaining_time / 60)
> + m = remaining_time % 60
> + end
> +
> + if ((h > 0) and (m > 0)) then
> + -- TRANSLATORS: Context: 'The game will end in 2 hours and 30 minutes.'
> + time = (ngettext("%1% hour and %2% minutes", "%1% hours and %2%
> minutes", h, m)):bformat(h, m)
> + elseif m > 0 then
> + -- TRANSLATORS: Context: 'The game will end in 30 minutes.'
> + time = (ngettext("%i minute", "%i minutes", m)):format(m)
> + else
> + -- TRANSLATORS: Context: 'The game will end in 2 hours.'
> + time = (ngettext("%1% hour", "%1% hours", h)):bformat(h)
> + end
> + -- TRANSLATORS: Context: 'The game will end in (2 hours and) 30 minutes.'
> + return p(_"The game will end in %s."):format(time)
> +end
> +
> +-- RST
> +-- .. function:: notification_remaining_time(max_time)
> +--
> +--calculate the remaining game time for notifications
> +--returns the remaining time and whether the notification should popup
> +--to be used when sending status meassages
> +--status messages are to be send every 30 minutes and every 5 during the
> last 30 minutes
> +--the message window pops up ever hour 30, 20 & 10 minutes berfore the
> game ends.
Will do so.
> +--
> +--:arg max_time:The time maximum game time in minutes
> +function notification_remaining_time(max_time, remaining_time)
> + local show_popup = false
> + if (wl.Game().time < ((max_time - 30) * 60 * 1000)) then --
> + wake_me(wl.Game().time + (30 * 60 * 1000)) -- 30 minutes
> + remaining_time = remaining_time - 30
> + if (remaining_time % 60 == 0) or (remaining_time == 30) then
> show_popup = true end
> + else
> + wake_me(wl.Game().time + (300 * 1000)) --5 Minutes
> + remaining_time = remaining_time - 5
> + if ((remaining_time ~= 0) and (remaining_time % 10 == 0)) then
> show_popup = true end
> + end
> + return remaining_time, show_popup
> +end
> \ No newline at end of file
--
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/collectors_notification into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/collectors_notification into lp:widelands has been updated. Commit message changed to: - unified some functions for wood_gnome and collectors - removed duplicated code and moved it to win_condition_functions - notify only every hour, 10 minutes in the last 30 - move _game_over function out of the main loop. - don't show points when game ended by military defeat - main loop ends when time is over or only one faction is left - check every second for defeated players For more details, see: https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification. ___ 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/collectors_notification into lp:widelands
Review: Approve diff, testing
Code is looking good and it seems to work as intended. A few possible
improvements are in the diff.
I assume it is intentional that there is no message on game start?
In the commit/merge message of this branch you could add that wood gnome is
also affected.
Diff comments:
> === modified file 'data/scripting/win_conditions/collectors.lua'
> --- data/scripting/win_conditions/collectors.lua 2018-10-13 08:51:51
> +
> +++ data/scripting/win_conditions/collectors.lua 2019-01-02 13:47:40
> +
> @@ -28,7 +28,11 @@
>
> -- set the objective with the game type for all players
> broadcast_objective("win_condition", wc_descname, wc_desc)
> -
> +
> + -- set the maximum game time of 4 hours
> + local max_time = 4 * 60
> +
> + local game = wl.Game()
> local plrs = wl.Game().players
Suggestion: Use new "game" variable
> local teams = {}
> for idx,plr in ipairs(plrs) do
> @@ -221,10 +199,14 @@
> lost_or_won = 1
> player:send_message(won_game_over.title, won_game_over.body)
> end
> - if (player.team == 0) then
> -wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> + if (count_factions(plrs) > 1) then
Suggestion: Call count_factions() only once before the loop?
> +if (player.team == 0) then
> + wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> +else
> + wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[3],
> team_score=info[2]}))
> +end
> else
> -wl.game.report_result(player, lost_or_won,
> make_extra_data(player, wc_descname, wc_version, {score=info[3],
> team_score=info[2]}))
> +wl.game.report_result(player, lost_or_won)
> end
>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 +
> +++ data/scripting/win_conditions/win_condition_functions.lua 2019-01-02
> 13:47:40 +
> @@ -237,3 +237,59 @@
> table.sort(ranked_players_and_teams, function(a,b) return a["points"] >
> b["points"] end)
> return ranked_players_and_teams
> end
> +
> +-- RST
> +-- .. function:: format_remaining_time(remaining_time)
> +--
> +--return a message that contains the remaining game time
> +--to be used when sending status meassages
> +--
> +--:arg remaining_time:The remaining game time in minutes
> +function format_remaining_time(remaining_time)
> + local h = 0
> + local m = 60
> + local time = ""
> + set_textdomain("win_conditions")
> +
> + if (remaining_time ~= 60) then
> + h = math.floor(remaining_time / 60)
> + m = remaining_time % 60
> + end
> +
> + if ((h > 0) and (m > 0)) then
> + -- TRANSLATORS: Context: 'The game will end in 2 hours and 30 minutes.'
> + time = (ngettext("%1% hour and %2% minutes", "%1% hours and %2%
> minutes", h, m)):bformat(h, m)
> + elseif m > 0 then
> + -- TRANSLATORS: Context: 'The game will end in 30 minutes.'
> + time = (ngettext("%i minute", "%i minutes", m)):format(m)
> + else
> + -- TRANSLATORS: Context: 'The game will end in 2 hours.'
> + time = (ngettext("%1% hour", "%1% hours", h)):bformat(h)
> + end
> + -- TRANSLATORS: Context: 'The game will end in (2 hours and) 30 minutes.'
> + return p(_"The game will end in %s."):format(time)
> +end
> +
> +-- RST
> +-- .. function:: notification_remaining_time(max_time)
> +--
> +--calculate the remaining game time for notifications
> +--returns the remaining time and whether the notification should popup
> +--to be used when sending status meassages
Typo "messages"
> +--status messages are to be send every 30 minutes and every 5 during the
> last 30 minutes
> +--the message window pops up ever hour 30, 20 & 10 minutes berfore the
> game ends.
If I understand it right this method should only be called within coroutines
and is blocking them. Add a commend about that to the documentation? Currently
it reads as if this method only calculates something.
> +--
> +--:arg max_time:The time maximum game time in minutes
> +function notification_remaining_time(max_time, remaining_time)
> + local show_popup = false
> + if (wl.Game().time < ((max_time - 30) * 60 * 1000)) then --
> + wake_me(wl.Game().time + (30 * 60 * 1000)) -- 30 minutes
> + remaining_time = remaining_time - 30
> + if (remaining_time % 60 == 0) or (remaining_time == 30) then
> show_popup = true end
> + else
> + wake_me(wl.Game().time + (300 * 1000)) --5 Minutes
> + remaining_time = remaining_time - 5
> + if ((remaining_time ~= 0) and (remaining_time % 10 == 0)) then
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/collectors_notification into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/collectors_notification into lp:widelands has been updated. Commit message changed to: Collectors Mode: - Notify only ever 30 minutes, 5 minutes in the last 30 - Move _game_over function out of the main loop. - Don't show points when game ended by military defeat - main loop ends when time is over or only one faction is left - Check every second for defeated players For more details, see: https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/collectors_notification 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/collectors_notification into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/collectors_notification into lp:widelands has been updated. Commit message changed to: Collectors Mode: - Notify only ever 30 minutes, 5 minutes in the last 30 - Move _game_over function out of the main loop. - Don't show points when game ended by military defeat - main loop ends when time is over or only one faction is left - Check every 5 seconds for defeated players For more details, see: https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/collectors_notification 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/collectors_notification into lp:widelands
Continuous integration builds have changed state: Travis build 4372. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/473313790. Appveyor build 4165. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_collectors_notification-4165. -- https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/collectors_notification 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/collectors_notification into lp:widelands
Toni Förster has proposed merging lp:~widelands-dev/widelands/collectors_notification into lp:widelands. Commit message: Collectors Mode: Send notification only ever 30 minutes; end game early if no factions are left Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1808634 in widelands: "Wood Gnome & Collectores need to adapt Territorial Time changes" https://bugs.launchpad.net/widelands/+bug/1808634 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334 Currently notifications are sent every 10 minutes, which is quite annoying. With this change it only sends them every 30 minutes and during the last 30 minutes every 5. Also, if a faction has been defeated the game ends immediately. -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/collectors_notification 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
