Standardize all core and obj functions with relative coords

This is a large iteration on a previous implementation of making
nh.getmap() parse its coordinates as relative to the last defined map or
room rather than absolute to the entire level. Now, everything in the
nh.* and obj.* functions interprets coords as relative rather than
absolute. (By default; if no map or room has been defined, or if the lua
code is executing after level creation is done, they will interpret the
coordinates as absolute).

The general motivation is basically the same - routines that use
absolute coordinates are difficult to use in level creation routines,
because then the designer has to remember to convert the relative
coordinate to an absolute one (and that was impossible before
nh.abscoord was added, particularly in themed rooms). And once
nh.getmap() takes relative coordinates, it would be very strange to have
all the other functions (setting timers, burying objects, etc) remain
with absolute ones.

In a couple places, code is changed to account for coordinates that are
relative to a *room* (which uses g.coder->croom->[lx,ly] as an offset,
instead of relative to a *map*, which uses [xstart,ystart].
Specifically, selection.iterate did not account for this, and without
this the ice themed room timer was not being started in the proper
place.

All tests are updated to respect the new behavior. Most of the modified
functions are not actually used anywhere in level files; the one
exception is starting a timer in a themed room, and that has been
adjusted.

Documentation updated as well to clarify when various things are tossing
around relative and absolute coordinates, both in comments and in
lua.adoc.

dat/themerms.lua

@@ -80,8 +80,7 @@ themerooms = {
                     if (percent(25)) then
                        local mintime = 1000 - (nh.level_difficulty() * 100);
                        local ice_melter = function(x,y)
-                          local ax,ay = nh.abscoord(x,y);
+                          nh.start_timer_at(x,y, "melt-ice", mintime + nh.rn2(1000));
-                          nh.start_timer_at(ax,ay, "melt-ice", mintime + nh.rn2(1000));
                        end;
                        ice:iterate(ice_melter);
                     end

doc/lua.adoc

@@ -6,13 +6,16 @@
 
 Functions exposed from the NetHack core. They are all in the `nh` table.
 
+All core functions involving xy coordinates interpret these as relative to the
+last defined map or room.
+
 === abscoord
 
-Convert a room-relative coordinate to map-absolute.
+Convert a room- or map-relative coordinate to absolute.
 Can accept one table with x and y keys (and in that case, returns similar),
 or two integer values (and returns two integer values)
-des-routines tend to use relative coordinates, nh and obj use absolute.
+des, nh, and obj routines all treat inputs as relative coordinates, but this is
-(This mess is still very much in need of improvement.)
+here in case an absolute one is needed for some reason (debugging?).
 
 Example:
 
@@ -385,6 +388,9 @@ Example:
 
 Functions for creating special levels. They are in the `des` table.
 
+All special level functions involving xy coordinates interpret these as relative
+to the last defined map or room.
+
 === altar
 
 Create an altar of certain type and alignment.

include/extern.h

@@ -2610,6 +2610,8 @@ extern void selection_do_gradient(struct selectionvar *, long, long, long,
 extern int lspo_reset_level(lua_State *);
 extern int lspo_finalize_level(lua_State *);
 extern boolean get_coord(lua_State *, int, lua_Integer *, lua_Integer *);
+extern void cvt_to_abscoord(coordxy *, coordxy *);
+extern void cvt_to_relcoord(coordxy *, coordxy *);
 extern int nhl_abs_coord(lua_State *);
 extern void update_croom(void);
 extern const char *get_trapname_bytype(int);

src/nhlobj.c

@@ -363,6 +363,8 @@ l_obj_at(lua_State *L)
 
         x = (coordxy) luaL_checkinteger(L, 1);
         y = (coordxy) luaL_checkinteger(L, 2);
+        cvt_to_abscoord(&x, &y);
+
         lua_pop(L, 2);
         (void) l_obj_push(L, g.level.objects[x][y]);
         return 1;
@@ -386,6 +388,8 @@ l_obj_placeobj(lua_State *L)
 
     x = (coordxy) luaL_checkinteger(L, 2);
     y = (coordxy) luaL_checkinteger(L, 3);
+    cvt_to_abscoord(&x, &y);
+
     lua_pop(L, 3);
 
     if (lobj_is_ok(lo)) {
@@ -569,6 +573,7 @@ l_obj_bury(lua_State *L)
     } else if (argc == 3) {
         x = (coordxy) lua_tointeger(L, 2);
         y = (coordxy) lua_tointeger(L, 3);
+        cvt_to_abscoord(&x, &y);
     } else
         nhl_error(L, "l_obj_bury: Wrong args");
 
@@ -617,7 +622,7 @@ l_obj_register(lua_State *L)
     luaL_setfuncs(L, l_obj_meta, 0);
     /* metatable.__index points at the object method table. */
     lua_pushvalue(L, -2);
-    lua_setfield(L, -2, "__index"); 
+    lua_setfield(L, -2, "__index");
 
     /* Don't let lua code mess with the real metatable.
        Instead offer a fake one that only contains __gc. */

src/nhlsel.c

@@ -839,7 +839,10 @@ l_selection_gradient(lua_State *L)
     return 1;
 }
 
-/* sel:iterate(function(x,y) ... end); */
+/* sel:iterate(function(x,y) ... end);
+ * The x, y coordinates passed to the function are map- or room-relative
+ * rather than absolute, unless there has been no previous map or room defined.
+ */
 static int
 l_selection_iterate(lua_State *L)
 {
@@ -854,9 +857,11 @@ l_selection_iterate(lua_State *L)
         for (y = rect.ly; y <= rect.hy; y++)
             for (x = max(1,rect.lx); x <= rect.hx; x++)
                 if (selection_getpoint(x, y, sel)) {
+                    coordxy tmpx = x, tmpy = y;
+                    cvt_to_relcoord(&tmpx, &tmpy);
                     lua_pushvalue(L, 2);
-                    lua_pushinteger(L, x - g.xstart);
+                    lua_pushinteger(L, tmpx);
-                    lua_pushinteger(L, y - g.ystart);
+                    lua_pushinteger(L, tmpy);
                     if (nhl_pcall(L, 2, 0)) {
                         impossible("Lua error: %s", lua_tostring(L, -1));
                         /* abort the loops to prevent possible error cascade */

src/nhlua.c

@@ -172,7 +172,7 @@ void
 nhl_error(lua_State *L, const char *msg)
 {
     lua_Debug ar;
-    char buf[BUFSZ];
+    char buf[BUFSZ*2];
 
     lua_getstack(L, 1, &ar);
     lua_getinfo(L, "lS", &ar);
@@ -389,6 +389,7 @@ nhl_gettrap(lua_State *L)
     }
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (isok(x, y)) {
             struct trap *ttmp = t_at(x,y);
@@ -439,7 +440,8 @@ nhl_deltrap(lua_State *L)
     }
     x = (coordxy) lx;
     y = (coordxy) ly;
-    
+    cvt_to_abscoord(&x, &y);
+
     if (isok(x, y)) {
         struct trap *ttmp = t_at(x,y);
 
@@ -452,6 +454,11 @@ nhl_deltrap(lua_State *L)
 /* get parameters (XX,YY) or ({ x = XX, y = YY }) or ({ XX, YY }),
    and set the x and y values.
    return TRUE if there are such params in the stack.
+   Note that this does not adjust the values of x and y at all from what is
+   specified in the level file; so, it returns absolute coordinates rather than
+   map-relative coordinates. Callers of this function must decide if they want
+   to interpret the values as absolute or as map-relative, and adjust
+   accordingly.
  */
 boolean
 nhl_get_xy_params(lua_State *L, lua_Integer *x, lua_Integer *y)
@@ -487,23 +494,9 @@ nhl_getmap(lua_State *L)
         nhl_error(L, "Incorrect arguments");
         return 0;
     }
-    if (g.in_mklev) {
-        /* xstart and ystart are set by the des.map() command to give
-         * coordinates relative to the 0,0 of that map. It can be quite useful
-         * to check what terrain is on a given space. But without compensating
-         * for the change in xstart and ystart here, this will lead to results
-         * like des.terrain(4,6,'L') and then nh.getmap(4,6) not being lava.
-         *
-         * Only valid during mklev, because xstart and ystart are not saved
-         * with the level and can change during the level creating process when
-         * additional des.map() are executed. They will not necessarily be the
-         * same later. */
-        x += g.xstart;
-        y += g.ystart;
-    }
-
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (isok(x, y)) {
             char buf[BUFSZ];
@@ -1177,6 +1170,7 @@ nhl_timer_has_at(lua_State *L)
 
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (isok(x, y)) {
         when = spot_time_expires(x, y, timertype);
@@ -1205,6 +1199,7 @@ nhl_timer_peek_at(lua_State *L)
 
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (timer_is_pos(timertype) && isok(x, y))
         when = spot_time_expires(x, y, timertype);
@@ -1230,6 +1225,7 @@ nhl_timer_stop_at(lua_State *L)
 
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (timer_is_pos(timertype) && isok(x, y))
         spot_stop_timers(x, y, timertype);
@@ -1254,6 +1250,7 @@ nhl_timer_start_at(lua_State *L)
 
     x = (coordxy) lx;
     y = (coordxy) ly;
+    cvt_to_abscoord(&x, &y);
 
     if (timer_is_pos(timertype) && isok(x, y)) {
         long where = ((long) x << 16) | (long) y;

src/sp_lev.c

@@ -1150,11 +1150,15 @@ rndtrap(void)
 }
 
 /*
- * Coordinates in special level files are handled specially:
+ * Translate a given coordinate from a special level definition into an actual
+ * location on the map.
  *
- *      if x or y is < 0, we generate a random coordinate.
+ * If x or y is negative, we generate a random coordinate within the area. If
- *      The "humidity" flag is used to ensure that engravings aren't
+ * not negative, they are interpreted as relative to the last defined map or
- *      created underwater, or eels on dry land.
+ * room, and are output as absolute g.level.locations coordinates.
+ *
+ * The "humidity" flag is used to ensure that engravings aren't created
+ * underwater, or eels on dry land.
  */
 static void
 get_location(
@@ -3085,6 +3089,12 @@ get_table_montype(lua_State *L, int *mgender)
     return ret;
 }
 
+/* Get x and y values from a table (which the caller has already checked for the
+ * existence of), handling both a table with x= and y= specified and a table
+ * with coord= specified.
+ * Returns absolute rather than map-relative coordinates; the caller of this
+ * function must decide if it wants to interpret the coordinates as map-relative
+ * and adjust accordingly. */
 static void
 get_table_xy_or_coord(lua_State *L, lua_Integer *x, lua_Integer *y)
 {
@@ -5237,7 +5247,56 @@ l_table_getset_feature_flag(
     }
 }
 
-/* convert relative coordinate to map-absolute.
+/* guts of nhl_abs_coord; convert a coordinate relative to a map or room into an
+ * absolute coordinate in g.level.locations.
+ *
+ * If there is no enclosing map or room, the coordinates are assumed to be
+ * absolute already.
+ *
+ * Part of the reason this is a function is to make it clearer in the calling
+ * code that this conversion is what is intended.
+ *
+ * NOTE: if the coordinates are going to get passed to one of the get_location
+ * family of functions, this should NOT be called; get_location already makes
+ * an adjustment like this. (What this function supports which get_location
+ * doesn't is the input coordinates being negative. get_location will treat that
+ * as "level designer wants a random coordinate".) */
+void
+cvt_to_abscoord(coordxy *x, coordxy *y)
+{
+    /* since commit 99715e0, xstart and ystart are only relevant in mklev when
+     * maps are being used, and 0 otherwise. It is possible in the future that
+     * map positions and dimensions can be saved and retrieved outside of mklev
+     * which would reintroduce nonzero xstart/ystart/xsiz/ysiz, but this is not
+     * currently implemented, so this function can be assumed to have no effect
+     * outside of mklev.
+     */
+    if (g.coder && g.coder->croom) {
+        *x += g.coder->croom->lx;
+        *y += g.coder->croom->ly;
+    }
+    else {
+        *x += g.xstart;
+        *y += g.ystart;
+    }
+}
+
+/* inverse of cvt_to_abscoord; turn an absolute g.level.locations coordinate
+ * into one relative to the current map or room. */
+void
+cvt_to_relcoord(coordxy *x, coordxy *y)
+{
+    if (g.coder && g.coder->croom) {
+        *x -= g.coder->croom->lx;
+        *y -= g.coder->croom->ly;
+    }
+    else {
+        *x -= g.xstart;
+        *y -= g.ystart;
+    }
+}
+
+/* convert map-relative coordinate to absolute.
   local ax,ay = nh.abscoord(rx, ry);
   local pt = nh.abscoord({ x = 10, y = 5 });
  */
@@ -5250,16 +5309,14 @@ nhl_abs_coord(lua_State *L)
     if (argc == 2) {
         x = (coordxy) lua_tointeger(L, 1);
         y = (coordxy) lua_tointeger(L, 2);
-        x += g.xstart;
+        cvt_to_abscoord(&x, &y);
-        y += g.ystart;
         lua_pushinteger(L, x);
         lua_pushinteger(L, y);
         return 2;
     } else if (argc == 1 && lua_type(L, 1) == LUA_TTABLE) {
         x = (coordxy) get_table_int(L, "x");
         y = (coordxy) get_table_int(L, "y");
-        x += g.xstart;
+        cvt_to_abscoord(&x, &y);
-        y += g.ystart;
         lua_newtable(L);
         nhl_add_table_entry_int(L, "x", x);
         nhl_add_table_entry_int(L, "y", y);

test/test_des.lua

@@ -3,7 +3,7 @@
 -- reset_level is only needed here, not in normal special level scripts.
 
 function is_map_at(x,y, mapch, lit)
-   local rm = nh.getmap(x + 1, y); -- + 1 == g.xstart
+   local rm = nh.getmap(x, y);
    if rm.mapchr ~= mapch then
       error("Terrain at (" .. x .. "," .. y .. ") is not \"" .. mapch .. "\", but \"" .. rm.mapchr .. "\"");
    end
@@ -29,7 +29,7 @@ function check_loc_flag(x, y, flag, value)
 end
 
 function check_trap_at(x,y, name)
-   local t = nh.gettrap(x + 1, y); -- + 1 == g.xstart
+   local t = nh.gettrap(x, y);
    if (t.ttyp_name ~= name) then
       error("Trap at " .. x .. "," .. y .. " is " .. t.ttyp_name .. ", not " .. name);
    end
@@ -211,10 +211,10 @@ LTL]])
 FFF
 F.F
 FFF]] })
-   for x = 60, 62 do
+   for x = 0,2 do
-      for y = 5, 7 do
+      for y = 0,2 do
          local nam = "iron bars";
-         if (x == 61 and y == 6) then
+         if (x == 1 and y == 1) then
              nam = "room";
          end
          check_loc_name(x, y, nam);
@@ -224,10 +224,10 @@ FFF]] })
 ...
 .T.
 ...]] })
-   for x = 60, 62 do
+   for x = 0, 2 do
-      for y = 5, 7 do
+      for y = 0, 2 do
          local nam = "room";
-         if (x == 61 and y == 6) then
+         if (x == 1 and y == 1) then
              nam = "tree";
          end
          check_loc_name(x, y, nam);
@@ -244,50 +244,50 @@ III]] })
                 des.terrain(0,0, "L");
                 des.terrain(map.width-1,map.height-1, "T");
    end});
-   check_loc_name(30, 5, "lava pool");
+   check_loc_name(0, 0, "lava pool");
-   check_loc_name(32, 7, "tree");
+   check_loc_name(2, 2, "tree");
 end
 
 function test_feature()
    des.reset_level();
    des.level_init({ style = "solidfill", fg = ".", lit = 1 });
    des.feature("fountain", 40, 08);
-   check_loc_name(40 + 1, 08, "fountain");
+   check_loc_name(40, 08, "fountain");
    des.feature("sink", {41, 08});
-   check_loc_name(41 + 1, 08, "sink");
+   check_loc_name(41, 08, "sink");
    des.feature({ type = "pool", x = 42, y = 08 });
-   check_loc_name(42 + 1, 08, "pool");
+   check_loc_name(42, 08, "pool");
    des.feature({ type = "sink", coord = {43, 08} });
-   check_loc_name(43 + 1, 08, "sink");
+   check_loc_name(43, 08, "sink");
 
    des.feature({ type = "throne", coord = {44, 08}, looted=true });
-   check_loc_name(44 + 1, 08, "throne");
+   check_loc_name(44, 08, "throne");
-   check_loc_flag(44 + 1, 08, "looted", true);
+   check_loc_flag(44, 08, "looted", true);
 
    des.feature({ type = "throne", coord = {44, 08}, looted=false });
-   check_loc_name(44 + 1, 08, "throne");
+   check_loc_name(44, 08, "throne");
-   check_loc_flag(44 + 1, 08, "looted", false);
+   check_loc_flag(44, 08, "looted", false);
 
    des.feature({ type = "tree", coord = {45, 08}, looted=true, swarm=false });
-   check_loc_name(45 + 1, 08, "tree");
+   check_loc_name(45, 08, "tree");
-   check_loc_flag(45 + 1, 08, "looted", true);
+   check_loc_flag(45, 08, "looted", true);
-   check_loc_flag(45 + 1, 08, "swarm", false);
+   check_loc_flag(45, 08, "swarm", false);
 
    des.feature({ type = "tree", coord = {45, 08}, looted=false, swarm=true });
-   check_loc_name(45 + 1, 08, "tree");
+   check_loc_name(45, 08, "tree");
-   check_loc_flag(45 + 1, 08, "looted", false);
+   check_loc_flag(45, 08, "looted", false);
-   check_loc_flag(45 + 1, 08, "swarm", true);
+   check_loc_flag(45, 08, "swarm", true);
 
    des.feature({ type = "fountain", coord = {46, 08}, looted=false, warned=true });
-   check_loc_name(46 + 1, 08, "fountain");
+   check_loc_name(46, 08, "fountain");
-   check_loc_flag(46 + 1, 08, "looted", false);
+   check_loc_flag(46, 08, "looted", false);
-   check_loc_flag(46 + 1, 08, "warned", true);
+   check_loc_flag(46, 08, "warned", true);
 
    des.feature({ type = "sink", coord = {47, 08}, pudding=false, dishwasher=true, ring=true });
-   check_loc_name(47 + 1, 08, "sink");
+   check_loc_name(47, 08, "sink");
-   check_loc_flag(47 + 1, 08, "pudding", false);
+   check_loc_flag(47, 08, "pudding", false);
-   check_loc_flag(47 + 1, 08, "dishwasher", true);
+   check_loc_flag(47, 08, "dishwasher", true);
-   check_loc_flag(47 + 1, 08, "ring", true);
+   check_loc_flag(47, 08, "ring", true);
 end
 
 function test_gold()

test/test_sel.lua

@@ -40,7 +40,7 @@ function sel_are_equal(sela, selb, msg)
 end
 
 function is_map_at(x,y, mapch, lit)
-   local rm = nh.getmap(x + 1, y); -- + 1 == g.xstart
+   local rm = nh.getmap(x, y); 
    if rm.mapchr ~= mapch then
       error("Terrain at (" .. x .. "," .. y .. ") is not \"" .. mapch .. "\", but \"" .. rm.mapchr .. "\"");
    end