search

AnssiR66 / AlanStdLib

The Standard Library for ALAN Interactive Fiction Language
Other
5 stars 2 forks source link

Bug in 'fill_with' #39

Closed tajmone closed 3 years ago

tajmone commented 5 years ago

EDIT — I've tried repeating the test, it seems that the bug was fixed (initially I though it wasn't, but I was wrong).

It seems it was fixed in commit 852de32, I want to learn more about it, just to mention this in the CHANGELOG

NOTE — from the commit's message, regarding how this was fixed:

I've discovered a bug in the fill_with verb implemented on liquid class.

PR #38 provides the test files for this bug. Look at what happens in liquids.a3log:

There is a jar and a bottle here. The bottle contains some wine.

> ; ==============================================================================
> ; INITIAL STATE OF CONTAINERS
> ; ==============================================================================
> ; The bottle has wine in it, the jar is empty:
> x bottle
The bottle contains some wine.

> x jar
The jar is empty.

> ; ==============================================================================
> ; THE 'FILL WITH' BUG!
> ; ==============================================================================
> ; Now we attempt to fill the jar with wine (it will fail by default):
> fill jar with wine
You can't fill the jar with the wine.

> ; IMPLICIT TAKING REVEALS BUG:
> pour wine
(taking the jar of wine first)

> ; NOTE: The wine has "magically" moved to the jar! This is due to a bug in how
> ;       'fill_with' is implemented on liquids --- i.e. it always sets the vessel
> ;       of 'substance' TO 'cont' (regardless if the action failed)!

The root of the problem here is twofold:

  1. The fill_with verb in lib_verbs.i is defined to always fail the action by default:

     DOES
   "You can't fill" SAY THE cont. "with" SAY THE substance. "."
   -- allow the action at individual substances only

  2. The fill_with verb on liquids is defined to always set the the vessel of substance to cont, regardless of wether the action succeeded or failed:

     VERB fill_with
   -- when something is filled with a liquid, this something becomes the
   -- vessel of the liquid:
   WHEN substance
      DOES SET vessel OF THIS TO cont.
 END VERB fill_with.

Therefore, trying to fill a container with a liquid which is in another container, will alter that liquid's vessel — and, altough still in its original container, all verbs that look at its vessel will act erratically (eg implicit taking).

AnssiR66 commented 5 years ago

Good catch again Tristano! The test suite has proven to be really useful, thanks again for implementing it. I guess the things to do is to add a IF or CHECK to that fill_with code. Shall I go ahead with it, or do you want to experiment with the test suite a bit more?

tajmone commented 5 years ago

I've tried to fix it, but it's a bit complicate.

As for the fill_with verb on liquids, all it requires is an If checking that the substance has effectively been moved to cont before changing it's vessel. I've tried that, and it works.

The problem has more to do with the main fill_with verb, which as a default doen't do anything. The issue has to do with the fact that the author need to implment a custom fill_with verb on the instances he wished to allow being "filled with", and if he uses a DOES ONLY then the fill_with body from liquid won't be executed, but if he uses just DOES (or DOES BEFORE/AFTER) than the "You can't" message from the main verb will show up!

I haven't worked out a way to keep the liquids' fill_with doing its job and at the same time prevent the response from the default verb showing up. Probably some temporary attribute might be required here to prevent the default message from creeping in.

In Alan there isn't a correspective of Inform 7's instead, is there? It's either ONLY/AFTER/BEFORE, right?

I think it's quite important to be able to preserve the SET vessel OF THIS TO cont. part from the liquid's verb body, so that author don't have to worry about and can let the library handle that behind the scene.

Have you any idea how to solve this?

AnssiR66 commented 5 years ago

I'll take a look at this, but when I look at the test code transcript you provided, it doesn't use the fill_with verb at all. So, the problem lies (also) somewhere else..? I think the 'pour' verb is what needs to be looked at (besides fill_with)? -Anssi

From: Tristano Ajmone notifications@github.com Sent: Saturday, November 3, 2018 4:07 PM To: AnssiR66/AlanStdLib Cc: AnssiR66; Comment Subject: Re: [AnssiR66/AlanStdLib] Bug in 'fill_with' (#39)

I've tried to fix it, but it's a bit complicate.

As for the fill_with verb on liquids, all it requires is an If checking that the substance has effectively been moved to cont before changing it's vessel. I've tried that, and it works.

The problem has more to do with the main fill_with verb, which as a default doen't do anything. The issue has to do with the fact that the author need to implment a custom fill_with verb on the instances he wished to allow being "filled with", and if he uses a DOES ONLY then the fill_with body from liquid won't be executed, but if he uses just DOES (or DOES BEFORE/AFTER) than the "You can't" message from the main verb will show up!

I haven't worked out a way to keep the liquids' fill_with doing its job and at the same time prevent the response from the default verb showing up. Probably some temporary attribute might be required here to prevent the default message from creeping in.

In Alan there isn't a correspective of Inform 7's instead, is there? It's either ONLY/AFTER/BEFORE, right?

I think it's quite important to be able to preserve the SET vessel OF THIS TO cont. part from the liquid's verb body, so that author don't have to worry about and can let the library handle that behind the scene.

Have you any idea how to solve this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/39#issuecomment-435590561, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHvd8n5g1WsBx9_rS3SFY0bndZNCiks5uraMhgaJpZM4YMejc.

AnssiR66 commented 5 years ago

Sorry, my bad, the fill_with is there in your example code alright, sorry for my oversight. I need to think about this for a bit, I think there is a workaround that can be implemented. -Anssi

From: Anssi Räisänen anssir66@hotmail.com Sent: Sunday, November 4, 2018 3:23 PM To: AnssiR66/AlanStdLib Subject: Re: [AnssiR66/AlanStdLib] Bug in 'fill_with' (#39)

I'll take a look at this, but when I look at the test code transcript you provided, it doesn't use the fill_with verb at all. So, the problem lies (also) somewhere else..? I think the 'pour' verb is what needs to be looked at (besides fill_with)? -Anssi

From: Tristano Ajmone notifications@github.com Sent: Saturday, November 3, 2018 4:07 PM To: AnssiR66/AlanStdLib Cc: AnssiR66; Comment Subject: Re: [AnssiR66/AlanStdLib] Bug in 'fill_with' (#39)

I've tried to fix it, but it's a bit complicate.

As for the fill_with verb on liquids, all it requires is an If checking that the substance has effectively been moved to cont before changing it's vessel. I've tried that, and it works.

The problem has more to do with the main fill_with verb, which as a default doen't do anything. The issue has to do with the fact that the author need to implment a custom fill_with verb on the instances he wished to allow being "filled with", and if he uses a DOES ONLY then the fill_with body from liquid won't be executed, but if he uses just DOES (or DOES BEFORE/AFTER) than the "You can't" message from the main verb will show up!

I haven't worked out a way to keep the liquids' fill_with doing its job and at the same time prevent the response from the default verb showing up. Probably some temporary attribute might be required here to prevent the default message from creeping in.

In Alan there isn't a correspective of Inform 7's instead, is there? It's either ONLY/AFTER/BEFORE, right?

I think it's quite important to be able to preserve the SET vessel OF THIS TO cont. part from the liquid's verb body, so that author don't have to worry about and can let the library handle that behind the scene.

Have you any idea how to solve this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/39#issuecomment-435590561, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHvd8n5g1WsBx9_rS3SFY0bndZNCiks5uraMhgaJpZM4YMejc.

tajmone commented 5 years ago

No, you were right: we need to first identify all the problems that surround these verbs, so we can work out the best solution.

I've just finished creating a new test file for these verbs, published via PR #43.

From now on I'll be using just a few source adventures to carry out all tests. The new tests script allows multiple tests to be run against a same adventure, so I can create as many independent tests for each Alan source as I want.

This will allow the creation of more complex source adventures, allowing a broader scenario of possible tests, and at the same time to keep test scripts shorter, by targetting a specific class, group of verbs or feature with each script. Hopefully, it should optimize time and efforts.

Right now, the house_room-objects.a3log tests the floor instance of rooms, and it covers every verb that should be overriden for that instance. But, as the tests show, the CHECKs in most of these verbs are blocking their execution, so these alternative verb bodies never get executed.

Similar problems are to be expected with other room and site objects, and probably with liquid too. For now, let's isolate each problematic case, and build a test file that will allow us to easily detect how changes in the library code affect their output, and to work in a more relaxed manner.

Probably these are going to prove tricky to fix, because of the way VERB CHECKs are handled (it looks like that in Alan, once you've triggered a CHECK condition, you can't conditionally bail out from it and resume the CHECKS).

The solution is likely to involve the CHECKs in the main body, and to take into account special classes and instances like floor, ground, etc. before carrying out the normal checks for containers -- these are containers too, but don't act as normal ones since it's not expected that they should manage a list of allowed items at all (even if they did, their VERB bodies would block any verbs that could affect their contents, ie. if their verbs worked as expected).

AnssiR66 commented 5 years ago

I tried this:

At the end of the Liquid class coding, there's the event check_vessel.

I added some code to it so it now looks like this:

EVENT check_vessel FOR EACH liq ISA LIQUID, DIRECTLY AT CURRENT LOCATION DO SET vessel OF liq TO null_vessel. END FOR. -- so far, the same as before

-- this is the new part: FOR EACH lc ISA LISTED_CONTAINER DO FOR EACH liq ISA LIQUID, DIRECTLY IN lc DO SET vessel OF liq TO lc. end FOR. end FOR. -- end of new part SCHEDULE check_vessel AFTER 1. END EVENT.

Also, I added an IF clause to the fill_with verb inside the liquid class, like you did, but I am not sure if fill_with is now needed at all at the liquid class, as I added that extra code to the event. Maybe you could check through the test suite, if everything would work if we just change the event like above, and omit fill_with altogether from the Liquid class. According to my own tests, omitting fill_with from the Liquid class should be safe now, as long as we add the above code to the event. This should also cater for cases when the player uses fill_with with DOES ONLY when programming a custom liquid. Let me know what you find out in the tests, -Anssi

From: Tristano Ajmone notifications@github.com Sent: Saturday, November 3, 2018 4:07 PM To: AnssiR66/AlanStdLib Cc: AnssiR66; Comment Subject: Re: [AnssiR66/AlanStdLib] Bug in 'fill_with' (#39)

I've tried to fix it, but it's a bit complicate.

As for the fill_with verb on liquids, all it requires is an If checking that the substance has effectively been moved to cont before changing it's vessel. I've tried that, and it works.

The problem has more to do with the main fill_with verb, which as a default doen't do anything. The issue has to do with the fact that the author need to implment a custom fill_with verb on the instances he wished to allow being "filled with", and if he uses a DOES ONLY then the fill_with body from liquid won't be executed, but if he uses just DOES (or DOES BEFORE/AFTER) than the "You can't" message from the main verb will show up!

I haven't worked out a way to keep the liquids' fill_with doing its job and at the same time prevent the response from the default verb showing up. Probably some temporary attribute might be required here to prevent the default message from creeping in.

In Alan there isn't a correspective of Inform 7's instead, is there? It's either ONLY/AFTER/BEFORE, right?

I think it's quite important to be able to preserve the SET vessel OF THIS TO cont. part from the liquid's verb body, so that author don't have to worry about and can let the library handle that behind the scene.

Have you any idea how to solve this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/39#issuecomment-435590561, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHvd8n5g1WsBx9_rS3SFY0bndZNCiks5uraMhgaJpZM4YMejc.

tajmone commented 5 years ago

The above addition should help dealing with liquids poured on the floor! (haven't checked that yet).

As for the put_in problem, probably what we need here is to use that event (or another event running at every turn) to check that a liquid's vessel is always its current direct container (excluding floor, ground, etc.).

I'm not sure if this could create problems in complex situation (nested containers that an author might implement, e.g. a bottle of water inside a bath tub full of water, or a swimming pool), but this solution could make the fill_with verb on liquid superfluous, letting the event handle this instead.

tajmone commented 5 years ago

This problem with liquids keeps puzzling me. If there was a way in ALAN to check the direct cointainer an instance is currently IN, then it would be just a matter of making the every-turn Event for liquids set the vessel of a liquid to its current container — but, if I understand correctly, there isn't a way to directly check for this (this being the reason why the HAS vessel attribute is needed in the first place).

The alternative solution could be to use the every-turn Event to check if the referenced vessel is still the current direct cointainer of the liquid instance, and if not ... well then it will have to iterate through all the containers AT the liquid location until it finds the direct container and (if it's not floor or ground) set that as the new vessel.

A few considerations (for which I need your advise, for I'm not sure I've fully grasped the whole mechanism of liquids):

  1. A similar Event shouldn't introduce overhead, since it only iterates through all containers in case vessel is no longer the direct container.

  2. I've suggested iterating only through containers AT the liquid location, but it might also be done only for liquids which are AT current location (i.e., if the liquid was moved elsewhere due to the verb execution, ignore it for now).

    But in theory, some custom "put" verbs might end up locating the liquid elsewere (eg, by putting it in a pipe that leads to another room), in such cases it should usually be OK if the event won't take care of refreshing its vessel until the liquid comes into player's scope again; but there might be edge cases where this could lead to problems (verbs that allow referring liquids not AT CURRENT LOCATION, via !).

    Overall, I'd say that limiting the Event's iterations only to liquids AT player's location should keep the event execution smaller. When the player ends up in the (new) liquid's location, and it comes into scope, then the Event will correct the vessel — so there aren't any real risks of the player interacting with a liquid whose vessel is messed up. For all other cases (rare edge-case implementations), the author will probably have to tweak the Event accordingly (which I'm sure he/she will be capable of, since it would be advanced stuff anyhow).

  3. Should the Event do something special if it finds out that the current direct container is floor or ground? On this point I'm confused, because comments in lib_locations.i mention:

    THE floor ISA room_object
  IS NOT takeable.
  IS NOT movable.
  CONTAINER
    -- to allow 'empty/pour/put something on floor'

    but the tests show that after dropping something it doesn't end up in the floor/ground.

    > drop apple
Dropped.

> take apple from floor
The apple is not in the floor.

    So I'm not sure how floor/ground containment is intended to work.

  4. This every-turn Event auto-magically handling liquids' vessel should allow users to freely create their own put* verbs variations with DOES ONLY on instances and not have to worry about the vessel (and even if they did, the Event won't clash with it, it would just do nothing).

  5. Could there be some edge cases situations to envisage here, which could conflict with a similar Event? (e.g. indirect containments, or liquids inside liquids, etc.)

    My gues is that (although technically possible to achieve, by setting a liquid instance to also be a container) having nested liquids wouldn't make much sense since liquids mix together — and probably authors will have to override verbs that deal with liquids entering other liquids by substituting them with a new liquid, or keeping just one of them.

AnssiR66 commented 5 years ago

Hi Tristano,

You said:

If there was a way in ALAN to check the direct cointainer an instance is currently IN, then it would be just a matter of making the every-turn Event for liquids set the vessel of a liquid to its current container — but, if I understand correctly, there isn't a way to directly check for this (this being the reason why the HAS vessel attribute is needed in the first place).

There's "CHECK liq DIRECTLY IN cont". Did you mean this?

The alternative solution could be to use the every-turn Event to check if the referenced vessel is still the current direct cointainer of the liquid instance, and if not ... well then it will have to iterate through all the containers AT the liquid location until it finds the direct container and (if it's not floor or ground) set that as the new vessel.

Can you just briefly reiterate, in what way exactly are liquids not working at present, after the small fix we discussed some time back? Did that fix help matters in any degree?

A few considerations (for which I need your advise, for I'm not sure I've fully grasped the whole mechanism of liquids):

  1. A similar Event shouldn't introduce overhead, since it only iterates through all containers in case vessel is no longer the direct container.

  2. I've suggested iterating only through containers AT the liquid location, but it might also be done only for liquids which are AT current location (i.e., if the liquid was moved elsewhere due to the verb execution, ignore it for now).

But in theory, some custom "put" verbs might end up locating the liquid elsewere (eg, by putting it in a pipe that leads to another room), in such cases it should usually be OK if the event won't take care of refreshing its vessel until the liquid comes into player's scope again; but there might be edge cases where this could lead to problems (verbs that allow referring liquids not AT CURRENT LOCATION, via !).

Overall, I'd say that limiting the Event's iterations only to liquids AT player's location should keep the event execution smaller. When the player ends up in the (new) liquid's location, and it comes into scope, then the Event will correct the vessel — so there aren't any real risks of the player interacting with a liquid whose vessel is messed up. For all other cases (rare edge-case implementations), the author will probably have to tweak the Event accordingly (which I'm sure he/she will be capable of, since it would be advanced stuff anyhow).

     I agree that limiting the event in some way could make the execution more economical. We could make this change to the code.

  1. Should the Event do something special if it finds out that the current direct container is floor or ground? On this point I'm confused, because comments in lib_locations.i mention:

    1.

THE floor ISA room_object IS NOT takeable. IS NOT movable. CONTAINER -- to allow 'empty/pour/put something on floor'

but the tests showhttps://github.com/AnssiR66/AlanStdLib/blob/room-site-tests/tests/house_room-objects.a3log#L105 that after dropping something it doesn't end up in the floor/ground.

drop apple Dropped.

take apple from floor The apple is not in the floor.

So I'm not sure how floor/ground containment is intended to work.

Here, 'take apple' should be enough. For example Inform7 doesn't support thekind of detail of "take apple from floor", either; nor would a player usually make the command so long. Making the floor a container only allows the player command "pour liquid on floor", but in reality the object is only placed AT the location. So, it allows the wording but not the actual containment. Do you think this is ok?

  1. This every-turn Event auto-magically handling liquids' vessel should allow users to freely create their own put* verbs variations with DOES ONLY on instances and not have to worry about the vessel (and even if they did, the Event won't clash with it, it would just do nothing) Could there be some edge cases situations to envisage here, which could conflict with a similar Event? (e.g. indirect containments, or liquids inside liquids, etc.)

Yes, we would have to think with more time if there is any reason to be more wary here.

5.>>My guess is that (although technically possible to achieve, by setting a liquid instance to also be a container) having nested liquids wouldn't make much sense since liquids mix together — and probably authors will have to override verbs that deal with liquids entering other liquids by substituting them with a new liquid, or keeping just one of them.

Yes, I agree that nested liquids is not a way to go. Sooner, the author would implement a new liquid etc.

-Anssi

You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/39#issuecomment-437066972, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHsBit-NkOUaeegwiSoQlZzGdGAe1ks5utF6bgaJpZM4YMejc.

tajmone commented 5 years ago

There's "CHECK liq DIRECTLY IN cont". Did you mean this?

No, I meant if there was a way to obtain a reference to the container the liquid is in — i.e. having no clue what the container might be.

Can you just briefly reiterate, in what way exactly are liquids not working at present, after the small fix we discussed some time back? Did that fix help matters in any degree?

I'm not sure which fix you are referring to — is the Event you mentioned above?

The current tests still produce the bug of a liquid magically changing vessel after a failed fill_with attempt:

> ; The bottle has wine in it, the jar is empty:
> x bottle
The bottle contains some wine.

> x jar
The jar is empty.

> ; Now we attempt to fill the jar with wine (it will fail by default):
> fill jar with wine
You can't fill the jar with the wine.

> ; IMPLICIT TAKING REVEALS BUG:
> pour wine
(taking the jar of wine first)

> ; BUG: The wine has "magically" moved to the jar!

So far I haven't been able to find an easy solution to this that will relief the author from the burder of having to manually update the liquid's vessel when implementing a custom fill_with verb on a container or liquid.

tajmone commented 3 years ago

Re-Opening Issue, Bug Was Never Fixed

I've checked if this bug is still there, and it is. I'm not sure why it was closed, but I'm reopening it again, so that we can start working on it.

EDIT — I was wrong, it was fixed. I now only want to find out more to mention it in the CHANGELOG.