I pulled down your -Z changes, built fizmo+remglk, and ran zcode tests successfully. Very nice.
I contacted Christoph Ender, the author of fizmo, and provided some feedback on building fizmo+remglk.
In response, he decided to streamline the process and has released a new beta version of fizmo with explicit remglk support.
So, thanks to Christoph, it’s now easier to set up automated zcode testing with fizmo.
Here are some brief instructions, assuming that remglk is already installed:
git clone https://github.com/chrender/fizmo-dist.git
cd fizmo-dist
git submodule init
git submodule update
autoreconf -fi
./configure --enable-remglk --with-remglk-includedir=/full/path/to/remglk/headers --with-remglk-libdir=/full/path/to/remglk/lib
(If configure gives you an error msg about sdl2 not being installed, install it or rerun the configure command with --disable-sdl added.)
make
(The compiled fizmo+remglk will be in fizmo-remglk/src/fizmo-remglk/fizmo-remglk. For convenience you can:)
ln -s fizmo-remglk/src/fizmo-remglk/fizmo-remglk fizmorIt looks like Zarf fixed two-thirds of the manifested problem. The remaining third appears to have something to do with fallout from inform7.com/mantis/view.php?id=1885). I had a nasty time trying to figure out just what Roger Firth was doing and thinking at that point in the old 6.40 repo. For the time being, I’ve rolled back that portion of parserm.h to before when I tried to consolidate all that code into InformLibrary.actor_act(). Evidently I didn’t completely understand it and so this bug was compounded.
I’m in some serious need of help. The ex23.inf bug is fixed, but in the process of fixing it, github.com/DavidGriffith/inform6lib/issues/30 showed up again. Could I please get a few more sets of eyes looking at this problem?
I grabbed the latest inform6lib from git. This is what I’m seeing with the test program for issue 30:
If I add an objectloop to Initialise that moves all rocks to the girl, then I get this, which mirrors the first output:
So:
- A non-action (taking a rock she already has, or dropping a rock that’s already on the ground) applied to a single rock produces good output.
- A non-action applied to a group of rocks produces “There is no reply.” (seems wrong).
- A significant action (taking a rock she doesn’t have, or dropping a rock she does have) applied to a single rock produces good output, and the model world changes accordingly.
- A significant action applied to a group of rocks produces a bunch of operations on nothing, and the model world does not change.
Somehow your test inspired me to look again and I found something: I managed to leave out a tidbit from the semi-rollback and I now realize that testing or not testing inp1 determines which bug manifests. It’s hopefully down to a simple test now!
This fixes #30 and breaks #34
if (actor ~= player && inp1) {This fixes #34 and breaks #30
if (actor ~= player) {The problem is not yet solved, but I think I can go to sleep now without having nightmares of code flying around in my head.
I’m tried this for the if, and it seems to fix both #30 and #34. It’s now in the latest git repo. Please beat on it and see if I managed to break something else in the process.
if ((actor ~= player && inp1 ~= nothing) ||
(actor ~= player && (inp1 == nothing && metaclass(inputobjs-->1) == nothing)) ) {
“girl, jump” is behaving strangely (expect her to wave once). Also, “girl, drop rocks” → “There is no reply.” (instead of “They’re already here.”).
Oh geez… I seem to have completely forgotten to test GIRL, DROP ROCKS. This hasn’t worked at all in the time I’ve had #30 to deal with. The problems with GIRL, JUMP seem to be quelled, strangely, by this:
grammar [;
if (verb_word == 'jump') {
action = ##WaveHands;
noun = Stone;
! second = 0;
rtrue;
}
! Didn't understand any of this, bailing out.
],
Notice that I had noun set to Stone.
At the critical if statement:
GIRL, DROP ROCKS (when rocks are indeed held)
inp1 == nothing
inp2 == nothing
inputobjs–>1 == Class
inputobjs–>2 == nothing
inputobjs–>3 == nothing
noun == nothing
actor == girl
GIRL, DROP ROCKS (when no rocks are held)
inp1 == Class
inp2 == girl
inputobjs–>1 == Object
inputobjs–>2 == Class
inputobjs–>3 == girl
noun == <routine 8235>
actor == girl
girl, jump
inp1: nothing
inp2: nothing
inputobjs–>1: Object
inputobjs–>2: nothing
inputobjs–>3: nothing
noun: nothing
actor: girl
At this point, the if rule appears to be incorrectly passed. I’m not sure what the correct path is.
I think that the if statement can be simplified. actor ~= player can be factored out, and, if inp1 ~= nothing, the first clause of the || will short circuit, so we know that inp1 == nothing if we reach the second clause.
if (actor ~= player && (inp1 ~= nothing || metaclass(inputobjs-->1) == nothing)) {Which of the 3 examples that you gave should pass the if? It seems to me that the first and the third won’t, because inp1 is nothing and metaclass of inputobjs–>1 is not nothing, while the second will, because inp1 is not nothing.
The first instance won’t pass the if. This is correct.
The second instance does pass the if, but I’m not sure if that’s correct.
The third instance won’t pass the if. Again, I’m not sure if that’s correct.
I’ve added some debugging print statements. Please see if you divine anything from that output.
I’ve taken a closer look at parserm.h and InformParser::play, and I’m now a little bit confused.
From what I can tell, inputobj–>0 … inputobj–>3 correspond to result–>0 … result–>3 in the parser proper (Parser__parse).
The meaning of these values appears to be:
result–>0 = inputobj–>0 = the action
result–>1 = inputobj–>1 = number of parameters (0, 1 or 2 – intransitive verb, direct obj only, direct and indirect objs)
result–>2 = inputobj–>2 = inp1 = the direct object. if this value is >1, it’s an object and placed in noun. Otherwise, it’s just 1, representing a special value (usually a number), and we get the value of noun from special_number1.
result–>3 = inputobj–>3 = inp2 = the indirect object. if this value is > 1, it’s placed in second. Otherwise, we get the value of second from special_number1 or special_number2 (if special_number1 was used for inp1).
There is one slight variation to this in section H of the parser (“cheaply parse unrecognized conversation”) where an actor has been given a command that’s not understood (girl, blah). In that case, result–>0 is ##NotUnderstood, result–>1 is 2 (meaning 2 params), result–>2 = 1 (with the actual content going in special_number1), and result–>3 = the actor (this is the unusual part). Note that this is consistent with what we’re seeing in your second example above for GIRL, DROP ROCKS (when no rocks are held). It’s obfuscated a bit because the (name) print rule turns 1 into “Class” and 2 into “Object.”
So, given all of that, I don’t understand the purpose of the test for metaclass(inputobjs–>1) == nothing, since inputobj–>1 doesn’t contain an object, but rather an integer count of how many objects the verb has. That would be why its value is always printed as Class or Object, since 1 is the object number of the object representing class Class and 2 is the object number of the object representing class Object.
I think the test for metaclass(inputobjs–>1) was bourne of my frustrated thrashing about.
I’m back to this:
! This if fixes #30 (GIRL, TAKE ROCKS), but breaks #34 (DAN, X CONSCIENCE)
! if (actor ~= player && inp1) {
! This if fixes #34 (DAN, X CONSCIENCE), but breaks #30 (GIRL, TAKE ROCKS)
if (actor ~= player) {
On the face of it, it seems like actor ~= player is the way to go, unless we want to exclude actions without objects (like wave, jump, inventory) from being passed to orders routines.
However, I notice that there’s a separate method called actor_act() that has very similar code in it: a test for (p ~= player && inp1) and then passing orders first to the player’s orders routine, and then the actor’s orders routine, and so on.
Is it possible the some of the problems are coming from changing the test in one place and not the other, or, more generally, from having duplicated code like that in two places? Maybe there should be a single “dispatch orders” routine (which it seems like actor_act is supposed to be) that is called from both places?
(Also, the first time (ft) stuff in actor_act seems sketchy. Looking at issue 23, I see that there’s been some churn around orders and actor_act.)
Edit:
Reading on, I see that we can have parameter count (inputobjs–>1) > 0 with the corresponding inp1 and inp2 values == 0 (nothing), meaning that the 0 parameters refer to multiple objects.
This explains the earlier GIRL, DROP ROCKS (when held) debug output that looked weird to me before.
I still don’t get why GIRL, JUMP has 2 parameters with inp1 = 0 and inp2 = 0, though it explains why she’s waving energetically four times: it’s being considered as a multiple action; multiflag is set and we call actor_act four times (debug msgs of BAZ, not ZOOM, and AAAA x4).
Edit 2:
I’m starting to think that the correct test is:
if (actor ~= player && ((i == 0) || (i == 1 && inp1 ~= 0) || (i ==2 && inp1 ~=0 && inp2 ~= 0)))
In other words, if the actor is not the player and this action does not have multiple objects, go ahead and process the orders. If it does, they need to be expanded individually, which is what happens later in the play method when actor_act is called by the multiple action code.
BUT since this same test (minus the actor ~= player part) happens just below, it would make more sense to get rid of this whole “if test and orders” chunk and then do something like:
! --------------------------------------------------------------
! Generate the action...
if ((i == 0) ||
(i == 1 && inp1 ~= 0) ||
(i == 2 && inp1 ~= 0 && inp2 ~= 0)) {
if (actor ~= player) {
switch (self.actor_act(actor, action, noun, second)) {
ACTOR_ACT_ABORT_NOTUNDERSTOOD: jump begin__action;
default: jump turn__end;
}
}
! only reach here if actor == player
self.begin_action(action, noun, second, 0);
jump turn__end;
}
! ...unless a multiple object must be substituted. First:This still doesn’t address the issue with GIRL, JUMP where it seems to be coming back from the parser as a multiple action when it’s not.
Edit 3:
Testing this, it doesn’t quite work.
GIRL, TAKE ROCKS and subsequent GIRL, DROP ROCKS work, but we have issue with GIRL, WAVE and DAN, EXAMINE CONSCIENCE.
Instead of the custom actions that are defined in the NPC’s orders routine, we get default descriptions of NPCs performing those actions. So, instead of the girl waving energetically, we get “The girl waves, feeling foolish.”, and instead of Dan saying “That I can do. I’m empty handed”, we get “Dyslexic Dan is carrying nothing.”
If I replace the call to actor_act with the duplicate stuff that was in the earlier if test, those problems go away.
My conclusion is that actor_act is slightly broken and should be fixed, because I think that the structure in the code block above (with a call to actor_act instead of funky tests and duplicated code) is cleaner.
So, what’s wrong with actor_act? Well, the test for inp1 ~= nothing is pretty suspect, since the actions we’re having trouble with are GIRL, WAVE and DAN, INVENTORY (which is what EXAMINE CONSCIENCE maps to).
The actor_act() routine is from Roger Firth’s code that caused Issue #23. The fix for that one meant that I needed to pull out the code that executed before “! Generate the action” and do something slightly different. I’ll reunify actor_act() and see what I come up with.
So, yeah, changing the test in actor_act from (p ~= player && inp1 ~= nothing) to simply (p ~= player) resolved the issues with GIRL, WAVE and DAN, X CONSCIENCE.
GIRL, JUMP (in the context of the test program with a grammar routine) is still broken. Think I need to go nose around the parser for that one.
And… I think I have it.
The test program has:
grammar [;
if (verb_word == 'jump') {
action = ##WaveHands;
noun = 0;
second = 0;
rtrue;
} The parser does this:
i = RunRoutines(actor, grammar);
...
if (i == 1) {
results-->0 = action;
results-->1 = 2; ! Number of parameters
results-->2 = noun;
results-->3 = second;
rtrue;
}Look how the parser sets results–>1 to 2 unconditionally, even when noun or second might be 0. And hence, we have a bogus multiple action.
I think we want something more like:
if (i == 1) {
results-->0 = action;
results-->1 = 0; ! Number of parameters
results-->2 = noun;
if (noun ~= nothing) {
(results-->1)++;
}
results-->3 = second;
if (second ~= nothing) {
(results-->1)++;
}
rtrue;
}A brief test with this change shows that it fixes GIRL, JUMP and doesn’t break any of the other (few) things that we’ve been testing with the girl and Dan examples.
Patch attached with the cumulative changes that I’ve made.
Couple of other thoughts (not addressed by the patch):
-Can legit multiple actions come through an NPC’s grammar routine? Probably, when grammar returns ‘verb’ or -‘verb’, but I don’t see it making much sense when grammar returns true.
-We might want to refuse to increment results–>1 beyond 0 if noun == 0 and second ~= 0. I don’t know of a scenario where the parser gets into this state, but a broken grammar routine could return this.
patch.txt (2.54 KB)
Which commit should this patch be applied to?
commit 187754db068dfd49ba161095937b647bdae09f03
At least one nit remains: GIRL, DROP ROCKS returns “There is no reply.”