[WIP] FIX: Incomplete objects tree for some games #54

MarcCote · 2021-11-10T21:55:27Z

This PR improves support for the games in Jericho.

Here are the main modifications/improvements of this PR.

Updating the objects count that was off from some games. Having the wrong number of objects leads to an incomplete object tree which is used to represent the game state, hence to track world_changed. Furthermore, this affects valid actions detection which uses the game state to know whether an action had any effect in the world.
Instead of relying on world_diff, world_changed now relies on
- the difference between the objects tree before and after the execution of a command
- special addresses in the RAM
- a different Returning Program Counter (RetPC)
- victory() > 0
- game_over() > 0.
Re-validating all the games to make sure that relevant commands from the walkthrough lead to a world_changed.
(in progress) Re-validating all the games to make sure that "noop" commands (e.g., look, inventory, examine OBJ) do not trigger world_changed.

Bonus additions:

As a bonus, some of the walkthroughs were improved (i.e., achieved a higher score).
curses.z5 max score is actually 554 instead of 550 (due to a bug in the game, see comment curses.c)
murdac.z5 will report 250/250 upon victory (even though the RAM says 249).

Here's the current progress of re-validating the games:

python tools/test_games.py ./roms/* --check

Game	Status	Noop check
905.z5	OK	Done
acorncourt.z5	OK	Done
advent.z5	OK	Done
adventureland.z5	OK	Done
afflicted.z8	OK	Done
anchor.z8	OK Done but score 99/100	Done
awaken.z5	OK	Done
balances.z5	FAIL Done but score 50/51	Done
ballyhoo.z3	OK	Done
curses.z5	OK	Done
cutthroat.z3	OK	Done
deephome.z5	OK	Done
detective.z5	OK	Done
dragon.z5	OK	Done
enchanter.z3	OK	Done
enter.z5	OK	Done
gold.z5	OK	Done
hhgg.z3	OK	Done
hollywood.z3	OK	Done
huntdark.z5	OK	Done
infidel.z3	OK	Done
inhumane.z5	OK	Done
jewel.z5	OK	Done
karn.z5	OK	Done
library.z5	OK	Done
loose.z5	OK	Done
lostpig.z8	OK	Done
ludicorp.z5	OK	Done
lurking.z3	OK	Done
moonlit.z5	OK	Done
murdac.z5	OK	Done
night.z5	OK	Done
omniquest.z5	OK	Done
partyfoul.z8	OK	Done
pentari.z5	OK
planetfall.z3	OK
plundered.z3	OK
reverb.z5	OK
seastalker.z3	OK
sherlock.z5	OK
snacktime.z8	OK
sorcerer.z3	OK
spellbrkr.z3	OK
spirit.z5	OK
temple.z5	OK
theatre.z5	OK
trinity.z4	OK
tryst205.z5	OK
weapon.z5	OK
wishbringer.z3	OK
yomomma.z8	OK
zenon.z5	OK
zork1.z5	OK
zork2.z5	OK
zork3.z5	OK
ztuu.z5	OK

MarcCote · 2021-11-10T22:06:06Z

@mhauskn this PR is really messed at the moment. I'm going to clean it up, but I wanted you to have an update on the fix. I now believe after this PR, we might want to do a major release (Jericho 4.0.0).

Things to look at, changes in the frotz_interface.c (assume all commented code will be deleted), and all the games c files.

Also, I would like you to try out the script that you used to validate world changed when playing the walkthroughs.

mhauskn · 2021-11-11T21:37:54Z

I'm getting the following error trying to install:

src/interface/frotz_interface.c:27:10: fatal error: md5.h: No such file or directory
 #include "md5.h"
          ^~~~~~~
compilation terminated.

I think we used to use the md5.c file to provide standalone md5 hashing, without the need for other dependencies. Is the PR intending to include a md5.h file?

MarcCote · 2021-11-12T00:56:48Z

My bad. I pushed the missing file. That will enable computing the md5 hash for a game state directly in C instead of having to get the state from Python, then call the hashing function from hashlib (i..e, less data moving around).

mhauskn · 2021-11-13T01:21:57Z

Hey Marc - I've done some small fixes around the get_cleaned_world_diff() - see them here: d59a6c3.

The motivation for these changes is to have a consistent world_diff returned by jericho._get_world_diff() and jericho._filter_candidate_actions(). This change makes both return a 256-length tuple that is comparable.

However, I'm detecting some issues with Zork1's walkthrough:

Step 162. NoWorldChange gold_act: Press yellow button, obs: Click.
Step 165. NoWorldChange gold_act: Turn bolt with wrench, obs: The sluice gates open and water pours through the dam.

Both of these seem to be issues with world_state not changing when we expect it should.

Here is the script I'm running to detect these issues: https://gist.github.com/mhauskn/861e4983f54a435013f66e9ab44ea308#file-test_walkthrough-py

MarcCote · 2021-11-15T14:45:24Z

Oh, I should have mentioned I have yet to test the valid_action code. I was focusing on making sure the commands from the walkthroughs appropriately trigger world_changed.

Also, I was thinking of removing the whole world_diff, since now I keep a copy of the previous objects tree that is used to compare against the current objects tree. The valid_action code still needs to be changed to use that.

For Zork1, I have yet to validate it. I'm currently doing planetfall.z3 (see list above). If you want to help, you can start from the bottom and work your way up. Here's my workflow:

Start with test_games.py game.z5 --check --no-precheck (as in my original post above).
If for some commands the world_changed is not triggered when it should have been, run the tools/find_special_ram game.z5 --index no_command which will detect all ram locations that have been changed by that command as well as listing all commands that have changed each detected ram location throughout the walkthrough.
Add the special address to the relevant game.c file or add an exception to SKIP_CHECK_STATE for that command in test_games.py.

MarcCote · 2021-11-17T21:35:47Z

frotz/src/games/seastalker.c

-    *n = 1;
-    return seastalker_ram_addrs;
+    *n = 5;
+    return seastalker_special_ram_addrs;


Wrong variable was returned.

MarcCote · 2021-11-17T21:36:50Z

frotz/src/games/partyfoul.c

@@ -43,7 +49,7 @@ char* partyfoul_clean_observation(char* obs) {
  if (pch != NULL) {
    *(pch-2) = '\0';
  }
-  return obs+1;
+  return obs + strspn(obs, "\n ");  // Skip leading newlines and whitespaces.


Sometimes a character was missing.

MarcCote · 2021-11-17T21:37:17Z

frotz/src/games/plundered.c

-  *n = 0;
-  return NULL;
+  *n = 1;
+  return plundered_intro;


Need to press "Enter" to start the game.

MarcCote · 2021-11-17T21:38:12Z

frotz/src/games/lostpig.c

+  // Winning messages change depending on the score.
+  // ***  Grunk bring pig back to farm ***  // 6 out of 7 points.
+  // ***  Grunk bring pig back to farm and make new friend ***  // 7 out of 7 points.
+  char *victory_text = "***  Grunk bring pig back to farm ";  // Works for both 6 and 7 points.


Different winning messages depending on how many points we got.

MarcCote · 2021-11-17T21:39:01Z

frotz/src/games/deephome.c

@@ -50,7 +51,7 @@ char* deephome_clean_observation(char* obs) {
 }

 int deephome_victory() {
-  char *death_text = "****  You have won  ****";
+  char *death_text = "*** You have won ***";


Death message had too many *

MarcCote · 2021-11-17T21:39:37Z

frotz/src/games/curses.c

-    int i;
-    char mask;
-    mask = ~(1 << 7);
-    // Clear attr 24


It says attr 24, but above the attr_idx == 25 was used.

MarcCote · 2021-11-17T21:40:06Z

frotz/src/games/curses.c

+  // return 550;
+  // Due to a bug, the max score is 554 points instead of 550.
+  // ref: https://raw.githubusercontent.com/heasm66/walkthroughs/main/curses_walkthrough.txt
+  return 554;


Changing the max score for curses.

MarcCote · 2021-11-17T21:40:51Z

frotz/src/games/anchor.c

+  //char *death_text = "****  You have won  ****";
+  char *death_text = "*** You have won... for now ***";


Incorrect winning message?

MarcCote · 2021-11-17T21:42:09Z

frotz/src/dumb/dumb_output.c

@@ -522,6 +522,7 @@ void dumb_init_output(void)
 {
    if (h_version == V3) {
 	h_config |= CONFIG_SPLITSCREEN;
+	h_config |= CONFIG_NOSTATUSLINE;


Using the option to disable status line for V3 games. This simplifies the clean_observation_... functions.

MarcCote · 2021-11-17T21:42:57Z

frotz/src/common/screen.c

@@ -1554,7 +1554,7 @@ void z_show_status (void)
    /* One V5 game (Wishbringer Solid Gold) contains this opcode by
       accident, so just return if the version number does not fit */

-    if (h_version >= V4)
+    if (h_version >= V4 || h_config & CONFIG_NOSTATUSLINE)


Skip printing the status line when the corresponding option is set. This simplifies the clean_observation_... functions.

MarcCote · 2021-11-17T21:46:07Z

@mhauskn I'm back working on it. I've done the initial validation for a few more games.

Also, I've left a couple of comments (see above) to show some bug fixes I found. Those should probably go in a separate PR to be merged in the current version of Jericho (i.e., v3).

MarcCote · 2021-11-24T15:01:30Z

frotz/src/games/sherlock.c

+    // Extract time from status line. E.g.
+    // " Vestibule   [...]   Saturday   5:01:00 a.m.      Score:    0"
+    //                       ^                            ^
+    //                       88                           117


I've added the time to the observation. I think it is crucial for Sherlock.z5

MarcCote · 2021-11-25T19:27:45Z

However, I'm detecting some issues with Zork1's walkthrough:

Step 162. NoWorldChange gold_act: Press yellow button, obs: Click.

Step 165. NoWorldChange gold_act: Turn bolt with wrench, obs: The sluice gates open and water pours through the dam.

Now detected correctly (see ab86cb5).

MarcCote · 2021-11-26T19:15:40Z

frotz/src/interface/frotz_interface.c

@@ -1756,21 +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!


@mhauskn I figured out the segfault. There's a step function in regexp.c (see https://code.woboq.org/userspace/glibc/misc/regexp.c.html#50). It was clashing with the one in Jericho, so I renamed it.

mhauskn · 2021-12-16T16:23:31Z

Hey Marc wanted to touch base on this PR. Is it still in-progress or in a finished state? If the latter - I will be happy to run some checks on it.

MarcCote · 2021-12-16T16:41:35Z

I finally have some time now to tidy it up. One thing that is still missing is to check whether no-op action (e.g., inventory, examine obj, look) doesn't change the state of the environment (e.g., thief is moving around).

mhauskn · 2021-12-20T20:11:30Z

I tested for false-positive world changes by attempting a combination of valid and invalid actions. Invalid actions included things like "z/wait/inventory/x me/navigating-into-a-dead-end" etc. Below are my findings:

curses, ballyhoo, gold, hhgg, inhumane, lostpig, omniquest, pentari, planetfall, tryst205, zork3, ztuu - all actions are triggering world changed.

huntdark - after going "left", all actions started triggering world changes (maybe there is a timer for bleeding out). Doesn't seem to happen if going "right".
moonlit.z5 - many of the failed navigation actions are triggering false-positive world changes.
weapon - going south results in false-positive world changed.

Also a handful of games are giving false-positive world changes the first time "inventory" command is issued, but not on subsequent invokations.

MarcCote requested a review from mhauskn November 10, 2021 21:55

MarcCote force-pushed the fix_obj_num branch from d77f6b9 to 7032ea4 Compare November 11, 2021 03:35

MarcCote commented Nov 17, 2021

View reviewed changes

MarcCote force-pushed the fix_obj_num branch 2 times, most recently from edbb67d to ec936df Compare November 24, 2021 13:56

MarcCote commented Nov 24, 2021

View reviewed changes

MarcCote force-pushed the fix_obj_num branch 2 times, most recently from 49cde86 to 0bd314d Compare November 25, 2021 16:32

MarcCote commented Nov 26, 2021

View reviewed changes

MarcCote mentioned this pull request Dec 16, 2021

Few questions on using Jericho > 3.0.0 #57

Open

MarcCote force-pushed the fix_obj_num branch from 3119bc2 to e44f7a7 Compare December 16, 2021 19:57

MarcCote added 26 commits January 6, 2025 15:12

Noop check: enter

0b6fbab

Noop check: gold

c20f5ea

Noop check: hhgg

b2d277f

Noop check: hollywood

5245fba

Noop check: huntdark

b6a440d

Noop check: infidel

0c87558

Sort interactable objects.

4fb314f

Noop check: inhumane

61c3af9

Noop check: jewel

a5e492f

Noop check: karn

ce3388d

Noop check: library

60b36ff

Noop check: loose

b946067

Noop check: loose.z5

68242f0

Tweak test_games.py to detect unused special ram addresses.

9aa45ef

Noop check: lostpig

869a950

Noop check: ludicorp

8baa787

Improve debugging tools

87ed928

Noop check: lurking

87eb5fc

Noop check: moonlit

bd99c9e

Halt emulator on z_quit

e4e4ce8

FIX: do not assume RAM size is even.

25fbed5

Noop check: murdac

dfbf697

Noop check: night

39b6f8d

Noop check: omniquest

93cff3f

Noop check: partyfoul + add option to skip z_read_char

9c4f9fa

Noop check: pentari

88f15eb

MarcCote force-pushed the fix_obj_num branch from 2793c18 to 88f15eb Compare January 6, 2025 20:13

MarcCote added 3 commits January 6, 2025 18:00

Fix md5 import

9348e2d

Improve find_special_ram printing

8bea90c

Improve checks for anchor.c

2634772

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] FIX: Incomplete objects tree for some games #54

[WIP] FIX: Incomplete objects tree for some games #54

MarcCote commented Nov 10, 2021 •

edited

Loading

MarcCote commented Nov 10, 2021

mhauskn commented Nov 11, 2021

MarcCote commented Nov 12, 2021

mhauskn commented Nov 13, 2021

MarcCote commented Nov 15, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote Nov 17, 2021

MarcCote commented Nov 17, 2021

MarcCote Nov 24, 2021

MarcCote commented Nov 25, 2021

MarcCote Nov 26, 2021

mhauskn commented Dec 16, 2021

MarcCote commented Dec 16, 2021

mhauskn commented Dec 20, 2021

		//char death_text = "* You have won **";
		char death_text = " You have won... for now *";

[WIP] FIX: Incomplete objects tree for some games #54

Are you sure you want to change the base?

[WIP] FIX: Incomplete objects tree for some games #54

Conversation

MarcCote commented Nov 10, 2021 • edited Loading

MarcCote commented Nov 10, 2021

mhauskn commented Nov 11, 2021

MarcCote commented Nov 12, 2021

mhauskn commented Nov 13, 2021

MarcCote commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcCote commented Nov 17, 2021

Choose a reason for hiding this comment

MarcCote commented Nov 25, 2021

Choose a reason for hiding this comment

mhauskn commented Dec 16, 2021

MarcCote commented Dec 16, 2021

mhauskn commented Dec 20, 2021

MarcCote commented Nov 10, 2021 •

edited

Loading