Pouring Liquid Into Its Current Vessel Isn't Prevented

[tajmone]

Pouring a liquid into its current contain is allowed!

• [x] Add CHECKs to prevent this.

From the old test file liquids.a3log (commit 3fb2136):

> pour wine in jar
(taking the bottle of wine first)
You pour the wine into the jar.

> x bottle
The bottle is empty.

> x jar
The jar contains some wine.

> ; And vice-versa:
> pour wine in bottle
(taking the jar of wine first)
You pour the wine into the bottle.

> x bottle
The bottle contains some wine.

> x jar
The jar is empty.

> ; edge-case test (should fail):
> pour wine in bottle
You pour the wine into the bottle.

> ; **BUG!** The wine is already in the bottle

For up-to-date tests, see the new test suite folder for the liquid class:

• tests/liquids/

[tajmone]

Problem

This seems to be a problem with the main verb definition in lib_verbs.i:

ADD TO EVERY OBJECT
  VERB empty_in, pour_in
    WHEN obj
      CHECK ...
      AND cont NOT IN obj
        ELSE SAY check_cont_not_in_obj OF my_game.

It CHECKs that the container is not inside the object, but it doesn't seem to check if the object is already in the container.

Possible Solutions

I'm not quite sure where this check should be added, i.e. on the main empty_in, pour_in definition in lib_verbs.i, or on the pour_in verb in lib_classes.i — these verbs don't behave quite the same with liquids, e.g. trying to pour the bottle into another container doesn't work, only pouring it's contents does.

Also, I'm wondering if there's a need for similar checks with other related verbs (e.g. the empty* and pour* group, both main verbs and on the various classes).

We currently don't have enough coverage in the test-suite, we should first add all appropriate tests to catch all current bugs, and to catch potential undesired side effects resulting from the fix attempts!

[AnssiR66]

Yes, seems like an extra check is needed there, to check if the liquid is not in the container already. This new check would naturally go before the check "CHECK cont NOT IN obj", I would guess. I wonder why it was not originally added. Anyway, this check already exists in the library (for example in put_in etc), so we don't have to invent a new check, just add the existing check to the "empty" and "pour" verbs. Would adding it in this way help?

[tajmone]

we don't have to invent a new check, just add the existing check to the "empty" and "pour" verbs. Would adding it in this way help?

Indeed, the put_in VERB in lib_verbs.i does contain that CHECK:

ADD TO EVERY OBJECT
  VERB put_in
    WHEN obj
      ...
      AND obj NOT IN cont
        ELSE
          IF cont ISA SUPPORTER
            THEN SAY check_cont_not_supporter OF my_game.
            ELSE
              IF obj IS NOT plural
                THEN SAY check_obj_not_in_cont_sg OF my_game.
                ELSE SAY check_obj_not_in_cont_pl OF my_game.
              END IF.
          END IF.

The problem with empty and pour is that they are jointly defined in lib_verbs.i in a single VERB block, although they have different parameters: in empty the source parameter refers to container, not its contents ("empty bag in trashcan"), whereas in pour it refers to liquid itself ("pour milk in bucket"). So we can't use a CHECK to investigate what's inside the source container of empty — as a matter of fact, it's in their DOES block that the verb checks that the source container is not empty.

If we were to split the two VERB definitions, it would add quite a lot of duplicate code, and make the maintenance of this similar verbs harder.

For the empty VERB, there's no alternative except to handle the code in VERB's body, I think, since CHECK expressions can't contain complex code references, but just simple expressions, from what I remember (their ELSE part can, but not the CHECKs). So we might just as well add an IF that handles both the contents of the container from empty and the liquid from pour inside the DOES block.

As for "pour [liquid]", that's another matter, for we could indeed add a check in the pour definition in lib_classes.i — but there isn't a pour VERB definition in that sense.

We might just as well add an extra VERB pour definition in lib_verbs.i (without the coupled empty), just to hold the additional CHECK.

I'm still not sure though, for until we actually try this out we might be missing out some complications due the shared VERB definition. Probably we should try all these different approaches, and see which combination works best.

But, if possible, I'd like to keep the solution simple and intuitive, so it won't be confusing for end users who might wish to extend or tweak the library (these VERBs already have rather lengthy CHECKs, which are hard to keep up with).

[tajmone]

Most likely, we should only focus on pour [liquid] in here, by adding a custom VERB pour_in VERB om liquid, in lib_classes.i.

After all, the original problem doesn't really affect the empty VERB, since the verb usage wouldn't even allow referring to the content (unlike "pour beer in jar", when the beer is already there). Furthermore, the empty verb (actually both, since they're jointly defined) does verify (in DOES) that the source container is not empty — which would produce a reasonable response when the player mistakenly tries to empty the same container twice in a row.

So we probably don't need to tweak the main definitions in lib_verbs.i, but only add definitions for those verbs that involve the liquid in their parameter, to add a check that it's not already in the destination container (verbs pour* only).

[tajmone]

OK, this is now fixed (I've added the CHECK to pour_in on LIQUID).

Also, I did some more testing and it doesn't look like this type of omission was affecting any other similar verb that could move a liquid from a container into another — all other verbs fail for other reasons, e.g. because you can't carry the liquid in your bare hands (when trying to pour onto a surface); so attempting to locate a liquid into its current vessel is prevented by either proper checks or due to different ones standing in the way.

[AnssiR66]

Great that a solution was found! We will still have to take a look at the other bugs you mentioned appearing with liquids (ex attribute not working properly etc. - is there still some other misbehavior, after this bug fix?)

[tajmone]

Great that a solution was found! We will still have to take a look at the other bugs you mentioned appearing with liquids (ex attribute not working properly etc. -

The ex attribute was fixed, now it's being honored everywhere (at least, I think so, unless I've missed out some verbs). As for the currently open bugs for LIQUID, I should be able to fix all of them in the weekend; they're rather minor BUGs.

is there still some other misbehavior, after this bug fix?)

Unfortunately yes. When I was working on the Alan Italian port of the Library, I had encountered over 100 BUGs, but didn't really manage to create a full list of all of them. I had decided to face them one-feature at the time (e.g. liquids, clothing, etc.), but then I ran short of the time I could dedicate to the project, and never had a chance to go back to it in over a year.

The best approach is always uncovering bugs via the test suite, and then create an issue for each encountered bug (or design flaw, which is not really a bug). Without proper tests coverage, the risk is that attempts to fix a problem can disrupt other parts of the library in unexpected ways. Also, sometimes fixing a specific bug also removes other bugs. Also, from experience I've learned that opening an Issue for a bug which isn't covered in the test suite is not really useful — the test suite grants you immediate access to the BUGs and the context that produces them, allowing quick testing with real use case examples.

Unfortunately, the test suite int his repo isn't as extensive as the one on Alan Italian (I had managed to spend much more time on that one).

Also, some bugs (but especially design flaws) are hard to fix without a Library Design Specification document. In some cases, arbitrary choices are required to fix the problem, and without some guidelines it's not easy to make coherent choices.

A simple example of this would be CHECKs priority:

• If a verb would fail due to both actions restriction and invalid parameter, which CHECK should have higher priority?
• Should all self-directed actions be implemented on the Hero, separately from the main VERBs bodies?

I'd say that the action-restriction should have the precedence, otherwise if the player is told that the verb is malformed he/she'll try it again, only to discover that the action is not possible anyhow. As for verbs directed to the Hero, implementing the various CHECKs and DOES ONLY directly on the Hero would certainly lead to cleaner code in the main VERBs, and possibly allow to group together multiple similar verbs under single definitions on the Hero instance; but, on the other side, this could affect the priority of CHECKs executions, lessening the degree of control we could have on which CHECKs should run first.

But of course, these are all arbitrary choices, and ultimately they boil down to deciding some formal rules to guide this decisions, and make them available in a Design Specs document.

There are some edge cases where some verbs take parameters that involve multiple features and classes, resulting in quite entangled situations that are not always easy to track "mentally" (i.e. without some reference tables that can be consulted to learn what classes, attributes and verb overrides might be at play).

In any case, for the upcoming v2.2.0-RC we'll have to limit BUGs fixing to those which require simple fixes, and postpone the more intricate stuff to a later release. But I think we've already managed to uncover and fix many problems already, and probably the intricate stuff will be solved once we look again into the Library design core, which should naturally result in double checking all verbs and spotting potential issues.