[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands
Janosch Peters has proposed merging
lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1638394 in widelands: "Crash on rendering roads"
https://bugs.launchpad.net/widelands/+bug/1638394
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1638394-render-road/+merge/310715
If the owner of the starting field of the road is not visible, the owner of the
end field of the road is used to determine the road texture in
RoadProgram::add_road.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands.
=== modified file 'src/graphic/gl/road_program.cc'
--- src/graphic/gl/road_program.cc 2016-10-24 20:07:22 +
+++ src/graphic/gl/road_program.cc 2016-11-13 08:26:42 +
@@ -72,12 +72,17 @@
const float road_thickness_x = (-delta_y / vector_length) * kRoadThicknessInPixels * scale;
const float road_thickness_y = (delta_x / vector_length) * kRoadThicknessInPixels * scale;
- assert(start.owner != nullptr);
+ assert(start.owner != nullptr || end.owner != nullptr);
+
+ Widelands::Player* visible_owner = start.owner;
+ if (start.owner == nullptr) {
+ visible_owner = end.owner;
+ }
+
const Image& texture =
- road_type == Widelands::RoadType::kNormal ?
- start.owner->tribe().road_textures().get_normal_texture(
- start.geometric_coords, direction) :
- start.owner->tribe().road_textures().get_busy_texture(start.geometric_coords, direction);
+ road_type == Widelands::RoadType::kNormal ?
+ visible_owner->tribe().road_textures().get_normal_texture(start.geometric_coords, direction) :
+ visible_owner->tribe().road_textures().get_busy_texture(start.geometric_coords, direction);
if (*gl_texture == 0) {
*gl_texture = texture.blit_data().texture_id;
}
___
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-1638394-render-road into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1638394-render-road into lp:widelands has been updated. Commit Message changed to: Fix a crash with rendering roads in previously visible areas: If the owner of the starting field of the road is not visible, the owner of the end field of the road is used to determine the road texture in RoadProgram::add_road. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1638394-render-road/+merge/310715 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1638394-render-road 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/sphinx-buildings into lp:widelands
I need this for other branches, so I'm gonna merge. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/sphinx-buildings/+merge/308704 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/sphinx-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/sphinx-buildings into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/sphinx-buildings into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/sphinx-buildings/+merge/308704 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/sphinx-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:~7010622-q/widelands/topple-seafaring-1 into lp:widelands
I tested my proposed logic on a "one-world" island and it turned out the expedition found portspaces in secondary access although it doesn't per-se perform secondary island circling. This is because the map is a torus and the ship is reflected only onto the island again under various angles and situations. So it works! Regarding your considerations, however, I would suggest a repetition probability of 25% (instead of 0). In this manner we could safely implement the improvements implied and work on more profound revisions later on. Would that find your agreement? -- https://code.launchpad.net/~7010622-q/widelands/topple-seafaring-1/+merge/310436 Your team Widelands Developers is requested to review the proposed merge of lp:~7010622-q/widelands/topple-seafaring-1 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:~7010622-q/widelands/topple-seafaring-1 into lp:widelands
Btw, is the C++ system applied for Widelands using garbage collection? I looked through some code in defaultai.cc and didn't find any free-memory operations so far, e.g. after local list creations. -- https://code.launchpad.net/~7010622-q/widelands/topple-seafaring-1/+merge/310436 Your team Widelands Developers is requested to review the proposed merge of lp:~7010622-q/widelands/topple-seafaring-1 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/animation_scaling into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/animation_scaling
into lp:widelands.
Commit message:
Implemented scaling for animations. Bigger idle textures for Barbarian
warehouse and shipyard.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718
This implements scaling for animations, so it will look nice when zoomed in.
I am having problems with the roof textures in the Blender models, but the
warehouse and shipyard look good enough for testing.
We will need spritemaps before we convert the whole lot, otherwise our file
size will explode - but that's for another branch.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/animation_scaling into lp:widelands.
=== modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png'
Binary files data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png 2014-12-13 20:54:34 + and data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png 2016-11-13 16:57:34 + differ
=== modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png'
Binary files data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png 2014-12-13 20:54:34 + and data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png 2016-11-13 16:57:34 + differ
=== modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/init.lua'
--- data/tribes/buildings/productionsites/barbarians/shipyard/init.lua 2016-09-03 14:59:10 +
+++ data/tribes/buildings/productionsites/barbarians/shipyard/init.lua 2016-11-13 16:57:34 +
@@ -7,6 +7,7 @@
descname = pgettext("barbarians_building", "Shipyard"),
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
+ representative_image = dirname .. "representative_image.png",
size = "medium",
needs_seafaring = true,
@@ -26,6 +27,7 @@
idle = {
pictures = path.list_files(dirname .. "idle_??.png"),
hotspot = { 62, 48 },
+ scale = 3.26
},
build = {
pictures = path.list_files(dirname .. "build_??.png"),
=== added file 'data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png'
Binary files data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png 1970-01-01 00:00:00 + and data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png 2016-11-13 16:57:34 + differ
=== modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png'
Binary files data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png 2015-02-05 17:05:45 + and data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png 2016-11-13 16:57:34 + differ
=== modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png'
Binary files data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png 2015-02-05 17:05:45 + and data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png 2016-11-13 16:57:34 + differ
=== modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/init.lua'
--- data/tribes/buildings/warehouses/barbarians/warehouse/init.lua 2015-12-28 21:53:11 +
+++ data/tribes/buildings/warehouses/barbarians/warehouse/init.lua 2016-11-13 16:57:34 +
@@ -7,6 +7,7 @@
descname = pgettext("barbarians_building", "Warehouse"),
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
+ representative_image = dirname .. "representative_image.png",
size = "medium",
buildcost = {
@@ -26,7 +27,8 @@
animations = {
idle = {
pictures = path.list_files(dirname .. "idle_??.png"),
- hotspot = { 60, 78 }
+ hotspot = { 60, 78 },
+ scale = 3.62
},
build = {
pictures = path.list_files(dirname .. "build_??.png"),
=== added file 'data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png'
Binary files data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png 1970-01-01 00:00:00 + and data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png 2016-11-13 16:57:34 + differ
=== modified file 'doc/sphinx/source/animations.rst'
--- doc/sphinx/source/animations.rst 2016-10-14 07:02:10 +
+++ doc/sphinx/source/animations.rst 2016-11-13 16:57:34 +
@@ -7,6 +7,7 @@
idle = {
pictures = path.list_files(path.dirname(__file__) .. "idle_??.png"),
hotspot = { 5, 7 },
+ scale = 2.5,
fps = 4,
sound_effect = {
directory = "sound/foo",
@@ -30,5 +31,9 @@
fps
*Optional*. The frames per second for this animation if you want to deviate from the default fps. Do not specify this value if you have only 1 animation frame
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
This looks very promising! -- https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling 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/fsmenu_fullscreen_2_about into lp:widelands
Review: Approve @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/fsmenu_fullscreen_2_about/+merge/309754 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fsmenu_fullscreen_2_about. ___ 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/fsmenu_fullscreen_2_about into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/fsmenu_fullscreen_2_about into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/fsmenu_fullscreen_2_about/+merge/309754 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fsmenu_fullscreen_2_about. ___ 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-1624216-horsepocalypse into lp:widelands
It could be coded a bit more elegantly(see my comments), but apart from that it
LGTM. However, I did not test it because I dont know a straight forward way to
get a warehous with 500+ workers.
Diff comments:
> === modified file 'src/logic/map_objects/tribes/warehouse.cc'
> --- src/logic/map_objects/tribes/warehouse.cc 2016-08-04 15:49:05 +
> +++ src/logic/map_objects/tribes/warehouse.cc 2016-11-07 19:19:08 +
> @@ -528,13 +529,24 @@
> if (upcast(Game, game, &egbase)) {
> const WareList& workers = get_workers();
> for (DescriptionIndex id = 0; id < workers.get_nrwareids();
> ++id) {
> - const uint32_t stock = workers.stock(id);
> + Quantity stock = workers.stock(id);
> // Separate behaviour for the case of loading the game
> // (which does save/destroy/reload) and simply
> destroying ingame
> if (game->is_loaded()) {
> // This game is really running
> - for (uint32_t i = 0; i < stock; ++i) {
> - launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + Quantity worker_counter = 0;
I dont see why we need "worker_counter" and "i". I suggest to initialize
"worker_counter" within for() and get rid of "i"
> + for (Quantity i = 0; i < stock; ++i,
> ++worker_counter) {
> + // Make sure that we won't flood the
> map with carriers etc.
> + if (worker_counter < kFleeingUnitsCap) {
This condition - which is part of the exit condition - should be in the for
statement and not in the loop. E.g. sth like
for (Quantity worker_counter = 0; worker_counter < stock && worker_counter <
kFleeingUnitsCap; ++worker_counter).
If implemented like this we dont need the break statement.
> + launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + } else {
> + break;
> + }
> + }
> + // Remove the remaining stock in case we hit
> the cap
> + stock = workers.stock(id);
> + if (stock > 0) {
> + remove_workers(id, stock);
> }
> assert(!incorporated_workers_.count(id) ||
> incorporated_workers_[id].empty());
> } else {
If we had a method remove_all_workers we wouldnt need to differntiate between
load-game and in-game. We could just remove all workers after the
if(game->is_loaded()) condition. So IMHO the cleanest solution would be to
implement this method, but I understand if you dont want to do this. Its not
nessecary to fix this bug.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1624216-horsepocalypse/+merge/310221
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1624216-horsepocalypse 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/animation_scaling into lp:widelands
Continuous integration builds have changed state: Travis build 1601. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175510561. Appveyor build 1439. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_animation_scaling-1439. -- https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling 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/oars_appdata into lp:widelands
Review: Needs Fixing LBTM. This is invalid XML, you have multiple root tags. The has to be a child of . You can use the appstream package of your distribution to validate appstream files. E.g: appstreamcli validate debian/widelands.appdata.xml It throws a lot of errors, even before your change but you can still use it to see if new validation errors where introduced. -- https://code.launchpad.net/~widelands-dev/widelands/oars_appdata/+merge/309624 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/oars_appdata. ___ 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/casern_workersqueue into lp:widelands
Thanks for the answers earlier. Wares- and WorkerQueues now have a common interface so they can be used interchangeable. Note that the code is not really tested yet. Both are using the same gui class for display, list on the first tab. Support for priorities on the graphical worker queues is disabled currently, I have to look into it if that makes sense (and re-enable it when it does). Since the classes are more or less the same now, it should be possible to replace the current Building::waresqueue(index) and Building::workersqueue(index) with a single Building::inputqueue(index, type) respectively a single ProductionSite::inputqueues() which returns both types of queues. This would clean up some code duplications where the same code is run on both queues. If no-one protests I will probably do so. Another, minor thing: I haven't looked into it, but when creating an expedition a builder is requested (at least, I assume he is). Now that we have worker queues: Should there be one for the builder in the tab of the port? -- https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue 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/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1603. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/175565469. Appveyor build 1441. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1441. -- https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue 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/animation_scaling into lp:widelands
This feels a bit messy. The scale of an animation must now be publicly knowable
on the outside for doing math on it, though it feels like it should be a purely
internal information. Can this not be internalized somehow by changing the
animations interface?
Also, if you want bigger images to look good at smaller zoom, we probably
should enable and create mipmaps while loading them.
Diff comments:
> === modified file
> 'data/tribes/buildings/productionsites/barbarians/shipyard/init.lua'
> --- data/tribes/buildings/productionsites/barbarians/shipyard/init.lua
> 2016-09-03 14:59:10 +
> +++ data/tribes/buildings/productionsites/barbarians/shipyard/init.lua
> 2016-11-13 16:57:34 +
> @@ -7,6 +7,7 @@
> descname = pgettext("barbarians_building", "Shipyard"),
> helptext_script = dirname .. "helptexts.lua",
> icon = dirname .. "menu.png",
> + representative_image = dirname .. "representative_image.png",
So we are back shipping an individual image for the representative image? I
understand that is for 'pics' in rich text, i.e. in messages and so on?
> size = "medium",
> needs_seafaring = true,
>
>
> === modified file 'src/graphic/animation.cc'
> --- src/graphic/animation.cc 2016-10-26 10:56:02 +
> +++ src/graphic/animation.cc 2016-11-13 16:57:34 +
> @@ -194,12 +208,12 @@
>
> uint16_t NonPackedAnimation::width() const {
> ensure_graphics_are_loaded();
> - return frames_[0]->width();
> + return frames_[0]->width() / scale_;
These should now return float?
> }
>
> uint16_t NonPackedAnimation::height() const {
> ensure_graphics_are_loaded();
> - return frames_[0]->height();
> + return frames_[0]->height() / scale_;
> }
>
> uint16_t NonPackedAnimation::nr_frames() const {
> @@ -215,20 +229,17 @@
> return hotspot_;
> }
>
> -Image* NonPackedAnimation::representative_image(const RGBColor* clr) const {
> +const Image* NonPackedAnimation::representative_image(const RGBColor* clr)
> const {
I am confused: why do you need the representative_image lua option?
> assert(!image_files_.empty());
> const Image* image = g_gr->images().get(image_files_[0]);
> -
> - if (!hasplrclrs_ || clr == nullptr) {
> - // No player color means we simply want an exact copy of the
> original image.
> - const int w = image->width();
> - const int h = image->height();
> - Texture* rv = new Texture(w, h);
> - rv->blit(Rectf(0, 0, w, h), *image, Rectf(0, 0, w, h), 1.,
> BlendMode::Copy);
> - return rv;
> - } else {
> - return playercolor_image(clr, image,
> g_gr->images().get(pc_mask_image_files_[0]));
> + if (hasplrclrs_ && clr) {
> + image = playercolor_image(clr, image,
> g_gr->images().get(pc_mask_image_files_[0]));
> }
> + const int w = image->width();
> + const int h = image->height();
> + Texture* rv = new Texture(w / scale_, h / scale_);
> + rv->blit(Rectf(0, 0, w / scale_, h / scale_), *image, Rectf(0, 0, w,
> h), 1., BlendMode::Copy);
> + return rv;
> }
>
> const std::string& NonPackedAnimation::representative_image_filename() const
> {
--
https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/animation_scaling 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/oars_appdata into lp:widelands
Review: Resubmit Thanks for the review - could you please check again for me? I have added the validator to utils/uppdate_appdata.py now, so I can't forget to validate again. It's now throwing a warning "Found invalid tag: 'content_rating'", but I guess that's because the format is new and the validator hasn't caught up yet. They also added some localization format checks, so I have fixed those as well. The validator now also complains about the desktop "Type" entry, but according to the latest spec, it's required. So, I'm leaving it in: https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s05.html -- https://code.launchpad.net/~widelands-dev/widelands/oars_appdata/+merge/309624 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/oars_appdata. ___ 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/oars_appdata into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/oars_appdata into lp:widelands has been updated. Commit Message changed to: Added age ratings to Appdata https://odrs.gnome.org/oars and validator call to utils/uppdate_appdata.py. Fixed localization format for long description. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/oars_appdata/+merge/309624 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/oars_appdata. ___ 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-863185-census-on-destroyed-building into lp:widelands
Review: Needs Fixing
Diff comments:
> === modified file 'src/logic/map_objects/immovable.cc'
> --- src/logic/map_objects/immovable.cc2016-11-03 07:20:57 +
> +++ src/logic/map_objects/immovable.cc2016-11-03 07:36:43 +
> @@ -339,6 +339,7 @@
>
> Immovable::Immovable(const ImmovableDescr& imm_descr)
> : BaseImmovable(imm_descr),
> + antecedent_(nullptr),
I am not familiar with this word. Can you find a simpler name?
> anim_(0),
> animstart_(0),
> program_(nullptr),
>
> === modified file 'src/logic/map_objects/immovable.h'
> --- src/logic/map_objects/immovable.h 2016-11-03 07:20:57 +
> +++ src/logic/map_objects/immovable.h 2016-11-03 07:36:43 +
> @@ -201,7 +201,11 @@
> Immovable(const ImmovableDescr&);
> ~Immovable();
>
> - void set_owner(Player*);
> + /// If this immovable was created by a building, this can be set in
> order to display information
> + /// about it. If this is a player immovable, you will need to set the
> owner first.
> + void set_antecedent(const BuildingDescr& building);
> +
> + void set_owner(Player* player);
I think this function is overwritten in base classes. You'll have a bad time if
you do not mark it as virtual here. Ideally you want to remove the overwritten
functions too.
>
> Coords get_position() const {
> return position_;
>
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc 2016-11-03 07:20:57 +
> +++ src/logic/map_objects/tribes/building.cc 2016-11-03 07:36:43 +
> @@ -441,8 +442,12 @@
> const Coords pos = position_;
> PlayerImmovable::destroy(egbase);
> // We are deleted. Only use stack variables beyond this point
> - if (fire)
> - egbase.create_immovable(pos, "destroyed_building",
> MapObjectDescr::OwnerType::kTribe);
> + if (fire) {
> + Immovable& destroyed_building =
> +egbase.create_immovable(pos, "destroyed_building",
> MapObjectDescr::OwnerType::kTribe);
You'll probably end up with cleaner code if you get rid of 'set_owner' and
'set_antcedent' and instead add a 'create_immovable_with_precedessor' in
EditorGame and set up all the state in the constructor of Immovable and/or in
the EgBase function.
> + destroyed_building.set_owner(get_owner());
> + destroyed_building.set_antecedent(descr());
> + }
> }
>
> std::string Building::info_string(const InfoStringFormat& format) {
--
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.
___
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
