Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/rework-resource-indicators into lp:widelands
Review: Needs Fixing I have done some in-game testing now - the code is almost ready, but still needs some tweaking. I'll add my comments for the graphics to the bug. Code review of the C++ bits to follow. Barbarian scenario 1 is fixed correctly, but the indicators are huge. They are also huge in the Encyclopedia. Please make them to scale - we can have bigger ones once we have mipmaps, which are pretty much ready but targeted for Build 21. https://code.launchpad.net/~widelands-dev/widelands/mipmaps I have fixed the seafaring tutorial. In-game help for mines is broken. I have fixed it for the Atlanteans, but the Barbarians don't follow the naming scheme. We can bzr mv the pictures, but I think the better solution would be to get the immovables by name and then get their representative_image. Please rename the "png" folder to "pics" for consistency and fix the scripts accordingly. May sound like nitpicking, but it does make the file structure easier to navigate ;) -- https://code.launchpad.net/~widelands-dev/widelands/rework-resource-indicators/+merge/353996 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/rework-resource-indicators. ___ 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/rework-resource-indicators into lp:widelands
Code review on the diff done - see comments.
Diff comments:
> === modified file 'data/tribes/atlanteans.lua'
> --- data/tribes/atlanteans.lua2017-11-23 09:13:06 +
> +++ data/tribes/atlanteans.lua2018-09-01 08:09:17 +
> @@ -73,6 +75,31 @@
>},
> },
>
> + resource_indicators = {
> + [""] = {
Please call this "none" for consistency
> + [0] = "atlanteans_resi_none",
> + },
> + coal = {
> + [10] = "atlanteans_resi_coal_1",
> + [20] = "atlanteans_resi_coal_2",
> + },
> + iron = {
> + [10] = "atlanteans_resi_iron_1",
> + [20] = "atlanteans_resi_iron_2",
> + },
> + gold = {
> + [10] = "atlanteans_resi_gold_1",
> + [20] = "atlanteans_resi_gold_2",
> + },
> + stones = {
> + [10] = "atlanteans_resi_stones_1",
> + [20] = "atlanteans_resi_stones_2",
> + },
> + water = {
> + [100] = "atlanteans_resi_water",
> + },
> + },
> +
> -- Wares positions in wares windows.
> -- This also gives us the information which wares the tribe uses.
> -- Each subtable is a column in the wares windows.
>
> === added file 'data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua
> 1970-01-01 00:00:00 +
> +++ data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua
> 2018-09-01 08:09:17 +
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the barbarian and empire helptexts
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings.
> They can be dug up by a crystal mine. You will also get granite from the
> mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are only a few precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a
> granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is only a little bit of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a
> marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is only a little bit of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua'
> --- data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua
> 1970-01-01 00:00:00 +
> +++ data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua
> 2018-09-01 08:09:17 +
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the barbarian and empire helptexts
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings.
> They can be dug up by a crystal mine. You will also get granite from the
> mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are many precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a
> granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is a lot of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a
> marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is a lot of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/barbarians/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/rework-resource-indicators into lp:widelands
Implemented most of your comments, and changed the graphics. Didn´t touch the atlantean images yet, will do so now. I disagree about hardcoding the none-resi as "none". Hardcoded name restrictions are always a bad thing; it should be possible for a future modder to create a new resource named "none" if they want. Better not to have any hardcoded names and use the only string that makes no sense at all as a resource name, "". -- https://code.launchpad.net/~widelands-dev/widelands/rework-resource-indicators/+merge/353996 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/rework-resource-indicators. ___ 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/rework-resource-indicators into lp:widelands
Review: Resubmit Remaining graphics changed. Please test again. -- https://code.launchpad.net/~widelands-dev/widelands/rework-resource-indicators/+merge/353996 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/rework-resource-indicators. ___ 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/frisian-buildings into lp:widelands
Benedikt Straub has proposed merging lp:~widelands-dev/widelands/frisian-buildings into lp:widelands. Commit message: New graphics for the frisian tower, fortress, port, headquarters, and reindeer farm Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1764606 in widelands: "Frisian building models are too dark" https://bugs.launchpad.net/widelands/+bug/1764606 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/354156 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian-buildings into lp:widelands. === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_00.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_00.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_00.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_00_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_00_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_00_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_01.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_01.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_01.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_01_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_01_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_01_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_02.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_02.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_02.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_02_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_02_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_02_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_03.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_03.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_03.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_03_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_03_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_03_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_04.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_04.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_04.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_04_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_04_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_04_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_05.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_05.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_05.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_05_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_05_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_05_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_06.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_06.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_06.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_06_pc.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_06_pc.png 2018-07-18 19:41:40 + and data/tribes/buildings/militarysites/frisians/fortress/idle_06_pc.png 2018-09-01 13:42:23 + differ === modified file 'data/tribes/buildings/militarysites/frisians/fortress/idle_07.png' Binary files data/tribes/buildings/militarysites/frisians/fortress/idle_07.png 201
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1787105_tada_edited_sounds into lp:widelands
I am happy with the sounds, let's have them. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1787105_tada_edited_sounds/+merge/353216 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1787105_tada_edited_sounds 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_bug_fix_enhance into lp:widelands
I have played the peaceful path, and I have also let the Barbarian sack me after first contact with the temple. all went well, so this can go in :) I noticed one more thing: when a player doesn't listen to instructions and doesn't click on an unproductive farm when told to do so, there's no reminder in the objectives. Add an objective for that? Please feel free to merge this branch without it :) -- https://code.launchpad.net/~widelands-dev/widelands/empire04_bug_fix_enhance/+merge/353742 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/empire04_bug_fix_enhance 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian-buildings into lp:widelands
Looking good :) Comparison screenshot: https://launchpadlibrarian.net/386353592/frisians_comparison.png The only thing that's a bit weird there is that the text for the woodcutter is on top of the steeple for the headquarters (I used https://code.launchpad.net/~widelands-dev/widelands/bug-1480927-building-texts/+merge/353728 as a code base for that; in trunk the text will disappear behind the steeple). I see no easy way around this problem - maybe try putting the steeple in front of the building to see how that looks? -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/354156 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian-buildings 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/bug-1787105_tada_edited_sounds into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1787105_tada_edited_sounds into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1787105_tada_edited_sounds/+merge/353216 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1787105_tada_edited_sounds 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian-buildings into lp:widelands
Putting it in front wouldn´t look good unless I redesigned the rest of the building as well. I shifted the steeple as far east as aesthetically possible, does this fix the issue? -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/354156 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian-buildings 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/frisian-buildings into lp:widelands
Continuous integration builds have changed state: Travis build 3873. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/423431595. Appveyor build 3671. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_frisian_buildings-3671. -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/354156 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian-buildings 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian-buildings into lp:widelands
That solves the problem, but it's less aesthetically pleasing. How about a small steeple growing out of the center of the roof? Or a design like this: https://ozoutback.com.au/the_Netherlands/cfryslan/slides/2005070105.jpg -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/354156 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian-buildings 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/rework-resource-indicators into lp:widelands
For fixing the help, add definitions for the icon to the init.lua, like this: icon = dirname .. "coal.png", You can then reference it via immovable_description.icon_name Maybe we should use the same trick in the Barbarian scenario too, that will make it more resistant against changes in the implementation details. -- https://code.launchpad.net/~widelands-dev/widelands/rework-resource-indicators/+merge/353996 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/rework-resource-indicators. ___ 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/ferry into lp:widelands
Continuous integration builds have changed state: Travis build 3874. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/423456112. Appveyor build 3672. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ferry-3672. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ferry 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
