[I6] Inform 6.33.1-b3 issues

[DavidG (David Griffith)]

@vlaviano

@DavidG

I think I’ve fixed this. The problem stemmed from improper usage of return value from ZRegion(). In the old inform-2006 repository, there was an effort to get rid of calls to ZRegion(), replacing them with metaclass(). Two calls were left in. One was used properly, but the other treated the return value as it if was from metaclass(). I have changed both calls to ZRegion() to their modern equivalents.

Awesome.

While you’re paying attention to the thread, I noticed another small issue in DropSub in verblibm.h:

    if (noun notin actor && ~~ImplicitTake(noun)) return L__M(##Drop, 2, noun);

Looking at ImplicitTake, it returns true on failure and false on success, so the test above ought to be ImplictTake(noun) rather than ~~ImplicitTake(noun), as it is in all of the other calls in verblibm.h.

I think that this issue has gone undetected in the past because the grammar for Drop uses the multiheld and multiexcept tokens which cause the parser to generate its own ImplicitTake prior to DropSub ever being called. I discovered the issue after having extended the grammar for Drop and having adapted DropSub in one of my own programs to handle a special case (of liquid in a container).

Hmm… Can you come up with a test case?

[vlaviano]

@DavidG

Hmm… Can you come up with a test case?

I investigated this further with the following test case:

Constant Story "DROPSUB IMPLICIT TAKE";
Constant Headline "^An Interactive Investigation^";

Include "Parser";
Include "VerbLib";

Object Start_Room "Start Room"
  with description "This is the starting room.",
  has light;

Object tin "metal tin"
  with name 'metal' 'tin',
  has container openable;

Object -> mint "breath mint"
  with name 'breath' 'mint',
  has edible;

[ Initialise;
  location = Start_Room;
  move tin to player;
];

Include "Grammar";

This has the following behavior:

DROPSUB IMPLICIT TAKE
An Interactive Investigation
Release 1 / Serial number 160202 / Inform v6.33 Library 6/12-beta1 SD

Start Room
This is the starting room.

i
You’re carrying:
a metal tin (which is closed)

open tin
You open the metal tin, revealing a breath mint.

drop mint
Perhaps you should take the breath mint out of the metal tin first.

eat mint
(first taking the breath mint)
You eat the breath mint. Not bad.

I had expected drop to have similar behavior as eat, implicitly taking the mint out of the tin.

However, reading ImplicitTake more carefully, I see that the first few lines are:

[ ImplicitTake obj
    res ks supcon;
    switch (metaclass(obj)) { Class, String, Routine, nothing: rfalse; }
    if (obj in actor) rfalse;
    if (action_to_be == ##Drop) rfalse;

It seems that, in the case of ##Drop, ImplicitTake will always return false before changing the model world in any way.

I overlooked this because, in the program that I referred to in my previous post, I had created a new action with its code based on the existing DropSub impl, so action_to_be wasn’t ##Drop. In the case of regular ##Drop, the distinction that I made between multiheld/multiexcept and other tokens is irrelevant since the parser’s invocation of ImplicitTake always matches this action_to_be test and returns false, so DropSub’s ImplicitTake line is called either way.

In light of this, rather than the ~~ImplicitTake() check in DropSub being reversed, it seems that it’s instead superfluous and that the same effect could be achieved with:

if (noun notin actor) return L__M(##Drop, 2, noun);

Evidently, the library explicitly does not want ##Drop ever to do an implicit take, although it’s unclear to me why this should be the case given how ##Eat works. I think that maybe it’s to prevent the parser’s multiheld/multiexcept ImplicitTake, when the user types “drop ”, from producing “(first taking the foo) Dropped.” ImplicitTake could deal with that by extending its acton_to_be check to:

if (action_to_be == ##Drop && obj in parent(actor)) rfalse;

This should allow the “drop ” case to produce “the foo is already here”, while allowing items that are indirectly contained in the actor to be implicitly taken and dropped.

All that being said, I’m not sure to what extent existing games rely on the old behavior or if there’s a mandate to improve I6 library behavior (assuming that we can agree on what constitutes an improvement) vs. just fixing bugs.

[DavidG (David Griffith)]

@vlaviano

I’ve continued working through DM4, and I’ve encountered an issue related to the exercises in §27 (Listing and grouping objects).

I have a group of Scrabble letters with a list_together routine (DM exercise 67) and a group of identical featureless cubes. The library’s item listing code produces a number of errors when these two groups of items coexist either in the same room or in the player’s inventory. If they are separated (one group in the room and the other group in the player’s inventory) or if only one group is implemented (comment out either of the chunks of code between dashed lines in the test program below), item listing behaves as expected.

Adding a regular item (e.g., Object → rock “rock” with name ‘rock’ doesn’t appear to have an impact either way. When the letters and cubes are together, the issue manifests.

This is now fixed. I’ve also fixed the extra newline for PLACES output.

[DavidG (David Griffith)]

@vlaviano

Orders fully parsed by an animate/talkable object’s grammar routine are not being delivered to its orders routine.

This is a simplified version of exercise 30 in the DM4. See page 140 for an explanation of what the return values from grammar are supposed to indicate and a description of the exercise, and pages 449-450 for the official solution.

Constant Story "GRAMMAR AND ORDERS";
Constant Headline "^An Interactive Bug Reproduction^";

Include "Parser";
Include "VerbLib";

Object Start_Room "Somewhere"
  with description "You're not sure where you are.",
  has light;

Object -> girl "girl"
  with name 'girl',
       orders [;
           WaveHands: "The girl waves energetically.";
       ],
       grammar [;
           if (verb_word == 'xyzzy') {
               action = ##WaveHands;
               noun = 0;
               second = 0;
               rtrue;
           }
       ],
  has animate female;

[ Initialise;
  location = Start_Room;
];

Include "Grammar";

Expected: “girl, xyzzy” produces the “The girl waves energetically.” message.

Actual: “girl, xyzzy” produces “Nothing to do!” from the parser. I took a look at the library code and see that both L__M(##Miscellany, 2) and L__M(##Miscellany, 43) are defined with this text.

I tracked this one down to a change in parserm.h made in github.com/DavidGriffith/inform … 47b314f0ca (from the old 2006 CVS). In the section prefaced by “! Generate the action”, before the change, the code that now appears in actor_act() appeared before the various tests of i, inp1, and inp2. After the change, this call comes only after tests on these variables succeeds. It’s unclear to me why this change was made, but it clearly is preventing correct behavior.

Simply moving the actor_act() call above the tests works, but not all the way. “Nothing to do!” is still printed afterwards by a call of L__M(##Miscellany, 2); because multiple_object–>0 equals 0. Clearly that printing of ##Miscellany, 2 should not happen if the result from the grammar routine was true. I don’t know how to do this correctly.

I PMed Zarf about this hoping that he might remember what was what, then I decided I’d better open it up for everybody to take a look.

[vlaviano]

I’ve been looking at some differences between zcode and glulx in the library, and I’ve noticed a small issue.

Banner() in verblibm.h prints out a newline before printing Story for zcode games but not for glulx games:

    if (Story) {
        #Ifdef TARGET_ZCODE;
        #IfV5; style bold; #Endif;
        print "^", (string) Story;
        #IfV5; style roman; #Endif;
        #Ifnot; ! TARGET_GLULX;
        glk($0086, 3); ! set header style
        print (string) Story;
        glk($0086, 0); ! set normal style
        #Endif; ! TARGET_
    }

The result is that a game with introductory text of the form:

[ Initialise;
  location = Start_Room;
  "Some introductory text.";
];

will look good when the game is compiled to zcode, but the intro text will run up against the story name when the game is compiled to glulx.

[DavidG (David Griffith)]

@vlaviano

I’ve been looking at some differences between zcode and glulx in the library, and I’ve noticed a small issue.

Banner() in verblibm.h prints out a newline before printing Story for zcode games but not for glulx games:

    if (Story) {
        #Ifdef TARGET_ZCODE;
        #IfV5; style bold; #Endif;
        print "^", (string) Story;
        #IfV5; style roman; #Endif;
        #Ifnot; ! TARGET_GLULX;
        glk($0086, 3); ! set header style
        print (string) Story;
        glk($0086, 0); ! set normal style
        #Endif; ! TARGET_
    }

The result is that a game with introductory text of the form:

[ Initialise;
  location = Start_Room;
  "Some introductory text.";
];

will look good when the game is compiled to zcode, but the intro text will run up against the story name when the game is compiled to glulx.

Fixed.

I noticed that Glulxe will put a newline just before the introductory text whereas most Z-machines don’t. Does anyone know if this newline is generated by the interpreter? I can’t find it in the Library. This effect is not visible with Gargoyle because the text always rises from the bottom instead of descending from the top.

[zarf (Andrew Plotkin)]

There’s nothing in the spec about it, and the interpreter isn’t generating any extra newline.

ADD: It looks like Zoom formats the text pane with margin space (between the top of the pane and the first line of text). Margins are up to the interpreter.

[DavidG (David Griffith)]

For the “Orders fully parsed by an animate/talkable object’s grammar routine are not being delivered to its orders routine.” problem (inform7.com/mantis/view.php?id=1813), I’ve committed a partial solution.

Prior to Roger Firth’s movement of code from before “! Generate the action” to InformLibrary.actor_act(), that code would ALWAYS result in a jump to begin__action or to turn__end if (actor ~= player). After the change, this was not always the case. It seems that this change was made to accomodate the new “<action [noun] [second], actor>” optional grammar for invoking R_Process (introduced in Inform compiler 6.33). For the moment, I’m rolling back this chunk to prior to Roger’s change to have an anchor point from which to investigate a more perfect solution.

Thanks goes to Zarf for pointing out to me the precise place and manner of the error in parserm.h that caused the problem.

[vlaviano]

@DavidG

For the moment, I’m rolling back this chunk to prior to Roger’s change to have an anchor point from which to investigate a more perfect solution.

Thanks goes to Zarf for pointing out to me the precise place and manner of the error in parserm.h that caused the problem.

That makes sense to me. Thanks to Zarf seconded, and thanks to DavidG for investing the time to investigate and fix all of these issues.

[DavidG (David Griffith)]

I found a new problem that was caused by the fix for L61115. That was “‘multiheld’ can match unholdable objects”. Remember that one? See github.com/DavidGriffith/inform6lib/issues/30. At first I thought it was part of the hangup between grammar and orders that hinges on Roger’s actor_act() code.

[vlaviano]

@DavidG

I found a new problem that was caused by the fix for L61115. That was “‘multiheld’ can match unholdable objects”. Remember that one? See github.com/DavidGriffith/inform6lib/issues/30. At first I thought it was part of the hangup between grammar and orders that hinges on Roger’s actor_act() code.

Hmm, it looks like the fix for L61115 is also responsible for the “implicit take doesn’t happen for ##Drop when the player encloses but doesn’t directly carry an object” inconsistency that I was going on about earlier in the thread.

[DavidG (David Griffith)]

The actor_act() hangup and the problem with an NPC taking or dropping multiple objects have been fixed. I’m still working on optimizing the solution to the former. The latter turned out to not really be connected to L61115. I’ll go ahead and write up the implicit take problem.

github.com/DavidGriffith/inform6lib/issues/31

inform7.com/mantis/view.php?id=1886

[DavidG (David Griffith)]

Investigating the problem with “implicit take doesn’t happen for ##Drop when the player encloses but doesn’t directly carry an object”. It looks like neither 6/11 nor 6/10 would perform the implicit take. This suggests to me that there was some sort of disagreement in 2006 over which is the proper way to do it. Look at this section of code in verblibm.h:

[ ImplicitTake obj
    res ks supcon;
    switch (metaclass(obj)) { Class, String, Routine, nothing: rfalse; }
    if (obj in actor) rfalse;
    if (action_to_be == ##Drop) rfalse;
    ...

If we comment out this line

if (action_to_be == ##Drop) rfalse;

Then we get this:

>open tin
You open the metal tin, revealing a breath mint.

>drop mint
(first taking the breath mint out of the metal tin)
Dropped.

Is this what you were expecting to happen?

[vlaviano]

@DavidG

Is this what you were expecting to happen?

Yes, I was expecting DROP MINT to behave the same way as EAT MINT, either with both producing an implicit take or neither.

I think that we might need something like:

if (action_to_be == ##Drop && ~~IndirectlyContains(player, obj)) rfalse; rather than removing the line completely, to prevent results like:

DROP MINT
(first taking the mint)
Dropped.

when the player isn’t carrying the mint.

[DavidG (David Griffith)]

@vlaviano

@DavidG

Is this what you were expecting to happen?

Yes, I was expecting DROP MINT to behave the same way as EAT MINT, either with both producing an implicit take or neither.

I think that we might need something like:

if (action_to_be == ##Drop && ~~IndirectlyContains(player, obj)) rfalse; rather than removing the line completely, to prevent results like:

DROP MINT
(first taking the mint)
Dropped.

when the player isn’t carrying the mint.

I’ll go for that. Now this brings up another potential irritation. Consider this:

OPEN TIN
You open the metal tin, revealing a breath mint.

DROP TIN
Dropped.

DROP MINT
Perhaps you should take the breath mint out of the metal tin first.

That doesn’t sound right. It should respond with “The mint is already here.”.

[vlaviano]

@DavidG

That doesn’t sound right. It should respond with “The mint is already here.”.

This is because of the following in DropSub:

    if (noun in parent(actor)) return L__M(##Drop, 1, noun);
    if (noun notin actor && ~~ImplicitTake(noun)) return L__M(##Drop, 2, noun);

where msg 1 is “already here” and msg 2 is “take it out of the foo first”.

But this is not a new problem triggered by the proposed change; it’s an existing issue with the test “noun in parent(actor)”, which is too weak for its intended purpose.

Consider the following modified version of our test program:

Constant Story "DROPSUB IMPLICIT TAKE";
Constant Headline "^An Interactive Investigation^";

Include "Parser";
Include "VerbLib";

Object Start_Room "Start Room"
  with description "This is the starting room.",
  has light;

Object -> table "big table"
  with name 'big' 'table',
  has enterable supporter;

Object tin "metal tin"
  with name 'metal' 'tin',
  has container openable;

Object -> mint "breath mint"
  with name 'breath' 'mint',
  has edible;

[ Initialise;
  location = Start_Room;
  move tin to player;
];

Include "Grammar";

When we run this with the unmodified library, we see:

DROPSUB IMPLICIT TAKE
An Interactive Investigation
Release 1 / Serial number 160320 / Inform v6.33 Library 6/12-beta1 SD

Start Room
This is the starting room.

You can see a big table here.

open tin
You open the metal tin, revealing a breath mint.

drop tin
Dropped.

get on table
You get onto the big table.

drop mint
Perhaps you should take the breath mint out of the metal tin first.

drop tin
Perhaps you should take the metal tin out of the Start Room first.

So, I think that the issue that you pointed out needs to be fixed regardless. I’d suggest treating it as a separate issue: dropping an object that is not carried by the actor and not directly contained in the parent of the actor produces an inappropriate response. I think that the correct response is arguable depending on how we define “here”. Does it mean the immediate parent of the actor, or does it mean in the broader location / in scope?

(In my previous post, s/player/actor in ~~IndirectlyContains(player, obj))

[vlaviano]

More thoughts on this:

Both of these issues are intertwined.

As far as I can tell, there are 3 things that we need to consider:

1. The ##Drop grammar’s multiheld token and the ImplicitTake call that this causes the parser to generate
2. DropSub and its own call to ImplicitTake
3. The body of ImplicitTake

Note that the special case “if (action_to_be == ##Drop)” test in ImplicitTake returns false, while the parser’s call to ImplicitTake treats a true return value as failure.

I think that this effectively makes the parser’s ImplicitTake a noop for ##Drop and causes us to move on to DropSub, where ##Drop processing (including the ImplicitTake) really happens. We need to keep this in mind when modifying (or removing) that test in ImplicitTake, lest we restore the significance of the parser’s ImplicitTake without realizing it.

Not that I’m saying that the current situation is a good thing. It would be nicer if ##Drop were handled more consistently with other actions that require held objects.

[DavidG (David Griffith)]

As I think over this latest issue, I think after it is fixed, it’ll be time for a point release of the 6/12 Library. I haven’t quite decided how to display the point release. I’m thinking having the Library itself call itself 6/12.1 and the package identify itself as 6.12.1.

[vlaviano]

@DavidG

As I think over this latest issue

I was browsing through the I7 manual the other day, and I noticed that the “can’t drop something that’s not directly carried” limitation is mentioned as part of example 224 (Celadon). It doesn’t directly bear on the solution for I6, but it shows us that (1) the current behavior is known, not purely accidental, and (2) the potential desire for different behavior is also understood.