[Freeciv-Dev] Re: (PR#12833) consider_settler_action simplification (rev.)

["Brian Dunstan" <bdunstan149@xxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12833 >

Yes here is a simple patch.  The factor used (1024) is
big enough to prevent rounding error but small enough
to avoid overflow risk.  The use of MAX was not
necessary; there is no chance of amortize() being fed
a negative number.

-Brian


--- Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> <URL:
> http://bugs.freeciv.org/Ticket/Display.html?id=12833
> >
> 
> Brian Dunstan wrote:
> 
> > My mistake, corrected below.  My earlier patch has
> the
> > exra still in there :)   Extra is nice also to
> make
> > sure pollution is cleaned up.
> > 
> > 
> >>>====================
> >>>I think this would suffice:
> >>>===================
> >>>
> >>>if (consider) {
> >>>    base_value = 
> >>>      MAX(0, new_tile_value - old_tile_value) +
> >>>      MAX(0, extra);
> >>>
> >>>    discount_value = amortize(base_value, delay);
> >>>    total_value = MAX(0, discount_value);
> >>>
> >>>  } else {
> >>>    total_value = 0;
> >>>  }
> 
> OK, but still we shouldn't get rid of inuse (until
> we have something
> better to replace it with - maybe x2 instead of x4)
> and the MAX
> shouldn't go on until the end (if at all) - a
> negative should be
> considered as negative not as 0!
> 
> And as long as we're working with integer values
> (which we shouldn't be)
> the multiplication-by-a-large-factor is needed to
> prevent rounding problems.
> 
> What about...
> 
>   if (consider) {
>     value = new_tile_value - old_tile_value;
> 
>     if (!inuse) {
>       value /= 2; /* rounding error, sigh... */
>     }
> 
>     value += MAX(extra, 0);
> 
>     value *= 64; /* Or some other large number... */
> 
>    value = amortize(value, delay);
>   }
> 
> I think the rest of your patch (MAX_WORKERS, other
> handling of inuse)
> isn't as clearly good as these changes.  Can you
> make a simple patch
> that just fixes the base calculation?
> 
> -jason
> 
> 
> 
> 
> 


                
__________________________________ 
Do you Yahoo!? 
Plan great trips with Yahoo! Travel: Now over 17,000 guides!
http://travel.yahoo.com/p-travelguide

diff -Nur -Xfreeciv/diff_ignore freeciv/server/settlers.c 
freeciv_altered/server/settlers.c
--- freeciv/server/settlers.c   2005-04-17 13:57:00.255465488 -0400
+++ freeciv_altered/server/settlers.c   2005-04-18 23:35:33.872397080 -0400
@@ -766,39 +766,30 @@
                                    struct tile **best_tile,
                                     struct tile *ptile)
 {
-  int discount_value, base_value = 0;
-  int total_value;
   bool consider;
-
+  int base_value=0, total_value=0;
+  
   if (extra >= 0) {
     consider = TRUE;
   } else {
     consider = (new_tile_value > old_tile_value);
+    extra = 0;
   }
 
+  /* find the present value of the future benefit of this action */
   if (consider) {
-    int diff = new_tile_value - old_tile_value;
+    base_value = new_tile_value - old_tile_value + extra;
+
+    /* use factor to prevent rounding errors */
+    total_value = amortize((1024* base_value), delay) / 1024;
 
-    /* The 64x is because we are dealing with small ints, usually from 0-20,
-     * which are insufficiently large to use directly in amortize().  Tiles
-     * which are not currently in use do not give us an improvement until
-     * a citizen works them, so they are reduced in value by 1/4. */
-    base_value = diff * (in_use ? 64 : 16) + extra * 64;
-    base_value = MAX(0, base_value);
-
-    discount_value = amortize(base_value, delay);
-
-    /* The total value is (roughly) equal to the base value multiplied by
-     * d / (1 - d), where d is the discount. (discount_value / base value)
-     * The MAX is a guard against the base value being greater or equal
-     * than the discount value, which would only happen if it or the 
-     * delay is <= 0. */
-    total_value = ((discount_value * base_value)
-                  / (MAX(1, base_value - discount_value))) / 64;
+    if(!in_use) {
+      total_value /= 2;
+    }
   } else {
     total_value = 0;
   }
-
+  
   if (total_value > *best_value
       || (total_value == *best_value
          && old_tile_value > *best_old_tile_value)) {