AnssiR66 / AlanStdLib

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

Actions-Restrictions Not Working As Expected #113

Open tajmone opened 3 years ago

tajmone commented 3 years ago

@AnssiR66, while working on the example code you sent me for the yes and 'no' verbs, I've realized that the whole actions restrictions feature isn't working as expected, and doesn't work as described in the StdLib documentation.

I had to heavily re-adapt the sample code you sent me, in order to make it work. Please have a look at the working example which I've added to example adventures that ship with the StdLib Manual:

The problem here is tied to the fact that the library checks actions restrictions via the check_restriction EVENT, which is executed at every turn, in the background. Because the interpreter executes EVENTS after player commands, this means that if some code in the adventure changes the value of restricted_level, and then tries to enable/disable some custom verbs (to override the default actions restrictions configurations), these custom attributes will be lost when the check_restriction EVENT will finally be executed — i.e. whenever we alter the value of restricted_level OF my_game, we're actually telling a scheduled library event to reset the actions restrictions according to a given configuration.

This is why in order to fix the yes and 'no' example I had to introduce an EVENT to effectively enable the yes and 'no' verbs after setting a restriction level of 4 (all verbs blocked).

Frankly, this makes the whole actions restrictions feature quite unpractical to use, unless you stick to the default configurations of enabled and disable verbs.

As an immediate solution, I propose to add a new restriction level to provide a dedicated configuration for the yes and 'no' verbs — i.e. all verbs blocked except those two. After all, these two verbs can only be used with that specific level of restriction, so we might as well provide a default configuration explicitly targeting their use — which could also temporarily change the restricted_response to "Please answer YES or NO.", and restore it to the default message in all the other configurations.

We also need to amend all the actions restrictions documentation, both in the library source comments and in the StdLib Manual.

But in the the long term, we should think of a better approach to the whole actions restrictions feature and how it's implemented, trying to make it more accessible to authors who wish to override the specific configurations.

tajmone commented 3 years ago

Proposed Solution (dropped)

EDIT: The proposed solution is not implementable! (see below)

Right now, if we want to allow authors to easily lock/unlock custom verbs bypassing the default restriction levels configurations, we could extend the actions restrictions feature as follows...

In lib_verbs_restrictions.i, extend the code of the current check_restriction EVENT so that after having checked if the restricted_level value has changed (and fixed the restrictions settings accordingly) it will always:

  1. If the my_game:actions_block is a non empty Set:
    • Iterate through all its referenced attributes (i.e. the my_game action-restriction boolean attributes), set them to CAN NOT and remove them from the set.
  2. If the my_game:actions_allow is a non empty Set:
    • Iterate through the set elements (as above), set them to CAN, then remove them (i.e. empty the Set).

Basically, we would be adding two new attributes to my_game: my_game:actions_block and my_game:actions_allow (both Sets of references to attributes type), which the author can use to block/unlock specific verbs, regardless of whether he/she's changing the current restricted_level value. Both sets are consumed by the check_restriction EVENT at every term, so they will always be empty at the next turn — they work like "queues" in which to store the list of actions to lock/unlock at the next scheduled execution of check_restriction.

Example:

-- Block every action/verb:
Set my_game:restricted_level to 4.

-- Then unlock YES and NO verbs:
Set my_game:actions_allow {my_game:yes, my_game:'no'}.

Of course, the author could also use INCLUDE and EXCLUDE to manipulate the two sets, thus allowing different parts of the adventure to dynamically manipulate the list of actions-restriction that will be affected. Sets are very flexible, and since we're using the check_restriction EVENT as if it was a "delayed function call", authors could exploit this feature when manipulating the two sets of actions-restriction.

This approach would solve the original problem, which is caused by the fact that we can't really use EVENTs as if they were functions (since EVENTs are executed after the adventure code). By storing in attributes-Sets the desired actions to lock and unlock, we're delegating the check_restriction EVENT to handle their locking/unlocking.

Since these attributes are checked after the conditional part in check_restriction that deal with changes in restricted_level, the author will be able to use the two sets wherever he wishes to, independently of a change of restriction level.

All this is, of course, if we wish to provide custom verbs enabling/locking to authors. If, instead, we'd rather limit the library to provide preset configurations of which types of actions should be allowed for each restriction level, then the easier solution would be to just dump all the CAN [NOT] <action>. attributes, and replace the actions-restrictions CHECKS in every library verb with as simple CHECK restricted_level OF my_game < X, where X would be restriction threshold value above which a specific verb is blocked.

In any case, I still suggest that we add a new restriction level, explicitly dedicated to YES and NO answers — I suggest making it the highest level, coming after the "block all" level, because it's a "special" level, since it also changes the default library message.

AnssiR66 commented 3 years ago

Good that you spotted the bug in how the restricted actions work in the library, and the solution you suggest sounds fine. We can address the issue in the way you propose.

tajmone commented 3 years ago

Excellent!

Regarding the new Restriction Level for YES/NO answers, what do you think is better, making it Level 4 (shifting all actions blocked to 5) or making it Level 5 (a special independent level)?

The former would break backward compatibility, but will respect the general order of each level restriction further actions, with the new Level 4 being the last level still allowing some actions (only YES/NO answers).

The latter approach would preserve backward compatibility, and somehow emphasized that the YES/NO level is a "special level", for it also changes the default restricted actions message to "Please answer YES or NO!".

Of course, regardless of which level number we choose, we'll add some dedicated code for this level in order to preserve a copy of the current restricted actions message (whatever its value is) and restore it when another level change is detected. It will require and additional attribute to store a copy of the current restricted actions message, but it will always preserve the message, even if the author has changed it (mid game or otherwise).

Since the YES/NO answers level is one of the most useful ones, it's worth the additional effort on our side, which translates to less work for authors.

AnssiR66 commented 3 years ago

So... maybe the latter approach? For backward compatibility issues etc.

tajmone commented 3 years ago

Level 5 Implemented!

OK @AnssiR66, the new YES/NO Restriction Level was implemented.

You can see it in action in the update test and transcripts:

And the update original example, which now has considerably less code, thanks to the new feature:

tajmone commented 3 years ago

Can't Use Set Attributes

Unfortunately, the above proposed solution of using Set attributes to store a queue of references to the actions restrictions attributes is not implementable.

I wrongly assumed that Set attributes could store any ALAN type, including references to instance-specific attributes, but in reality they can only contain integers or references to instances (see ALAN Manual, sec. Set Type Attributes).

I'm afraid that the only viable solution right now is to update the StdLib Manual to inform end users that if they need to change Restriction Level AND enable/disable specific verbs within a same turn, they'll need to carry out the latter part in a custom EVENT.

If the adventure doesn't alter the value of restricted_level OF my_game during a turn, then it's possible to safely use the classical MAKE my_game NOT talk. approach, for these changes won't be overridden by the check_restriction EVENT.

So, unless we find an alternative solution (none comes to my mind right now), we can only update the documentation regarding this problem. Most likely, the current actions restrictions presets already cover the most common uses cases, by blocking actions according to type.

Also, with the newly added Level 5 for YES/NO answers, the library now covers another common uses case of the actions-restrictions feature (if not the most common), thus further relieving end users from the burden of having to deal with the nitty gritty of locking all actions except for YES and NO.

Having to resort to custom EVENTs to override the actions presets of the various levels is a bit impractical, but it's not the end of the world (and providing detailed documentation and a good example should suffice).

I'll keep trying to find an alternative solution to the problem. If you have any proposal let me know.

AnssiR66 commented 3 years ago

Great that level 5 is implemented! We just have to find a solution that the remaining problem. I will take a look into the code. Why again is the check_restriction event carried out every turn? Do we need it? (I haven't taken a look at how everything works here, for some time, so I need to refresh my memory. I will be back shortly with further comments.)

AnssiR66 commented 3 years ago

If using an event causes a problem here, can we find a workaround by using for example instances and 'describe'? I mean: Instead of :

SET restricted_level OF my_game TO 1.

we might use

DESCRIBE restricted_level_1.

by defining something like the following:

THE restricted_level_1 ISA DEFINITION_BLOCK DESCRIPTION ..... MAKE my_game NOT ask. MAKE my_game attack. ..... MAKE my_game take. MAKE my_game NOT talk. ...

END THE.

THE restricted_level_2 ISA DEFINITION_BLOCK DESCRIPTION .... MAKE my_game NOT attack. MAKE my_game NOT ask. .... .... END THE.

etc.. with all verbs in all levels separately defined as working or not working (and not building on the previous level.)

Descriptions make the change happen instantaneously, and we wouldn't have to worry about events.

This might be not as elegant ("DESCRIBE restricted_level_1 ...." is not as intuitive as "SET restricted_level OF my_game TO 1. etc) but it might eliminate the problem of using events. If I can think clearly at the moment, this approach should eliminate the every-turn check_restriction event. Does this make any sense, or would you get any other ideas of something similar, based on this?

tajmone commented 3 years ago

That's an interesting solution. I'd avoid creating them as instances of DEFINITION_BLOCK, because it would add a huge memory bloat, and just create them as instances of location — or maybe, even better, we could create a dedicated restriction_level subclass of location for the task, which might be more practical in the future, if we need to add some commonly shared behavior to these levels.

Also, if we were to create them as DEFINITION_BLOCK instances, we might run into problems with their INTIALIZE code, which makes every location a nested location of the DEFINITION_BLOCK — which is what the library expects for the my_game instance, but not for others. The DEFINITION_BLOCK is what's commonly referred to as a singleton, i.e. a class designed to be used for a single instance; so I think that there should never be any other such instance beside my_game, otherwise there might be unexpected consequences due to the initialization code destined for my_game being executed on other instances too (e.g. every location might no longer be nested in my_game, thus breaking the feature that allows overriding library verbs via my_game definitions). Previously, the Banner was declared as a DEFINITION_BLOCK instance, but we fixed that a long time ago (I think the banner was preventing verbs on my_game from being in scope, which is why I though the feature didn't actually work, maybe because I had tried it when the Banner was still a DEFINITION_BLOCK, in v2.0).

This new approach needs to be tested extensively, in a separate branch, before inclusion into the main code. But yes, even if it's not as elegant and intuitive a solution as the current restriction attribute, it would make life much easier for end users — having to create a custom EVENT to be able to override a few actions beside the default presets is overkill.

In any case, using EVENTs as if they were functions is not less inelegant than using DESCRIBE statements for the same purpose — the only advantage of the original attribute + EVENT system was its intuitiveness, when reading the code.

AnssiR66 commented 3 years ago

Ok! We can of course use another instance type than 'definition_block'; I didn't remember all the various features that instances of that class inherit, so it definitely is not ideal here. The instance for 'restricted_level_1' etc need not be a location at all; it can also be an object, or even a thing, as well (THE restricted_level_1 ISA THING, etc.).

tajmone commented 3 years ago

I think that using a location is safer, because if we use a thing it would end up being caught in omnipotent verbs that allow using ALL as a parameter; on the other hand, it's really rare that end users implement verbs that take locations as parameters (although it's possible), and even rarer that such verbs would allow using ALL (although they would be omnipotent) — in any case, for such verbs the problem would affect also the nowhere, my_game and banner locations.

One thing that we need to do, though, is tweak the INITIALIZE code of DEFINITION_BLOCK to exclude instances of the restriction_level class from inclusion in the nested Set (just like it's now being done for nowhere and my_game).

tajmone commented 3 years ago

Using a Single LOCATION for Restrictions Levels

After having considered for a while the proposed solutions, I think that the best approach would be to use a single location and preserve the restricted_level attribute.

If we create a location for every restriction level, we'll end up not only with 6 extra hidden in-game locations, but each DESCRIPTION block of every such locations would have to set/clear every single action attribute (there are 172 of them), according to the target level.

On the other hand, if we use just one location, and preserve the original restricted_level attribute, we can reuse the original code from the check_restriction EVENT as it is, without changes.

Memory bloat aside, the single-location/single-code approach is also easier and safer to maintain, e.g. if we ever need to add/remove an action to/from the lot, and (most important) it's going to be much easier for end users who wish to add their custom verbs to lib_verbs_restrictions.i, for they'd have to edit the code in a single place (bear in mind that some attributes already occur more than once in the current code, so multiplying the locations would make it very hard to track all the attributes).

We could even name the location restricted_level, like the attribute, so end users will:

SET restricted_level OF my_game TO 4.
DESCRIBE restricted_level.

which, for some reason, seems to diminish the awkwardness of having to use DESCRIBE to render effective the changes in restriction level (maybe it's the illusion that these instructions seem to refer the same thing).

This new system will indeed break backward compatibility, but only slightly — end users will only have to add the DESCRIBE statement after every change of restricted_level, which is not a big deal.

Separate Location for YES/NO?

With this new system, we could even consider detaching Level 5 for YES/NO answers, and dedicate an independent location for this special level — e.g. yes_no_restriction. This way, end users would only need to write

DESCRIBE yes_no_restriction.

to switch to this mode, without needing to set the restricted_level attribute.

Behind the scenes, in the library module, the DESCRIPTION block of this special room could trigger a Level 4 restriction and then, after, just unlock the YES and NO verbs and change the default restricted actions message:

SET restricted_level OF my_game TO 4.
DESCRIBE restricted_level.
MAKE my_game CAN yes.
MAKE my_game CAN `no`.
... etc. ...

so in this case, the extra location wouldn't introduce redundant code nor memory bloat.

I'm not sure whether this is needed or not, but it might make sense to keep the special YES/NO level separate. What do you think?