--- deliantra/server/common/object.C	2008/04/30 08:29:31	1.223
+++ deliantra/server/common/object.C	2008/05/06 20:08:37	1.236
@@ -141,33 +141,18 @@
 static bool
 compare_ob_value_lists_one (const object *wants, const object *has)
 {
-  key_value *wants_field;
-
-  /* n-squared behaviour (see get_ob_key_link()), but I'm hoping both
+  /* n-squared behaviour (see kv_get), but I'm hoping both
    * objects with lists are rare, and lists stay short. If not, use a
    * different structure or at least keep the lists sorted... 
    */
 
   /* For each field in wants, */
-  for (wants_field = wants->key_values; wants_field; wants_field = wants_field->next)
-    {
-      key_value *has_field;
-
-      /* Look for a field in has with the same key. */
-      has_field = get_ob_key_link (has, wants_field->key);
-
-      if (!has_field)
-        return 0; /* No field with that name. */
-
-      /* Found the matching field. */
-      if (has_field->value != wants_field->value)
-        return 0; /* Values don't match, so this half of the comparison is false. */
-
-      /* If we get here, we found a match. Now for the next field in wants. */
-    }
+  for (key_value *kv = wants->key_values; kv; kv = kv->next)
+    if (has->kv_get (kv->key) != kv->value)
+      return false;
 
   /* If we get here, every field in wants has a matching field in has. */
-  return 1;
+  return true;
 }
 
 /* Returns TRUE if ob1 has the same key_values as ob2. */
@@ -177,7 +162,8 @@
   /* However, there may be fields in has which aren't partnered in wants,
    * so we need to run the comparison *twice*. :(
    */
-  return compare_ob_value_lists_one (ob1, ob2) && compare_ob_value_lists_one (ob2, ob1);
+  return compare_ob_value_lists_one (ob1, ob2)
+      && compare_ob_value_lists_one (ob2, ob1);
 }
 
 /* Function examines the 2 objects given to it, and returns true if
@@ -370,7 +356,7 @@
 
 // adjust weight per container type ("of holding")
 static sint32
-weight_adjust (object *op, sint32 weight)
+weight_adjust_for (object *op, sint32 weight)
 {
   return op->type == CONTAINER
          ? lerp (weight, 0, 100, 0, 100 - op->stats.Str)
@@ -386,7 +372,10 @@
 {
   while (op)
     {
-      weight = weight_adjust (op, weight);
+      // adjust by actual difference to account for rounding errors
+      // i.e. (w2 - w1) / f != w2 / f - w1 / f and the latter is correct
+      weight = weight_adjust_for (op, op->carrying)
+             - weight_adjust_for (op, op->carrying - weight);
 
       if (!weight)
         return;
@@ -419,7 +408,7 @@
       sum += op->total_weight ();
     }
 
-  sum = weight_adjust (this, sum);
+  sum = weight_adjust_for (this, sum);
 
   if (sum != carrying)
     {
@@ -605,35 +594,39 @@
   op->key_values = 0;
 }
 
-object &
-object::operator =(const object &src)
+/*
+ * copy_to first frees everything allocated by the dst object,
+ * and then copies the contents of itself into the second
+ * object, allocating what needs to be allocated.  Basically, any
+ * data that is malloc'd needs to be re-malloc/copied.  Otherwise,
+ * if the first object is freed, the pointers in the new object
+ * will point at garbage.
+ */
+void
+object::copy_to (object *dst)
 {
-  bool is_freed   = flag [FLAG_FREED];
-  bool is_removed = flag [FLAG_REMOVED];
-
-  *(object_copy *)this = src;
-
-  flag [FLAG_FREED]   = is_freed;
-  flag [FLAG_REMOVED] = is_removed;
+  dst->remove ();
+  *(object_copy *)dst = *this;
+  dst->flag [FLAG_REMOVED] = true;
 
   /* Copy over key_values, if any. */
-  if (src.key_values)
+  if (key_values)
     {
       key_value *tail = 0;
-      key_values = 0;
+      dst->key_values = 0;
 
-      for (key_value *i = src.key_values; i; i = i->next)
+      for (key_value *i = key_values; i; i = i->next)
         {
           key_value *new_link = new key_value;
 
-          new_link->next = 0;
-          new_link->key = i->key;
+          new_link->next  = 0;
+          new_link->key   = i->key;
           new_link->value = i->value;
 
           /* Try and be clever here, too. */
-          if (!key_values)
+          if (!dst->key_values)
             {
-              key_values = new_link;
+              dst->key_values = new_link;
               tail = new_link;
             }
           else
@@ -643,20 +636,6 @@
             }
         }
     }
-}
-
-/*
- * copy_to first frees everything allocated by the dst object,
- * and then copies the contents of itself into the second
- * object, allocating what needs to be allocated.  Basically, any
- * data that is malloc'd needs to be re-malloc/copied.  Otherwise,
- * if the first object is freed, the pointers in the new object
- * will point at garbage.
- */
-void
-object::copy_to (object *dst)
-{
-  *dst = *this;
 
   if (speed < 0)
     dst->speed_left -= rndm ();
@@ -688,6 +667,7 @@
 {
   object *neu = create ();
   copy_to (neu);
+  neu->map = map; // not copied by copy_to
   return neu;
 }
 
@@ -974,6 +954,11 @@
     alloc ();
     in_memory = MAP_ACTIVE;
   }
+
+  ~freed_map ()
+  {
+    destroy ();
+  }
 } freed_map; // freed objects are moved here to avoid crashes
 
 void
@@ -1074,10 +1059,13 @@
 
       adjust_weight (env, -total_weight ());
 
+      // make sure cmov optimisation is applicable
       *(above ? &above->below : &env->inv) = below;
+      *(below ? &below->above : &above   ) = above; // &above is just a dummy
 
-      if (below)
-        below->above = above;
+      above = 0;
+      below = 0;
+      env   = 0;
 
       /* we set up values so that it could be inserted into
        * the map, but we don't actually do that - it is up
@@ -1086,9 +1074,6 @@
       map   = env->map;
       x     = env->x;
       y     = env->y;
-      above = 0;
-      below = 0;
-      env   = 0;
 
       /* NO_FIX_PLAYER is set when a great many changes are being
        * made to players inventory.  If set, avoiding the call
@@ -1125,25 +1110,9 @@
         }
 
       /* link the object above us */
-      if (above)
-        above->below = below;
-      else
-        ms.top = below;       /* we were top, set new top */
-
-      /* Relink the object below us, if there is one */
-      if (below)
-        below->above = above;
-      else
-        {
-          /* Nothing below, which means we need to relink map object for this space 
-           * use translated coordinates in case some oddness with map tiling is
-           * evident
-           */
-          if (GET_MAP_OB (map, x, y) != this)
-            LOG (llevError, "remove_ob: GET_MAP_OB does not return object to be removed even though it appears to be on the bottom? %s\n", debug_desc ());
-
-          ms.bot = above;      /* goes on above it. */
-        }
+      // re-link, make sure compiler can easily use cmove
+      *(above ? &above->below : &ms.top) = below;
+      *(below ? &below->above : &ms.bot) = above;
 
       above = 0;
       below = 0;
@@ -1163,7 +1132,10 @@
             pl->close_container ();
 
           //TODO: the floorbox prev/next might need updating
-          esrv_del_item (pl->contr, count);
+          //esrv_del_item (pl->contr, count);
+          //TODO: update floorbox to preserve ordering
+          if (pl->contr->ns)
+            pl->contr->ns->floorbox_update ();
         }
 
       for (tmp = ms.bot; tmp; tmp = tmp->above)
@@ -1307,7 +1279,7 @@
    */
   if (!xy_normalise (m, op->x, op->y))
     {
-      op->destroy (1);
+      op->head_ ()->destroy (1);// remove head_ once all tail object destroyers found
       return 0;
     }
 
@@ -1471,7 +1443,10 @@
 
   if (object *pl = ms.player ())
     //TODO: the floorbox prev/next might need updating
-    esrv_send_item (pl, op);
+    //esrv_send_item (pl, op);
+    //TODO: update floorbox to preserve ordering
+    if (pl->contr->ns)
+      pl->contr->ns->floorbox_update ();
 
   /* If this object glows, it may affect lighting conditions that are
    * visible to others on this map.  But update_all_los is really
@@ -1601,7 +1576,7 @@
     {
       decrease (nr);
 
-      object *op = object_create_clone (this);
+      object *op = deep_clone ();
       op->nrof = nr;
       return op;
     }
@@ -1832,7 +1807,7 @@
     }
 
   for (object *tmp = m->at (x, y).bot; tmp; tmp = tmp->above)
-    if (tmp->arch == at)
+    if (tmp->arch->archname == at->archname)
       return tmp;
 
   return NULL;
@@ -1906,8 +1881,8 @@
 object *
 present_arch_in_ob (const archetype *at, const object *op)
 {
-  for (object *tmp = op->inv; tmp != NULL; tmp = tmp->below)
-    if (tmp->arch == at)
+  for (object *tmp = op->inv; tmp; tmp = tmp->below)
+    if (tmp->arch->archname == at->archname)
       return tmp;
 
   return NULL;
@@ -2326,40 +2301,23 @@
  * create clone from object to another
  */
 object *
-object_create_clone (object *asrc)
+object::deep_clone ()
 {
-  object *dst = 0, *tmp, *src, *prev, *item;
-
-  if (!asrc)
-    return 0;
+  assert (("deep_clone called on non-head object", is_head ()));
 
-  src = asrc->head_ ();
+  object *dst = clone ();
 
-  prev = 0;
-  for (object *part = src; part; part = part->more)
+  object *prev = dst;
+  for (object *part = this->more; part; part = part->more)
     {
-      tmp = part->clone ();
-      tmp->x -= src->x;
-      tmp->y -= src->y;
-
-      if (!part->head)
-        {
-          dst = tmp;
-          tmp->head = 0;
-        }
-      else
-        tmp->head = dst;
-
-      tmp->more = 0;
-
-      if (prev)
-        prev->more = tmp;
-
+      object *tmp = part->clone ();
+      tmp->head = dst;
+      prev->more = tmp;
       prev = tmp;
     }
 
-  for (item = src->inv; item; item = item->below)
-    insert_ob_in_ob (object_create_clone (item), dst);
+  for (object *item = inv; item; item = item->below)
+    insert_ob_in_ob (item->deep_clone (), dst);
 
   return dst;
 }
@@ -2378,143 +2336,46 @@
   return 0;
 }
 
-/* If ob has a field named key, return the link from the list,
- * otherwise return NULL. 
- *
- * key must be a passed in shared string - otherwise, this won't
- * do the desired thing.
- */
-key_value *
-get_ob_key_link (const object *ob, const char *key)
+const shstr &
+object::kv_get (const shstr &key) const
 {
-  for (key_value *link = ob->key_values; link; link = link->next)
-    if (link->key == key)
-      return link;
-
-  return 0;
-}
-
-/* 
- * Returns the value of op has an extra_field for key, or NULL.
- *
- * The argument doesn't need to be a shared string.
- *
- * The returned string is shared.
- */
-const char *
-get_ob_key_value (const object *op, const char *const key)
-{
-  key_value *link;
-  shstr_cmp canonical_key (key);
-
-  if (!canonical_key)
-    {
-      /* 1. There being a field named key on any object
-       *    implies there'd be a shared string to find.
-       * 2. Since there isn't, no object has this field.
-       * 3. Therefore, *this* object doesn't have this field.
-       */
-      return 0;
-    }
-
-  /* This is copied from get_ob_key_link() above -
-   * only 4 lines, and saves the function call overhead.
-   */
-  for (link = op->key_values; link; link = link->next)
-    if (link->key == canonical_key)
-      return link->value;
+  for (key_value *kv = key_values; kv; kv = kv->next)
+    if (kv->key == key)
+      return kv->value;
 
-  return 0;
+  return shstr_null;
 }
 
-/*
- * Updates the canonical_key in op to value.
- *
- * canonical_key is a shared string (value doesn't have to be).
- *
- * Unless add_key is TRUE, it won't add fields, only change the value of existing
- * keys.
- *
- * Returns TRUE on success.
- */
-int
-set_ob_key_value_s (object *op, const shstr & canonical_key, const char *value, int add_key)
+void
+object::kv_set (const shstr &key, const shstr &value)
 {
-  key_value *field = NULL, *last = NULL;
-
-  for (field = op->key_values; field != NULL; field = field->next)
-    {
-      if (field->key != canonical_key)
-        {
-          last = field;
-          continue;
-        }
-
-      if (value)
-        field->value = value;
-      else
-        {
-          /* Basically, if the archetype has this key set,
-           * we need to store the null value so when we save
-           * it, we save the empty value so that when we load,
-           * we get this value back again.
-           */
-          if (get_ob_key_link (op->arch, canonical_key))
-            field->value = 0;
-          else
-            {
-              if (last)
-                last->next = field->next;
-              else
-                op->key_values = field->next;
-
-              delete field;
-            }
-        }
-      return TRUE;
-    }
-  /* IF we get here, key doesn't exist */
-
-  /* No field, we'll have to add it. */
-
-  if (!add_key)
-    return FALSE;
-
-  /* There isn't any good reason to store a null
-   * value in the key/value list.  If the archetype has
-   * this key, then we should also have it, so shouldn't
-   * be here.  If user wants to store empty strings,
-   * should pass in ""
-   */
-  if (value == NULL)
-    return TRUE;
+  for (key_value *kv = key_values; kv; kv = kv->next)
+    if (kv->key == key)
+      {
+        kv->value = value;
+        return;
+      }
 
-  field = new key_value;
+  key_value *kv = new key_value;
 
-  field->key = canonical_key;
-  field->value = value;
-  /* Usual prepend-addition. */
-  field->next = op->key_values;
-  op->key_values = field;
+  kv->next  = key_values;
+  kv->key   = key;
+  kv->value = value;
 
-  return TRUE;
+  key_values = kv;
 }
 
-/*
- * Updates the key in op to value.
- *
- * If add_key is FALSE, this will only update existing keys,
- * and not add new ones.
- * In general, should be little reason FALSE is ever passed in for add_key
- *
- * Returns TRUE on success.
- */
-int
-set_ob_key_value (object *op, const char *key, const char *value, int add_key)
+void
+object::kv_del (const shstr &key)
 {
-  shstr key_ (key);
-
-  return set_ob_key_value_s (op, key_, value, add_key);
+  for (key_value **kvp = &key_values; *kvp; kvp = &(*kvp)->next)
+    if ((*kvp)->key == key)
+      {
+        key_value *kv = *kvp;
+        *kvp = (*kvp)->next;
+        delete kv;
+        return;
+      }
 }
 
 object::depth_iterator::depth_iterator (object *container)