widelands

Imperial sentry returns more wares when dismantled than it needed

Bug #1170086 reported by wl-zocker
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
High
cghislai
widelands build18-rc1

Bug Description

Build costs: 1 stone, 1 trunk, 1 wood (short: 1s, 1t, 1w)
Returned when dismantled: 1s, 1t, 2w

I think this is because the sentry is an enhanced building (from the barrack), so the build costs of the two buildings are added:
Barrack: 1t, 2w + Sentry: 1s, 1t, 1w = 1s, 2t, 3w
These values are divided by two and rounded up to calculate the returned resources. This gives 1s, 1t, 2w, as observed.

The same issue appears for the Imperial barrier (costs: 2s, 2t, 2w, 1 marble) because of the outpost (costs: 1s, 1t, 1w, 1m). Added, it is 3s, 3t, 3w, 2m, which leads to 2s, 2t, 2w, 1m when dismantling. These are the actual build costs (although it should be 1s, 1t, 1w, 1m).

These are the only two buildings where the problem appears because the are buildable directly (using less resources) and via upgrade (using additional resources for the lower building), but return the same wares in both cases.

Tags: empire

Related branches

lp://staging/~widelands-dev/widelands/bug1170086
SirVer: Approve
Diff: 3061 lines (+867/-155)
144 files modified
src/logic/building.cc (+20/-11)
src/logic/building.h (+45/-6)
src/logic/constructionsite.cc (+18/-23)
src/logic/constructionsite.h (+0/-3)
src/logic/dismantlesite.cc (+19/-26)
src/logic/dismantlesite.h (+2/-3)
src/logic/editor_game_base.cc (+10/-6)
src/logic/editor_game_base.h (+7/-4)
src/logic/player.cc (+3/-4)
src/map_io/widelands_map_building_data_packet.cc (+10/-6)
src/map_io/widelands_map_buildingdata_data_packet.cc (+93/-32)
src/map_io/widelands_map_buildingdata_data_packet.h (+3/-0)
src/wui/buildingwindow.cc (+3/-2)
tribes/atlanteans/armoursmithy/conf (+5/-0)
tribes/atlanteans/bakery/conf (+4/-0)
tribes/atlanteans/blackroot_farm/conf (+5/-0)
tribes/atlanteans/burners_house/conf (+4/-0)
tribes/atlanteans/castle/conf (+5/-0)
tribes/atlanteans/coalmine/conf (+4/-0)
tribes/atlanteans/crystalmine/conf (+4/-0)
tribes/atlanteans/dungeon/conf (+8/-0)
tribes/atlanteans/farm/conf (+5/-0)
tribes/atlanteans/fish_breeders_house/conf (+3/-0)
tribes/atlanteans/fishers_house/conf (+3/-0)
tribes/atlanteans/foresters_house/conf (+4/-0)
tribes/atlanteans/goldmine/conf (+4/-0)
tribes/atlanteans/goldweaver/conf (+4/-0)
tribes/atlanteans/guardhall/conf (+5/-0)
tribes/atlanteans/guardhouse/conf (+3/-0)
tribes/atlanteans/high_tower/conf (+4/-1)
tribes/atlanteans/horsefarm/conf (+4/-0)
tribes/atlanteans/hunters_house/conf (+3/-0)
tribes/atlanteans/ironmine/conf (+4/-0)
tribes/atlanteans/labyrinth/conf (+8/-0)
tribes/atlanteans/mill/conf (+5/-0)
tribes/atlanteans/port/conf (+7/-0)
tribes/atlanteans/quarry/conf (+3/-0)
tribes/atlanteans/sawmill/conf (+4/-0)
tribes/atlanteans/scouts_house/conf (+3/-0)
tribes/atlanteans/shipyard/conf (+6/-0)
tribes/atlanteans/small_tower/conf (+4/-0)
tribes/atlanteans/smelting_works/conf (+3/-0)
tribes/atlanteans/smokery/conf (+3/-0)
tribes/atlanteans/spiderfarm/conf (+4/-0)
tribes/atlanteans/toolsmithy/conf (+4/-0)
tribes/atlanteans/tower/conf (+5/-0)
tribes/atlanteans/warehouse/conf (+6/-0)
tribes/atlanteans/weaponsmithy/conf (+5/-0)
tribes/atlanteans/weaving-mill/conf (+5/-0)
tribes/atlanteans/well/conf (+4/-0)
tribes/atlanteans/woodcutters_house/conf (+4/-0)
tribes/barbarians/axefactory/conf (+6/-1)
tribes/barbarians/bakery/conf (+5/-0)
tribes/barbarians/barrier/conf (+4/-0)
tribes/barbarians/battlearena/conf (+7/-0)
tribes/barbarians/big_inn/conf (+4/-1)
tribes/barbarians/brewery/conf (+5/-1)
tribes/barbarians/burners_house/conf (+4/-0)
tribes/barbarians/cattlefarm/conf (+4/-0)
tribes/barbarians/citadel/conf (+7/-1)
tribes/barbarians/coalmine/conf (+4/-0)
tribes/barbarians/deep_coalmine/conf (+5/-1)
tribes/barbarians/deep_goldmine/conf (+5/-1)
tribes/barbarians/deep_oremine/conf (+5/-1)
tribes/barbarians/deeper_coalmine/conf (+5/-1)
tribes/barbarians/deeper_goldmine/conf (+5/-1)
tribes/barbarians/deeper_oremine/conf (+5/-1)
tribes/barbarians/donjon/conf (+4/-0)
tribes/barbarians/farm/conf (+5/-0)
tribes/barbarians/fernery/conf (+4/-0)
tribes/barbarians/fishers_hut/conf (+3/-0)
tribes/barbarians/fortress/conf (+6/-0)
tribes/barbarians/gamekeepers_hut/conf (+4/-0)
tribes/barbarians/goldmine/conf (+4/-0)
tribes/barbarians/granitemine/conf (+4/-0)
tribes/barbarians/hardener/conf (+4/-0)
tribes/barbarians/helmsmithy/conf (+6/-0)
tribes/barbarians/hunters_hut/conf (+4/-0)
tribes/barbarians/inn/conf (+5/-1)
tribes/barbarians/lime_kiln/conf (+4/-0)
tribes/barbarians/lumberjacks_hut/conf (+3/-0)
tribes/barbarians/metalworks/conf (+5/-0)
tribes/barbarians/micro-brewery/conf (+5/-0)
tribes/barbarians/oremine/conf (+4/-0)
tribes/barbarians/port/conf (+9/-0)
tribes/barbarians/quarry/conf (+5/-2)
tribes/barbarians/rangers_hut/conf (+3/-0)
tribes/barbarians/scouts_hut/conf (+4/-0)
tribes/barbarians/sentry/conf (+3/-0)
tribes/barbarians/shipyard/conf (+5/-0)
tribes/barbarians/smelting_works/conf (+5/-0)
tribes/barbarians/tavern/conf (+5/-0)
tribes/barbarians/trainingscamp/conf (+6/-0)
tribes/barbarians/warehouse/conf (+6/-0)
tribes/barbarians/warmill/conf (+6/-1)
tribes/barbarians/weaving-mill/conf (+4/-0)
tribes/barbarians/well/conf (+3/-0)
tribes/empire/arena/conf (+15/-7)
tribes/empire/armoursmithy/conf (+5/-0)
tribes/empire/bakery/conf (+4/-0)
tribes/empire/barracks/conf (+3/-0)
tribes/empire/barrier/conf (+15/-0)
tribes/empire/brewery/conf (+4/-0)
tribes/empire/burners_house/conf (+5/-0)
tribes/empire/castle/conf (+7/-1)
tribes/empire/coalmine/conf (+4/-0)
tribes/empire/colosseum/conf (+8/-1)
tribes/empire/deep_coalmine/conf (+5/-1)
tribes/empire/deep_goldmine/conf (+5/-1)
tribes/empire/deep_marblemine/conf (+5/-1)
tribes/empire/deep_oremine/conf (+5/-1)
tribes/empire/donkeyfarm/conf (+4/-0)
tribes/empire/farm/conf (+6/-0)
tribes/empire/fishers_house/conf (+3/-0)
tribes/empire/foresters_house/conf (+4/-0)
tribes/empire/fortress/conf (+7/-0)
tribes/empire/goldmine/conf (+4/-0)
tribes/empire/hunters_house/conf (+3/-0)
tribes/empire/inn/conf (+5/-1)
tribes/empire/lumberjacks_house/conf (+4/-0)
tribes/empire/marblemine/conf (+4/-0)
tribes/empire/mill/conf (+5/-0)
tribes/empire/oremine/conf (+4/-0)
tribes/empire/outpost/conf (+4/-0)
tribes/empire/piggery/conf (+5/-0)
tribes/empire/port/conf (+8/-0)
tribes/empire/quarry/conf (+3/-0)
tribes/empire/sawmill/conf (+4/-0)
tribes/empire/scouts_house/conf (+3/-0)
tribes/empire/sentry/conf (+11/-0)
tribes/empire/sheepfarm/conf (+4/-0)
tribes/empire/shipyard/conf (+5/-0)
tribes/empire/smelting_works/conf (+4/-0)
tribes/empire/stonemasons_house/conf (+4/-0)
tribes/empire/tavern/conf (+5/-0)
tribes/empire/toolsmithy/conf (+4/-0)
tribes/empire/tower/conf (+5/-0)
tribes/empire/trainingscamp/conf (+7/-0)
tribes/empire/vineyard/conf (+4/-0)
tribes/empire/warehouse/conf (+6/-0)
tribes/empire/weaponsmithy/conf (+6/-0)
tribes/empire/weaving-mill/conf (+4/-0)
tribes/empire/well/conf (+4/-0)
tribes/empire/winery/conf (+5/-0)
Revision history for this message
Nicolai Hähnle (nha) wrote :

Should all upgraded buildings only return resources relative to the last upgrade step? Or should there be a special case for upgraded buildings that can be built directly? Discuss.

Changed in widelands:
importance: Undecided → High
status: New → Confirmed
Revision history for this message
wl-zocker (wl-zocker) wrote :

I think the other upgraded buildings (fortress, Barbarian warmill, mines) should return all the wares they needed summed up (and divided by 2) because there is no other way to obtain them.
Only for buildings that can be built directly there should be a special case (if there is no line "buildable=no" in the config file).
If the player upgrades e.g. his barrack, he will not get all the resources back. I think that nothing to worry about: The player chose himself to do it that way.

Revision history for this message
SirVer (sirver) wrote :

Wow, good catch. And a nice cheat, actually 8).

I agree with wl-zocker. Seems reasonable to only give back wares for the last stage if it is buildable=yes, otherwise for all stages. Setting to triaged if noone complains.

Changed in widelands:
status: Confirmed → Triaged
Revision history for this message
Nasenbaer (nasenbaer) wrote :

I would go even further and see the bug in a different place - when upgrading a barracks to a sentry, the sentry should cost less than when it is built directly (however the whole building process - from nothing over barracks to sentry should maybe cost a bit more (e.g. 1 more trunk))

Hans Joachim Desserud (hjd)
tags: added: empire
Hans Joachim Desserud (hjd)
summary: - Imperial sentry returns more wares when dismatled than it needed
+ Imperial sentry returns more wares when dismantled than it needed
Revision history for this message
cghislai (charlyghislain) wrote :

So lets say building X costs X, building Y costs Y. X may be enhanced to Y.
I like Nasenbaer suggestion.

enhancing X to Y costs Y/2
destroying X returns X/2
destroying Y returns Y/2
destroying enhanced Y returns (X+Y/2)/2

Changed in widelands:
assignee: nobody → cghislai (charlyghislain)
Revision history for this message
SirVer (sirver) wrote :

I think making the return values explicit in the conf files (like the build cost) gives us more control. Don't you think?

Revision history for this message
cghislai (charlyghislain) wrote :

Yep, sounds good too me as well.
What about allowing a second value to be given in the buildcost section, so 'trunk=2' would become 'trunk=2 1' if only 1 is needed for enhancing. Or maybe is it better to add a new section?

Revision history for this message
cghislai (charlyghislain) wrote :

Reading #6 again I see you were talking about return values. If they are put in the conf file, which I'm not against, this does not solve the issue with returned ware of enhanced buildings vs directly built ones. So we must allow two values in that section as well.

I think of keeping a pointer (or vector of pointer) to the Descr of the former building if enhanced - this is already done in constructionsite. This way we can easily track down the return values for all enhancement steps.

Revision history for this message
SirVer (sirver) wrote :

Mmmh - I like the idea of having everythig explicit in the conf files (new sections, much easier to read). But I cannot come up with a format that is easy to read and understand and flexible enough to fullfill all our needs. Could you conjecture a conf example that deals with the most complex scenarios? remember that theoretically buildings could be enhanced multiple times and/or build directly.

Revision history for this message
cghislai (charlyghislain) wrote :

Then i think [buildcost] [enhancecost] [returngain] [enhancementreturngain] sections are explicit and enough to deal with all cases. enhancementreturngain could have a better name though

Revision history for this message
cghislai (charlyghislain) wrote :

Concerning the flexibility, a single building can only be enhanced to an other single one, and only be the ehancement of a single third one. I don't know if such limitation is intended or if we should better considerate multiple enhancements.

IMO multiple enhancements involve too much work to be considered now.

Revision history for this message
SirVer (sirver) wrote :

You are right I think. I think good names for the sections are return_on_dismantle (you could drop the _ for symmetry with the other names, but I think they add readability) and return_on_dismantle_on_enhanced. I see no pain in making these long and explicit - we will not write them often and tab completion is your friend.

Revision history for this message
cghislai (charlyghislain) wrote :

If buildable, [buildcost] and [return_on_dismantle] are mandatory
If enhaced building, [enhancement_cost] and [return_on_dismantle_on_enhanced] are mandatory

Revision history for this message
SirVer (sirver) wrote :

Makes sense. I am not too happy with the las section name but could not come up with anything better. Feel free to use a better name :).

cghislai (charlyghislain)
Changed in widelands:
milestone: none → build18-rc1
cghislai (charlyghislain)
Changed in widelands:
status: Triaged → In Progress
SirVer (sirver)
Changed in widelands:
status: In Progress → Fix Committed
Revision history for this message
SirVer (sirver) wrote :

Released in build-18 rc1.

Changed in widelands:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.