Re: [Freeciv-Dev] (PR#35641) Unit transport loading cleanup

2007-02-10 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=35641 >

Per I. Mathisen wrote:
> http://bugs.freeciv.org/Ticket/Display.html?id=35641 >
> 
> This patch
>  * Removes the second parameter to find_transporter_for_unit(), which
> is already known.
>  * Adds function could_unit_load() which checks if a given transporter
> could add a unit as cargo, were it on the same tile.
>  * Since find_transporter_for_unit() calls can_unit_load(), it is
> meaningless to call the latter on results from the former. Remove such
> usages.
>  * Make use of all checks in could_unit_load() in
> find_transport_from_tile(), instead of the smaller and duplicating
> list of checks there, that seemed to make no sense.
> 
> Can people who have touched this code, please take a look?

Seems okay.  The likely reason for the extra parameter for
find_transporter_for_unit is unit movement - but the unittools change
you make looks fine since the unit's tile has already been changed by
this point.

-jason



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


Re: [Freeciv-Dev] (PR#35641) Unit transport loading cleanup

2007-02-10 Thread Per Inge Mathisen

http://bugs.freeciv.org/Ticket/Display.html?id=35641 >

On 2/10/07, Marko Lindqvist <[EMAIL PROTECTED]> wrote:
>  I think you cannot check for is_native_tile() in could_unit_load()
> (this was probably bug already in find_transport_from_tile()) It means
> that you cannot load to boat in coastal city.

Fixed. New patch attached. I have tested boarding transports in
cities. It works now, at least.

>  You could check that transport can_unit_exist_at_tile(), but I don't
> think that is necessary. can_unit_exist_at_tile() would return FALSE
> only if we attempt recursive transportation, which is prohibited by
> other means already. Assert, maybe?

I think you got some functions mixed up above.

  - Per

Index: server/unittools.c
===
--- server/unittools.c	(revision 12614)
+++ server/unittools.c	(working copy)
@@ -2844,7 +2844,7 @@
 
   /* Special checks for ground units in the ocean. */
   if (!can_unit_survive_at_tile(punit, pdesttile)) {
-ptransporter = find_transporter_for_unit(punit, pdesttile);
+ptransporter = find_transporter_for_unit(punit);
 if (ptransporter) {
   put_unit_onto_transporter(punit, ptransporter);
 }
Index: common/unit.c
===
--- common/unit.c	(revision 12614)
+++ common/unit.c	(working copy)
@@ -543,22 +543,15 @@
 }
 
 /
-  Return TRUE iff the given unit can be loaded into the transporter.
+  Return TRUE iff the given unit could be loaded into the transporter
+  if we moved there.
 /
-bool can_unit_load(const struct unit *pcargo, const struct unit *ptrans)
+bool could_unit_load(const struct unit *pcargo, const struct unit *ptrans)
 {
-  /* This function needs to check EVERYTHING. */
-
   if (!pcargo || !ptrans) {
 return FALSE;
   }
 
-  /* Check positions of the units.  Of course you can't load a unit onto
-   * a transporter on a different tile... */
-  if (!same_pos(pcargo->tile, ptrans->tile)) {
-return FALSE;
-  }
-
   /* Double-check ownership of the units: you can load into an allied unit
* (of course only allied units can be on the same tile). */
   if (!pplayers_allied(unit_owner(pcargo), unit_owner(ptrans))) {
@@ -576,16 +569,37 @@
   }
 
   /* Make sure this transporter can carry this type of unit. */
-  if(!can_unit_transport(ptrans, pcargo)) {
+  if (!can_unit_transport(ptrans, pcargo)) {
 return FALSE;
   }
 
+  /* Transporter must be native to the tile it is on. */
+  if (!can_unit_exist_at_tile(ptrans, ptrans->tile)) {
+return FALSE;
+  }
+
   /* Make sure there's room in the transporter. */
   return (get_transporter_occupancy(ptrans)
 	  < get_transporter_capacity(ptrans));
 }
 
 /
+  Return TRUE iff the given unit can be loaded into the transporter.
+/
+bool can_unit_load(const struct unit *pcargo, const struct unit *ptrans)
+{
+  /* This function needs to check EVERYTHING. */
+
+  /* Check positions of the units.  Of course you can't load a unit onto
+   * a transporter on a different tile... */
+  if (!same_pos(pcargo->tile, ptrans->tile)) {
+return FALSE;
+  }
+
+  return could_unit_load(pcargo, ptrans);
+}
+
+/
   Return TRUE iff the given unit can be unloaded from its current
   transporter.
 
@@ -1392,9 +1406,10 @@
 /
   Find a transporter at the given location for the unit.
 /
-struct unit *find_transporter_for_unit(const struct unit *pcargo,
-   const struct tile *ptile)
-{ 
+struct unit *find_transporter_for_unit(const struct unit *pcargo)
+{
+  struct tile *ptile = pcargo->tile;
+
   unit_list_iterate(ptile->units, ptrans) {
 if (can_unit_load(pcargo, ptrans)) {
   return ptrans;
Index: common/unit.h
===
--- common/unit.h	(revision 12614)
+++ common/unit.h	(working copy)
@@ -216,6 +216,7 @@
 bool unit_can_airlift_to(const struct unit *punit, const struct city *pcity);
 bool unit_has_orders(const struct unit *punit);
 
+bool could_unit_load(const struct unit *pcargo, const struct unit *ptrans);
 bool can_unit_load(const struct unit *punit, const struct unit *ptrans);
 bool can_unit_unload(const struct unit *punit, const struct unit *ptrans);
 bool can_unit_paradrop(const struct unit *punit);
@@ -297,8 +298,7 @@
 void free_unit_orders(struct unit *punit);
 
 int get_transporter_occupancy(const struct unit *ptrans);
-struct unit *find_transporter_for_unit(const struct unit *pcargo,
-  

Re: [Freeciv-Dev] (PR#35641) Unit transport loading cleanup

2007-02-10 Thread Marko Lindqvist

http://bugs.freeciv.org/Ticket/Display.html?id=35641 >

On 2/10/07, Per I. Mathisen <[EMAIL PROTECTED]> wrote:
>
> This patch
>  * Removes the second parameter to find_transporter_for_unit(), which
> is already known.
>  * Adds function could_unit_load() which checks if a given transporter
> could add a unit as cargo, were it on the same tile.
>  * Since find_transporter_for_unit() calls can_unit_load(), it is
> meaningless to call the latter on results from the former. Remove such
> usages.
>  * Make use of all checks in could_unit_load() in
> find_transport_from_tile(), instead of the smaller and duplicating
> list of checks there, that seemed to make no sense.
>
> Can people who have touched this code, please take a look?

 I think you cannot check for is_native_tile() in could_unit_load()
(this was probably bug already in find_transport_from_tile()) It means
that you cannot load to boat in coastal city.
 You could check that transport can_unit_exist_at_tile(), but I don't
think that is necessary. can_unit_exist_at_tile() would return FALSE
only if we attempt recursive transportation, which is prohibited by
other means already. Assert, maybe?


 - ML



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