FIX: step -> jericho_step to avoid name clashing with regexp.c:step function; ENH: filter_candidate_actions relies on state hashes instead world_diff.

MarcCote · MarcCote · commit 3fef4a6cf31d · 2021-11-26T14:06:08.000-05:00
diff --git a/frotz/Makefile b/frotz/Makefile
@@ -20,7 +20,8 @@ CFLAGS += -w
 # OPTS = -O3 -fomit-frame-pointer -falign-functions=2 -falign-loops=2 -falign-jumps=2 -fPIC
 OPTS = -O3 -fPIC
 # These are handy for debugging.
-# OPTS = $(CFLAGS) -g -fPIC
+# OPTS = $(CFLAGS) -O3 -g -fPIC  # With optimization (faster code)
+# OPTS = $(CFLAGS) -g -fPIC		 # Without optimization (slower code)
 # These are handy for profiling
 # OPTS = $(CFLAGS) -pg -fPIC
 
diff --git a/frotz/src/interface/frotz_interface.c b/frotz/src/interface/frotz_interface.c
@@ -92,8 +92,6 @@ zobject* new_objs = NULL;
 int ram_diff_cnt;
 zword ram_diff_addr[16];
 zword ram_diff_value[16];
-//char last_state_hash[64];
-//char current_state_hash[64];
 bool state_has_changed = FALSE;
 
 
@@ -187,7 +185,7 @@ enum SUPPORTED {
 
 // Set ROM_IDX according to the story_file.
 void load_rom_bindings(char *story_file) {
-  char md5_hash[64];
+  char md5_hash[32];
   char *start;
 
   FILE * f = fopen (story_file, "r");
@@ -333,6 +331,14 @@ void shutdown() {
     free(special_ram_values);
     special_ram_values = NULL;
   }
+  if (new_objs != NULL) {
+    free(new_objs);
+    new_objs = NULL;
+  }
+  if (old_objs != NULL) {
+    free(old_objs);
+    old_objs = NULL;
+  }
 }
 
 // Save the state of the game into a string buffer
@@ -1568,10 +1574,6 @@ char* setup(char *story_file, int seed, void *rom, size_t rom_size) {
     run_free();
   }
 
-  // Initialize last and current state hashes.
-  // get_world_state_hash(last_state_hash);
-  // get_world_state_hash(current_state_hash);
-
   // Concatenate upper and lower screens.
   strcat(world, dumb_get_lower_screen());
   strcat(world, dumb_get_upper_screen());
@@ -1583,24 +1585,16 @@ char* setup(char *story_file, int seed, void *rom, size_t rom_size) {
   return world;
 }
 
-char* step(char *next_action) {
-  char* text;
-  char last_state_hash[64];
-  char current_state_hash[64];
-
+char* jericho_step(char *next_action) {
   if (emulator_halted > 0)
     return halted_message;
 
-  // get_world_state_hash(last_state_hash);
-
   // Swap old_objs and new_objs.
   zobject* tmp;
   tmp = old_objs;
   old_objs = new_objs;
   new_objs = tmp;
 
-  // printf("%x, %x\n", old_objs, new_objs);
-
   // Clear the object, attr, and ram diffs
   move_diff_cnt = 0;
   attr_diff_cnt = 0;
@@ -1617,20 +1611,7 @@ char* step(char *next_action) {
 
   // Check for changes to special ram
   update_ram_diff();
-  //get_world_state_hash(current_state_hash);
-
   update_objs_tracker();
-  // // memset(new_objs, 0, (get_num_world_objs() + 1) * sizeof(zobject));
-  // get_world_objects(new_objs, TRUE);
-
-  // // For a more robust state hash, do not include siblings and children
-  // // since their ordering in memory may change.
-  // for (int i=1; i<=get_num_world_objs(); ++i) {
-  //   new_objs[i].sibling = 0;
-  //   new_objs[i].child = 0;
-  // }
-
-  //state_has_changed = strcmp(current_state_hash, last_state_hash) != 0;
   state_has_changed = memcmp(old_objs, new_objs, (get_num_world_objs() + 1) * sizeof(zobject)) != 0;
   // printf("%s =(%d)= %s <== %s", current_state_hash, state_has_changed, last_state_hash, next_action);
 
@@ -1715,33 +1696,7 @@ char* get_current_state() {
 // Returns 1 if the last action changed the state of the world.
 int world_changed() {
   int objs_has_changed = state_has_changed;
-  //return state_has_changed;
-
-  // int i;
-  // for (i=0; i<move_diff_cnt; ++i) {
-  //   if (ignore_moved_obj(move_diff_objs[i], move_diff_dest[i])) {
-  //     continue;
-  //   }
-  //   return 1;
-  // }
-  // for (i=0; i<attr_diff_cnt; ++i) {
-  //   if (ignore_attr_diff(attr_diff_objs[i], attr_diff_nb[i])) {
-  //     continue;
-  //   }
-  //   return 1;
-  // }
-  // for (i=0; i<attr_clr_cnt; ++i) {
-  //   if (ignore_attr_clr(attr_clr_objs[i], attr_clr_nb[i])) {
-  //     continue;
-  //   }
-  //   return 1;
-  // }
-  // for (i=0; i<prop_put_cnt; ++i) {
-  //   // if (ignore_prop_put(prop_put_objs[i], prop_put_nb[i])) {
-  //   //   continue;
-  //   // }
-  //   return 1;
-  // }
+
   if (game_over() > 0 || victory() > 0) {
     return 1;
   }
@@ -1766,10 +1721,6 @@ void get_object(zobject *obj, zword obj_num) {
   zbyte length;
   LOW_BYTE(obj_name_addr, length);
 
-  // if (length <= 0 || length > 64) {
-  //  return;
-  // }
-
   (*obj).num = obj_num;
 
   if (length > 0 && length <= 64) {
@@ -1985,54 +1936,26 @@ void get_world_state_hash(char* md5_hash) {
 
   int n_objs = get_num_world_objs() + 1; // Add extra spot for zero'th object
   size_t objs_size = n_objs * sizeof(zobject);
-  zobject* objs = calloc(n_objs, sizeof(zobject));
 
   size_t ram_size = get_special_ram_size() * sizeof(unsigned char);
   unsigned char* ram = malloc(ram_size);
   get_special_ram(ram);
 
   int retPC = getRetPC();
 
-  get_world_objects(objs, TRUE);
-
-  // For a more robust state hash, do not include siblings and children
-  // since their ordering in memory may change.
-  for (int i=1; i<=get_num_world_objs(); ++i) {
-    objs[i].sibling = 0;
-    objs[i].child = 0;
-  }
-
-  // Debug print.
-  // printf("\n--Start-\n");
-  // for (int k=0; k != n_objs; ++k) {
-  //   print_object2(&objs[k]);
-  // }
-  // printf("\n--End-\n");
-
-  // char temp_md5_hash[64];
-  // FILE* tmp_fp = fmemopen(objs, objs_size, "rb");
-  // sum(tmp_fp, temp_md5_hash);
-  // fclose(tmp_fp);
-  // printf("temp_md5_hash: %s", temp_md5_hash);
-
   size_t state_size = objs_size + ram_size + sizeof(int);
   char* state = malloc(state_size);
-  memcpy(state, objs, objs_size);
+  memcpy(state, new_objs, objs_size);
   memcpy(&state[objs_size], ram, ram_size);
   memcpy(&state[objs_size+ram_size], &retPC, sizeof(int));
 
-  // int chk = checksum(state, state_size);
-  // sprintf(md5_hash, "%2X", chk);
-
   FILE* fp = fmemopen(state, state_size, "rb");
   sum(fp, md5_hash);
   fclose(fp);
 
   // printf("md5 (objs): %s\n", md5_hash);
 
-  free(objs);
   free(state);
-  return 0;
 }
 
 // Teleports an object (and all children) to the desired destination
@@ -2063,9 +1986,10 @@ void test() {
 // candidate_actions contains a string with all the candidate actions, seperated by ';'
 // valid_actions will be written with each of the identified valid actions seperated by ';'
 // diff_array will be written with the world_diff for each valid_action indicating
-// which of the valid actions are equivalent to each other in terms of their world diffs.
+// which of the valid actions are equivalent to each other in terms of their state hashes.
 // Returns the number of valid actions found.
-int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword *diff_array) {
+int filter_candidate_actions(char *candidate_actions, char *valid_actions, char *hashes) {
+
   char *act = NULL;
   char *act_newline = NULL;
   char *text;
@@ -2106,22 +2030,7 @@ int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword
     act_newline = malloc(strlen(act) + 2);
     strcpy(act_newline, act);
     strcat(act_newline, "\n");
-
-    // Step: Code is copied due to inexplicable segfault when calling step() directly; Ugh!
-    move_diff_cnt = 0;
-    attr_diff_cnt = 0;
-    attr_clr_cnt = 0;
-    prop_put_cnt = 0;
-    ram_diff_cnt = 0;
-    update_special_ram();
-    dumb_set_next_action(act_newline);
-    zstep();
-    run_free();
-    update_ram_diff();
-    text = dumb_get_screen();
-    text = clean_observation(text);
-    strcpy(world, text);
-    dumb_clear_screen();
+    text = jericho_step(act_newline);
 
     if (emulator_halted > 0) {
       printf("Emulator halted on action: %s\n", act);
@@ -2137,7 +2046,7 @@ int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword
         valid_actions[v_idx++] = ';';
 
         // Write the world diff resulting from the last action.
-        get_cleaned_world_diff(&diff_array[128*valid_cnt], &diff_array[(128*valid_cnt) + 64]);  // TODO: replace 128
+        get_world_state_hash(&hashes[32*valid_cnt]);
         valid_cnt++;
       }
     }
@@ -2151,6 +2060,7 @@ int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword
     setFP(fp_cpy);
     set_opcode(next_opcode_cpy);
     setFrameCount(frame_count_cpy);
+    update_objs_tracker();
 
     act = strtok(NULL, ";");
     free(act_newline);
diff --git a/frotz/src/interface/frotz_interface.h b/frotz/src/interface/frotz_interface.h
@@ -39,7 +39,7 @@ extern char* setup(char *story_file, int seed, void* rom, size_t rom_size);
 
 extern void shutdown();
 
-extern char* step(char *next_action);
+extern char* jericho_step(char *next_action);
 
 extern int save(char *filename);
 
@@ -51,7 +51,8 @@ extern int getRAMSize();
 
 extern void getRAM(unsigned char *ram);
 
-int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword *diff_array);
+// int filter_candidate_actions(char *candidate_actions, char *valid_actions, zword *diff_array);
+int filter_candidate_actions(char *candidate_actions, char *valid_actions, char *hashes);
 
 extern char world[256 + 8192];
 
diff --git a/jericho/jericho.py b/jericho/jericho.py
@@ -238,8 +238,8 @@ def _load_frotz_lib():
     frotz_lib.setup.restype = c_char_p
     frotz_lib.shutdown.argtypes = []
     frotz_lib.shutdown.restype = None
-    frotz_lib.step.argtypes = [c_char_p]
-    frotz_lib.step.restype = c_char_p
+    frotz_lib.jericho_step.argtypes = [c_char_p]
+    frotz_lib.jericho_step.restype = c_char_p
     frotz_lib.save.argtypes = [c_char_p]
     frotz_lib.save.restype = int
     frotz_lib.restore.argtypes = [c_char_p]
@@ -498,7 +498,7 @@ def step(self, action):
         :rtype: string, float, boolean, dictionary
         '''
         old_score = self.frotz_lib.get_score()
-        next_state = self.frotz_lib.step((action+'\n').encode('utf-8')).decode('cp1252')
+        next_state = self.frotz_lib.jericho_step((action+'\n').encode('utf-8')).decode('cp1252')
         score = self.frotz_lib.get_score()
         reward = score - old_score
         return next_state, reward, (self.game_over() or self.victory()),\
@@ -765,7 +765,7 @@ def get_world_state_hash(self):
         '79c750fff4368efef349b02ff50ffc23'
 
         """
-        md5_hash = np.zeros(64, dtype=np.uint8)
+        md5_hash = np.zeros(32, dtype=np.uint8)
         self.frotz_lib.get_world_state_hash(as_ctypes(md5_hash))
         md5_hash = (b"").join(md5_hash.view(c_char)).decode('cp1252')
         return md5_hash
@@ -1034,20 +1034,18 @@ def _filter_candidate_actions(self, candidate_actions, use_ctypes=False, use_par
             candidate_str = (";".join(candidate_actions)).encode('utf-8')
             valid_str = (' '*(len(candidate_str)+1)).encode('utf-8')
 
-            DIFF_SIZE = 128
-            diff_array = np.zeros(len(candidate_actions) * DIFF_SIZE, dtype=np.uint16)
+            hashes = np.zeros(len(candidate_actions), dtype='S32')
             valid_cnt = self.frotz_lib.filter_candidate_actions(
                 candidate_str,
                 valid_str,
-                as_ctypes(diff_array)
+                as_ctypes(hashes.view(np.uint8))
             )
             if self._emulator_halted():
                 self.reset()
 
             valid_acts = valid_str.decode('cp1252').strip().split(';')[:-1]
             for i in range(valid_cnt):
-                diff = tuple(diff_array[i*DIFF_SIZE:(i+1)*DIFF_SIZE])
-                diff2acts[diff].append(valid_acts[i])
+                diff2acts[hashes[i]].append(valid_acts[i])
 
         else:
             orig_score = self.get_score()
diff --git a/tests/bench_valid_actions.py b/tests/bench_valid_actions.py
