Re: [Freeciv-Dev] (PR#39792) explore server assert

2008-01-18 Thread William Allen Simpson

URL: http://bugs.freeciv.org/Ticket/Display.html?id=39792 

Unfortunately, it appears that the activity and explore code has so many
problems that it's not practical to completely fix at this time.

One of the most egregious problems is that folks didn't always use access
functions for setting activities, and/or didn't uniformly use the same
functions.  In this case, one code path set ACTIVITY_IDLE directly with
set_unit_activity(), while another used an intermediate function that in
turn calls unit_activity_dependencies().  They achieve different results

There are far too many such paths used for all activities.  This will need
considerable cleanup to find them all, such as PR#39870.

With 24 set_unit_activity() call variants, 44 unit_activity_handling()
call variants, and 31 ai.done references, not to mention 198 ai.control
references, taming this beast will take some doing!

===

I've checked in this pair of patches (for 2.1 and 2.2) to at least allow
the current savegame(s) to go forward without crashing.

First and foremost, combine the similar code paths into do_explore(), which
will indirectly call unit_activity_dependencies().

Secondly, but perhaps more importantly, use enum unit_move_result returns
for ai_manage_explorer(), and pass along the new MR_DEATH result.  This
eliminates several crashing bugs where ai.done was set for dead units.

More FIXME added for future work!

Committed S2_1 revision 14246.
Committed S2_2 revision 14247.
Committed trunk revision 14248.

Index: server/unittools.c
===
--- server/unittools.c  (revision 14245)
+++ server/unittools.c  (working copy)
@@ -652,24 +652,7 @@
   unit_restore_movepoints(pplayer, punit);
 
   if (activity == ACTIVITY_EXPLORE) {
-bool more_to_explore = ai_manage_explorer(punit);
-
-if (!player_find_unit_by_id(pplayer, id)) {
-  /* Died */
-  return;
-}
-
-/* ai_manage_explorer isn't supposed to change the activity but we
- * don't count on this. */
-if (punit-activity != ACTIVITY_EXPLORE || !more_to_explore) {
-  handle_unit_activity_request(punit, ACTIVITY_IDLE);
-
-  /* FIXME: When the ai_manage_explorer call changes the activity from
-   * EXPLORE to IDLE, then for some reason the ai.control value gets left
-   * set.  We reset it here.  See PR#12931. */
-  punit-ai.control = FALSE;
-}
-send_unit_info(NULL, punit);
+do_explore(punit);
 return;
   }
 
@@ -2041,6 +2024,35 @@
 }
 
 /**
+  ...
+**/
+void do_explore(struct unit *punit)
+{
+  switch (ai_manage_explorer(punit)) {
+  case MR_DEATH:
+/* don't use punit! */
+return;
+  case MR_OK:
+/* FIXME: ai_manage_explorer() isn't supposed to change the activity,
+ * but don't count on this.  See PR#39792.
+ */
+if (punit-activity == ACTIVITY_EXPLORE) {
+  break;
+}
+/* fallthru */
+  default:
+handle_unit_activity_request(punit, ACTIVITY_IDLE);
+
+/* FIXME: When the ai_manage_explorer() call changes the activity from
+ * EXPLORE to IDLE, in unit_activity_handling() ai.control is left
+ * alone.  We reset it here.  See PR#12931. */
+punit-ai.control = FALSE;
+break;
+  };
+  send_unit_info(NULL, punit); /* probably duplicate */
+}
+
+/**
   Returns whether the drop was made or not. Note that it also returns 1 
   in the case where the drop was succesful, but the unit was killed by
   barbarians in a hut.
Index: server/unittools.h
===
--- server/unittools.h  (revision 14245)
+++ server/unittools.h  (working copy)
@@ -81,6 +81,7 @@
 /* doing a unit activity */
 void do_nuclear_explosion(struct player *pplayer, struct tile *ptile);
 bool do_airline(struct unit *punit, struct city *city2);
+void do_explore(struct unit *punit);
 bool do_paradrop(struct unit *punit, struct tile *ptile);
 void load_unit_onto_transporter(struct unit *punit, struct unit *ptrans);
 void unload_unit_from_transporter(struct unit *punit);
Index: server/unithand.c
===
--- server/unithand.c   (revision 14245)
+++ server/unithand.c   (working copy)
@@ -591,34 +591,35 @@
 return;
   }
 
-  if (punit-activity != activity ||
-  punit-activity_target != activity_target ||
-  punit-ai.control) {
+  if (punit-activity == activity
+punit-activity_target == activity_target
+!punit-ai.control) {
 /* Treat change in ai.control as change in activity, so
-   * idle autosettlers behave correctly when selected --dwp
+ * idle autosettlers behave correctly when selected --dwp
  */
-punit-ai.control = FALSE;
-punit-goto_tile = NULL;
+return;
+  }
+
+  punit-ai.control 

Re: [Freeciv-Dev] (PR#39792) explore server assert

2008-01-15 Thread William Allen Simpson

URL: http://bugs.freeciv.org/Ticket/Display.html?id=39792 

Jason Short wrote:
 I'd really like a savegame from which this can be reproduced.
 
What's wrong with the two existing savegames they've already provided?

Perhaps you didn't actually read the report(s)  Next time, please
re-read the whole thread before replying to the last message.

Yes, it is triggered for ground units.  Apparently, you're the one who
added the bad assert() because you didn't properly study the code.

[I just didn't say so publicly until now.  I was trying to be polite.]

I'm using the two savegames to test my work, and I'll post the patch
after they've been tested on both 2.1 and 2.2!

Thank you (the bug reporters) for your patience.



___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev