widelands

Merge lp:~widelands-dev/widelands/bug-1554552 into lp:widelands

  1. bug-1554552
  2. Merge into trunk
Proposed by TiborB
Status: Merged
Merged at revision: 7924
Proposed branch: lp:~widelands-dev/widelands/bug-1554552
Merge into: lp:widelands
Diff against target: 230 lines (+54/-22)
3 files modified
src/logic/map_objects/bob.h (+1/-1)
src/logic/map_objects/tribes/soldier.cc (+50/-21)
src/logic/map_objects/tribes/soldier.h (+3/-0)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1554552
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+289830@code.launchpad.net

Commit message

Soldier object got new variable - retreat_health_

Description of the change

I got rid of ui32var3 task's variable, that misbehaved and replaced it with retreat_health_ variable (soldier's atribute)

When you will test it, you can keep soldier's DBG window opened and watch what is going on (also with retreat_health_)

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 885. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/117807015.
Appveyor build 718. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1554552-718.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I have streamlined the code a bit, that shouldn't change the semantics.

Code looks clean, and soldiers seem to be retreating properly.

review: Abstain
Revision history for this message
TiborB (tiborb95) wrote :

This change is mostly relevant for peoples who tend to micromanage everything and somebody like that should review and comment this. Also we can further modify the log's content, but somebody should say what is he/she missing there....

Revision history for this message
GunChleoc (gunchleoc) wrote :

OK, let's wait for somebody else to come in and do some more testing - you have my approval for the code (my "Abstain" was a misclick).

review: Approve
Revision history for this message
TiborB (tiborb95) wrote :

Oh wait a bit, I forgot to add new value into game save/load code. (And break save compatibility again :) )

Revision history for this message
GunChleoc (gunchleoc) wrote :

I had double-checked and didn't see any saveloading of the old value - I might have missed something thoug, or it might be a bug that it's not there.

Revision history for this message
TiborB (tiborb95) wrote :

@Gun, I mean we have to save and load also retreat_health_ value - that is new variable of soldier object... otherwise it will be 0 after game load...

Revision history for this message
GunChleoc (gunchleoc) wrote :

I know - can you double-check if ivar3 is saved as well while you're dealing with this?

Revision history for this message
TiborB (tiborb95) wrote :

OK, I added saving/loading of retreat_health_ and increased the packed number as well...

ivar3 is not new value and it is taken care of in bob.cc

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 910. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/119339492.
Appveyor build 742. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1554552-742.

Revision history for this message
GunChleoc (gunchleoc) wrote :

LGTM - this messes up the testsuite though. So, we need to test map loading, especially for scenarios.

review: Needs Fixing
Revision history for this message
TiborB (tiborb95) wrote :

I see:

Packet Name: Soldier
Saved Version: 2
Current Version: 3

Where did the version 2 came from? I will look at it of course...

Revision history for this message
GunChleoc (gunchleoc) wrote :

The test scenario probably has a bob packet - we need the bob packets for the critters, so we can't just exclude them from the map loading code in general. We might be safe killing the packet in the test suite though - it should not affect normal map loading, because those should only have critters (in theory! - we need to double-check).

Revision history for this message
TiborB (tiborb95) wrote :

I failed to find out what exactly this test does :(

Can a map contains soldiers?

Revision history for this message
GunChleoc (gunchleoc) wrote :

I found it in MapObjectPacket - case MapObject::HeaderWorker.

We didn't have any compatibility code for soldiers in Build18, so this is new. We obviously don't want compatibility code where we don't need it, but we also want to be able to load old maps, which I now fear might have soldiers in their MapObjectPackets, or at least a header for the loader.

So, we should test a few old maps to check if they throw the same error. If they do, we should add compatibility code.

Revision history for this message
TiborB (tiborb95) wrote :

Do you mean inserting in load some if..then.. depending on packet version 2 or 3?

if v.2 retreat_health_ = 0
if v.3 retreat_health_ is to be loaded from save

No big deal for me...

Revision history for this message
GunChleoc (gunchleoc) wrote :

Exactly something like this.

Revision history for this message
TiborB (tiborb95) wrote :

@Gun, compatibility added, the test runs now

Revision history for this message
GunChleoc (gunchleoc) wrote :

Excellent!

@bunnybot merge

Revision history for this message
GunChleoc (gunchleoc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map_objects/bob.h'
2--- src/logic/map_objects/bob.h 2016-02-07 09:30:20 +0000
3+++ src/logic/map_objects/bob.h 2016-03-31 20:29:03 +0000
4@@ -213,7 +213,7 @@
5 const Task * task;
6 int32_t ivar1;
7 int32_t ivar2;
8- union {int32_t ivar3; uint32_t ui32var3;};
9+ int32_t ivar3;
10 ObjectPointer objvar1;
11 std::string svar1;
12
13
14=== modified file 'src/logic/map_objects/tribes/soldier.cc'
15--- src/logic/map_objects/tribes/soldier.cc 2016-03-08 08:22:28 +0000
16+++ src/logic/map_objects/tribes/soldier.cc 2016-03-31 20:29:03 +0000
17@@ -224,7 +224,8 @@
18 defense_level_ = 0;
19 evade_level_ = 0;
20
21- current_health_ = get_max_health();
22+ current_health_ = get_max_health();
23+ retreat_health_ = 0;
24
25 combat_walking_ = CD_NONE;
26 combat_walkstart_ = 0;
27@@ -238,6 +239,7 @@
28 attack_level_ = 0;
29 defense_level_ = 0;
30 evade_level_ = 0;
31+ retreat_health_ = 0;
32
33 current_health_ = get_max_health();
34
35@@ -301,6 +303,11 @@
36
37 evade_level_ = evade;
38 }
39+void Soldier::set_retreat_health(const uint32_t retreat) {
40+ assert(retreat <= get_max_health());
41+
42+ retreat_health_ = retreat;
43+}
44
45 uint32_t Soldier::get_level(TrainingAttribute const at) const {
46 switch (at) {
47@@ -696,12 +703,13 @@
48 state.ivar3 = 0; // Counts how often the soldier is blocked in a row
49
50 state.ivar1 |= CF_RETREAT_WHEN_INJURED;
51- state.ui32var3 = kRetreatWhenHealthDropsBelowThisPercentage * get_max_health() / 100;
52+ set_retreat_health(kRetreatWhenHealthDropsBelowThisPercentage * get_max_health() / 100);
53
54 // Injured soldiers are not allowed to attack
55- if (state.ui32var3 > get_current_health()) {
56- state.ui32var3 = get_current_health();
57+ if (get_retreat_health() > get_current_health()) {
58+ set_retreat_health(get_current_health());
59 }
60+ molog("[attack] starting, retreat health: %d\n", get_retreat_health());
61 }
62
63 void Soldier::attack_update(Game & game, State & state)
64@@ -883,14 +891,17 @@
65
66 if
67 (!enemy ||
68- ((state.ivar1 & CF_RETREAT_WHEN_INJURED) &&
69- state.ui32var3 > get_current_health() &&
70+ (get_retreat_health() > get_current_health() &&
71 defenders > 0))
72 {
73 // Injured soldiers will try to return to safe site at home.
74- if (state.ui32var3 > get_current_health() && defenders) {
75- state.coords = Coords::null();
76- state.objvar1 = nullptr;
77+ if (get_retreat_health() > get_current_health()) {
78+ assert(state.ivar1 & CF_RETREAT_WHEN_INJURED);
79+ if (defenders) {
80+ molog(" [attack] badly injured (%d), retreating...\n", get_current_health());
81+ state.coords = Coords::null();
82+ state.objvar1 = nullptr;
83+ }
84 }
85 // The old militarysite gets replaced by a new one, so if "enemy" is not
86 // valid anymore, we either "conquered" the new building, or it was
87@@ -1016,15 +1027,19 @@
88 // Here goes 'configuration'
89 if (stayhome) {
90 state.ivar1 |= CF_DEFEND_STAYHOME;
91+ set_retreat_health(0);
92 } else {
93 /* Flag defenders are not allowed to flee, to avoid abuses */
94 state.ivar1 |= CF_RETREAT_WHEN_INJURED;
95- state.ui32var3 = get_max_health() * kRetreatWhenHealthDropsBelowThisPercentage / 100;
96+ set_retreat_health(get_max_health() * kRetreatWhenHealthDropsBelowThisPercentage / 100);
97
98 // Soldier must defend even if he starts injured
99- if (state.ui32var3 < get_current_health())
100- state.ui32var3 = get_current_health();
101+ // (current health is below retreat treshold)
102+ if (get_retreat_health() > get_current_health()) {
103+ set_retreat_health(get_current_health());
104+ }
105 }
106+ molog("[defense] retreat health set: %d\n", get_retreat_health());
107 }
108
109 struct SoldierDistance {
110@@ -1128,13 +1143,15 @@
111
112 if
113 (soldiers.empty() ||
114- ((state.ivar1 & CF_RETREAT_WHEN_INJURED) &&
115- get_current_health() < state.ui32var3))
116+ (get_current_health() < get_retreat_health()))
117 {
118+ if (get_retreat_health() > get_current_health()) {
119+ assert(state.ivar1 & CF_RETREAT_WHEN_INJURED);
120+ }
121
122- if (get_current_health() < state.ui32var3)
123- molog("[defense] I am heavily injured!\n");
124- else
125+ if (get_current_health() < get_retreat_health()) {
126+ molog("[defense] I am heavily injured (%d)!\n", get_current_health());
127+ } else
128 molog("[defense] no enemy soldiers found, ending task\n");
129
130 // If no enemy was found, return home
131@@ -1158,7 +1175,6 @@
132 descr().get_right_walk_anims(does_carry_ware()),
133 true);
134 }
135-
136 molog("[defense] return home\n");
137 if
138 (start_task_movepath
139@@ -1602,7 +1618,7 @@
140 if
141 (!attackdefense ||
142 ((attackdefense->ivar1 & CF_RETREAT_WHEN_INJURED) &&
143- attackdefense->ui32var3 > get_current_health()))
144+ get_retreat_health() > get_current_health()))
145 {
146 // Retreating or non-combatant soldiers act like normal bobs
147 return Bob::check_node_blocked(game, field, commit);
148@@ -1718,6 +1734,7 @@
149 ("Levels: %d/%d/%d/%d\n",
150 health_level_, attack_level_, defense_level_, evade_level_);
151 molog ("Health: %d/%d\n", current_health_, get_max_health());
152+ molog ("Retreat: %d\n", retreat_health_);
153 molog ("Attack: %d-%d\n", get_min_attack(), get_max_attack());
154 molog ("Defense: %d%%\n", get_defense());
155 molog ("Evade: %d%%\n", get_evade());
156@@ -1739,7 +1756,9 @@
157 ==============================
158 */
159
160-constexpr uint8_t kCurrentPacketVersion = 2;
161+constexpr uint8_t kCurrentPacketVersion = 3;
162+// TODO(TiborB): This is only for map compatibility in regression tests, we should get rid of this ASAP
163+constexpr uint8_t kOldPacketVersion = 2;
164
165 Soldier::Loader::Loader() :
166 battle_(0)
167@@ -1752,10 +1771,16 @@
168
169 try {
170 uint8_t packet_version = fr.unsigned_8();
171- if (packet_version == kCurrentPacketVersion) {
172+ if (packet_version == kCurrentPacketVersion || packet_version == kOldPacketVersion) {
173
174 Soldier & soldier = get<Soldier>();
175 soldier.current_health_ = fr.unsigned_32();
176+ if (packet_version == kCurrentPacketVersion) {
177+ soldier.retreat_health_ = fr.unsigned_32();
178+ } else {
179+ // not ideal but will be used only for regression tests
180+ soldier.retreat_health_ = 0;
181+ }
182
183 soldier.health_level_ =
184 std::min(fr.unsigned_32(), soldier.descr().get_max_health_level());
185@@ -1769,6 +1794,9 @@
186 if (soldier.current_health_ > soldier.get_max_health())
187 soldier.current_health_ = soldier.get_max_health();
188
189+ if (soldier.retreat_health_ > soldier.get_max_health())
190+ soldier.retreat_health_ = soldier.get_max_health();
191+
192 soldier.combat_walking_ = static_cast<CombatWalkingDir>(fr.unsigned_8());
193 if (soldier.combat_walking_ != CD_NONE) {
194 soldier.combat_walkstart_ = fr.unsigned_32();
195@@ -1816,6 +1844,7 @@
196
197 fw.unsigned_8(kCurrentPacketVersion);
198 fw.unsigned_32(current_health_);
199+ fw.unsigned_32(retreat_health_);
200 fw.unsigned_32(health_level_);
201 fw.unsigned_32(attack_level_);
202 fw.unsigned_32(defense_level_);
203
204=== modified file 'src/logic/map_objects/tribes/soldier.h'
205--- src/logic/map_objects/tribes/soldier.h 2016-02-21 19:43:07 +0000
206+++ src/logic/map_objects/tribes/soldier.h 2016-03-31 20:29:03 +0000
207@@ -159,6 +159,7 @@
208 void set_attack_level (uint32_t);
209 void set_defense_level(uint32_t);
210 void set_evade_level (uint32_t);
211+ void set_retreat_health (uint32_t);
212 uint32_t get_level (TrainingAttribute) const;
213 uint32_t get_health_level () const {return health_level_;}
214 uint32_t get_attack_level () const {return attack_level_;}
215@@ -178,6 +179,7 @@
216 void draw_info_icon(RenderTarget &, Point, bool anchor_below) const;
217
218 uint32_t get_current_health() const {return current_health_;}
219+ uint32_t get_retreat_health() const {return retreat_health_;}
220 uint32_t get_max_health() const;
221 uint32_t get_min_attack() const;
222 uint32_t get_max_attack() const;
223@@ -256,6 +258,7 @@
224 uint32_t attack_level_;
225 uint32_t defense_level_;
226 uint32_t evade_level_;
227+ uint32_t retreat_health_;
228
229 /// This is used to replicate walk for soldiers but only just before and
230 /// just after figthing in a battle, to draw soldier at proper position.

Subscribers

People subscribed via source and target branches

to status/vote changes: