-
-
Notifications
You must be signed in to change notification settings - Fork 688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the event system as a contrib #1290
Conversation
Thanks, I have a pending review going but it will take a little while to go through it all. Might put up my review of the documentation separately from the code itself. |
@Griatch: no worry, I know it's quite big. Perhaps indeed the documentation will help before diving into the code (I know you already read some of it). Don't hesitate to ask for clarifications here or on IRC if I'm around (I won't be on it all day, teaching, but I'll be here tomorrow and the following days, I think). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contrib! It's comprehensive and has a lot of potential! I have reviewed the documentation (both files) so far and have given specific comments in it. I give a lot of feedback here but don't be disheartened, I'm just trying to help you shape this into something lean and useful.
Here are some overview thoughts on the docs.
- The text is overall well written but way too wordy. It also tends to describe things that should be offloaded to existing Evennia documentation, like how to use the
@perm
command and the EvEditor. - I believe it's a mistake to assume this documentation is to be read by both the game dev Immortal (which is presumed to know Python) as well as the builder(s) they trust enough to invite in. This is source-code level documentation. Aim it at the developer only - that's the person that will have to decide if they want to use this system in the first place and will always be the first user to try it out.
- For this reason, the two files should both merge into the README file and shortened considerably. The 'Event user documentation' contains critical information without which the event system makes no sense to the developer either. If there is a need for a more higher-level description for less Python-savvy, that's something for a different, third document to cover. As it is, neither document on its own gives me the whole picture of what this system is or how it hangs together.
- I would like to see an overview of all the moving parts, all in one place. What happens when you define and event, where is the executable code stored and just which parts must the developer specify and which one can the event-user work with. Having read both documents and assuming I came at this as a newcomer, this would still not be clear to me.
- A suggestion is to start your description of a function, command or method by an example of how that method is called. That will cater for the experienced users with one glance. Only after that you can describe more detail, clarifying things for people of all experience levels.
evennia/contrib/events/README.md
Outdated
|
||
Being in a separate contrib, the event system isn't installed by default. You need to do it manually, following three steps: | ||
|
||
1. Launch the main script: the event system is contained in a general script that holds all data. It has the advantage of saving nothing in your objects, and you can decide to turn it on and off fairly easily. In order to turn events on, you need to activate the script. Once executed, the script will remain, including after server reset or reload: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this first installation section to be superfluous - you are describing the same thing in the following sections as you do here, and almost as verbosely. It makes it not only longer but also harder to keep updated. I would suggest removing everything in this section except for a simple quick list:
- Launch the main event handler script
- Set permissions for who can create, edit and review events
- Add the new
@event
command to the default character.
That is all an experienced Evennia user needs, they can then read up on the details in the subsequent sections.
evennia/contrib/events/README.md
Outdated
|
||
This contrib is installed with default permissions. They define who can edit events without validation, and who can edit events but needs validation. Validation is a process in which an administrator (or somebody trusted as such) will check the events produced by others and will accept or reject them. If accepted, the events are connected, otherwise they are never run. | ||
|
||
By default, events can only be created by immortals. They don't need to be validated by anyone, after all, immortals also have access to the `@py` command, so they are probably trusted to use it wisely and not to run dangerous code on your server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already mentioned Immortals and the @py
command in the introduction, no need to repeat it again.
evennia/contrib/events/README.md
Outdated
|
||
The events contrib adds three permissions in the settings. You can override them by changing the settings into your `server/conf/settings.py` file (see below for an example). The settings defined in the events contrib are: | ||
|
||
- `EVENTS_WITH_VALIDATION`: this defines a group that can edit events, but will need approval. If you set this to "wizards", for instance, users with the permission "wizards" will be able to edit events. These events will not be connected, though, and will need to be checked and approved by an administrator. This setting can contain `None`, meaning that no group is allowed to edit events with validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "group" is not a term used anywhere in Evennia, you should probably say something like 'permission' here too. Also, is this a list or a single string?
evennia/contrib/events/README.md
Outdated
EVENTS_VALIDATING = "immortals" | ||
``` | ||
|
||
This set of settings means that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this clarification is needed, better to expand on the original description of each permission above if you feel more meat is needed.
evennia/contrib/events/README.md
Outdated
|
||
If you have an active staff of immortals, or are yourself sufficiently active on your project and have some contributors, you might decide to grant the privilege to write events **with** validation to builders, for instance (wizards, as the above permission, will automatically be included). It is recommended not to give contributors the right to edit events without validation unless you know, for a fact, that you can trust them. Remember, events have the potential to do many things... including freeze or crash your server... and potentially worse. | ||
|
||
In addition, there is another setting that must be set if you plan on using the time-related events (events that are scheduled at specific, in-game times). You would need to specify the type of calendar you are using. By default, time-related events are disabled. You can change the `EVENTS_CALENDAR` to set it to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should group all settings together in the same list rather than have this at the end. It's easy to miss like this.
evennia/contrib/events/USERDOC.md
Outdated
# At 10:00 AM, the subway arrives in the room of ID 22. | ||
# Notice that exit #23 and #24 are respectively the exit leading | ||
# on the platform and back in the subway. | ||
station = get(id=22) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is get
made available in the general namespace of the event? Where is it coming from?
evennia/contrib/events/USERDOC.md
Outdated
station.msg_content("After a short warning signal, the doors close and the subway begins moving.") | ||
``` | ||
|
||
Behind the scene, the `call` function freezes all variables ("room", "station", "to_exit, "back_exit" in our example), so you don't need to define them afterward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scene->scenes
evennia/contrib/events/USERDOC.md
Outdated
station.msg_content("After a short warning signal, the doors close and the subway begins moving.") | ||
``` | ||
|
||
Behind the scene, the `call` function freezes all variables ("room", "station", "to_exit, "back_exit" in our example), so you don't need to define them afterward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel information like this we would like to learn from main description of the event handler mechanism, not as a comment to an example.
evennia/contrib/events/USERDOC.md
Outdated
|
||
Behind the scene, the `call` function freezes all variables ("room", "station", "to_exit, "back_exit" in our example), so you don't need to define them afterward. | ||
|
||
A word of caution on events that call chained events: it isn't impossible for an event to call itself at some recursion level. If `chain_1` calls `chain_2` that calls `chain_3` that calls `chain_`, particularly if there's no pause between them, you might run into an infinite loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A word on implementation - it should be perfectly possible to protect again infinite loops by using a call stack and monitor for loops.
evennia/contrib/events/USERDOC.md
Outdated
|
||
There are a lot of ways to make mistakes while writing events. Once you begin, you might encounter syntax errors very often, but leave them behind as you gain in confidence. However, there are still so many ways to trigger errors: passing the wrong arguments to a helper function is only one of many possible examples. | ||
|
||
When an event encounters an error, it stops abruptly and sends the error on a special channel, named "everror", on which you can connect or disconnect should the amount of information be overwhelming. These error messages will contain: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced this error reporting mechanism is a good idea, at least not as the only reporting outlet. It can be very spammy and hard to find your errors if you have many people making events sharing the same error channel. Is there really no way to have the system echo errors to you during development but once things are 'in the wild', report errors to the channel?
@Griatch: thanks for the review. Creating good documentation is important, I believe, and I'm more than willing to improve it. A good system with bad documentation is, as one of my teachers said, a bad system altogether. I have checked all the comments you have given (in general or specific lines). Some, I'll address directly in the files, but I thought some clarification was due, and I should have said it in the original description of this PR:
Thanks again for the review, |
It took me some time, but I've merged both documents and addressed the suggestions, errors and global structure. The README file, as it is, is not shorter at all, since it now contains the user documentation I had designed. However, some sections have been shortened... others have been added... and still others have been put in a different place. There is now a section dedicated to the structure of the system (to help users see the relation between event types, triggers, code hooks, commands, and individual events). I also added a practical example on how to extend the system by adding a new event type for a specific action (this includes every steps, including the command to link the new event type). I have added some examples to try to make things more clear when editing an event as well. And I have removed some information that wasn't really helpful and somewhat confusing. Also, it's quite possible I overlooked/forgot some modifications, I'm not sure I intercepted all line comments. Here are more specific answers/questions I would like to ask you before going on though:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through the code and started re-reviewing the documentation again. You write good code and are very deliberate in your documentation! I however come to the conclusion that 'event system' is not really a correct term for this. The main point of this system is to allow users to add their own code from in-game, and what you are describing as them adding are not events. Hear me out.
First of all, unless I completely misunderstood what you have done here, Evennia and the Event system does fire events. Those are events like 'traverse' or 'move'. You then set up a typeclass with an event handler (you somewhat confusingly refer to this as an event type or trigger). The custom code people then add comes in the form of a callback or maybe an action that fires when the event fires.
That very last part is what you call an 'event', which I think is wrong. This mixup of terms is why I've had a hard time following your thinking on this. In my review I suggest a fundamental change of the terms you use to describe your components, from event type, trigger and event to event, event handler and callback. This makes more sense to me, but I'd be willing to discuss (the current use is really confusing to me though).
evennia/contrib/events/custom.py
Outdated
@@ -0,0 +1,265 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that this is an utils.py
module. The name custom
doesn't really tell me that it has utilities for the developer to use. If it's named utils
you can also put the lone exception in here and get rid of a spurious file for a single class.
evennia/contrib/events/custom.py
Outdated
custom_call (function, optional): a callback to call when | ||
preparing to call the event. | ||
|
||
Events obey the inheritance hierarchy: if you set an event on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes under a Notes:
documentation block header.
evennia/contrib/events/custom.py
Outdated
Args: | ||
format (str): a time format matching the set calendar. | ||
|
||
The time format could be something like "2018-01-08 12:00". The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should either indent directly under the format
argument block or as a Notes:
section at the end.
evennia/contrib/events/custom.py
Outdated
Create a time-related event. | ||
|
||
Args: | ||
obj (Object): the object on which stands the event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an event "sits" on an object, not "stands". English and its weird positions!
evennia/contrib/events/exceptions.py
Outdated
@@ -0,0 +1,15 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module could be merged into the custom.py
module (renamed to utils.py
).
evennia/contrib/events/README.md
Outdated
|
||
## A WARNING REGARDING SECURITY | ||
|
||
Evennia's event system will run arbitrary Python code without much restriction. Such a system is as powerful as potentially dangerous, and you will have to keep in mind two important questions, and answer them for yourself, before deciding to use this system in your game: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without any restriction, really. The two points can be shortened considerably:
- Untrusted people can ruin your server with this.
- You can do all of this in Python outside the game.
I won't force you to rewrite it, but if the text is too long people will just miss the critical warning that this really is.
evennia/contrib/events/README.md
Outdated
|
||
## Basic structure and vocabulary | ||
|
||
- At the basis of the event system are **event types**. An **event type** defines the context in which we would like to call some arbitrary code. For instance, one event type is defined on exits and will fire every time a character traverses through this exit. Event types are described on a [typeclass](https://github.com/evennia/evennia/wiki/Typeclasses) (like [exits](https://github.com/evennia/evennia/wiki/Objects#exits) in our example). All objects inheriting from this typeclass will have access to this event type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should really push the fact that since an event type is something you code on the typeclass it is thus done by the developer as part of the core game design and not in-game.
evennia/contrib/events/README.md
Outdated
## Basic structure and vocabulary | ||
|
||
- At the basis of the event system are **event types**. An **event type** defines the context in which we would like to call some arbitrary code. For instance, one event type is defined on exits and will fire every time a character traverses through this exit. Event types are described on a [typeclass](https://github.com/evennia/evennia/wiki/Typeclasses) (like [exits](https://github.com/evennia/evennia/wiki/Objects#exits) in our example). All objects inheriting from this typeclass will have access to this event type. | ||
- An event type should specify a **trigger**, a simple name describing the moment when the event type will be fired. The event type that will be fired every time a character traverses through an exit is called "traverse". Both "event types" and "trigger" can describe the same thing, although the term **trigger** in the rest of the documentation will be used to describe the moment when the event fires. Users of the system will be more interested in knowing what triggers are available for such and such objects, while developers will be there to create event types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confuses me, reading as a newcomer to these concepts. Where is the name 'traverse' coming from? How do I know this is what it's called? Is that something you have hardcoded in the event system? I feel there is a confusion of terms here, you are using the term "events" both for the real events fired by the engine (like 'traverse') and for the code callbacks you are assigning to individual objects to handle those events. The latter are not events and this mixup I think is the reason I have not been able to figure out just how your system is meant to work. Here is how I understand the system, along with what I think would be better names for the components. Correct me if I'm way off:
- Someone traverses an exit. Evennia(and the event system) fires an event of type traverse. The event sometimes include arguments relevant for that particular event.
- You can add event handlers to your typeclasses. An event handler will catch events of a given type (like 'traverse').
- To these event handlers you in turn attach code callbacks. These execute when a given events fire making use of eventual arguments from the event. These callbacks can be different for every instance of the typeclass on which the event handler sits and the code for the callback is added from in-game.
Note the change in nomenclature here. Trigger to Event, Event type to Event Handler and Event to Callback.
evennia/contrib/events/README.md
Outdated
- `"custom"`: a custom calendar that will use the [custom_gametime](https://github.com/evennia/evennia/blob/master/evennia/contrib/custom_gametime.py) contrib to schedule events. | ||
- A special callback to schedule time-related events in a way not supported by the `gametime` utility and the `custom_gametime` contrib (see below). | ||
|
||
#### Permissions on individual users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be merged with the previous section and a reference given to the permission section of the wiki.
evennia/contrib/events/README.md
Outdated
+------------------+---------+-----------------------------------------------+ | ||
``` | ||
|
||
### Creating a new event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is, as said, unclear because the "event" is the event sent by the engine. like 'move' or 'puppeted'. What you are describing here is how to attach a callback to the event, for doing something when that event fires.
@Griatch: thanks for your review. I will just address one question here, but it will affect most of the PR itself, since we're considering a change of names: I approve of the new nomenclature. It would make things easier. I was against "callbac" at first because it implied "events" run like functions... but don't they? They have variables too (like arguments). They have parameters and that's not exactly the same but perhaps I can find something else for this name, unless my documentation is clear on what "callback parameters" mean. The real question is on "event handler". This proposal to replace "event type" isn't a bad idea, but I'm afraid "event handler" will come into conflict with what the current "events handler" is doing. The current "events handler" (to be renamed "event handler") adds a lazy property on all typeclassed objects: self.events Event types (to be renamed "event handlers"?) represents definitions of what event can be set on an object (what callbacks can be added to it). Isn't that going to create confusion? |
@Griatch: I'm going to update the names in the event system, but I need your opinion for one. You suggested to rename "event type" to "event handler". Trouble is, there's also an event handler in code (the |
@Griatch: renaming and documentation updates are now done!
Things that remain to be decided:
|
READY FOR REVIEW! I just coded the alternative handling of errors we discussed, @Griatch. You can review the entire system, when you get the time and motivation. |
@vlegoff Just as a heads up - the unittest suite fails for this PR at this time. |
Strange. Unittests pass with me. What could cause this issue? |
It seems at least 2 of the reported errors have to do with the EvEditor unable to open (at least, it's not in the caller's |
@Griatch My apologies. It worked, but not on a clean install. It should now be fixed. |
@vlegoff Looks a lot better now. I still find the readme file too long but that's something we could gradually refine as we go. The change of names makes the functionality more intuitive and the use is more clear. The use of |
Brief overview of PR changes/additions
This PR adds the event system as a separate contrib in
evennia/contrib/events
.Motivation for adding to Evennia
As discussed previously, this contrib allows to customize individual objects without creating in-code modifications. This allow for quick customization of individual objects. Motivations and examples can be found in the documentation (see the resource section).
Summary of the contrib structure
commands.py
contains theCmdEvent
class (the@event
command).custom.py
contains mostly top-level functions used to extend the system (mostly adding new event types).exceptions.py
contains exceptions used by the system.handler.py
contains the events handler defined on all objects and accessible through a lazy property.helpers.py
contains top-level functions accessible inside event code.scripts.py
contains scripts of events, that is the main event handler (a global script that contains all events) and the definition of time event scripts. It is rather a huge file, since most of the logic actually happens in it.tests.py
contains the contrib's unittests, encompassing testing the command, the script, the handler and some basic event types.typeclasses.py
contain the definition of modified DefaultObject and similar Evennia objects. Note that these aren't inherited, rather, individual hooks are patched whenever necessary. This way of doing is controversial at best, but it avoids complicating the hierarchy tree for just a few methods, while allowing a greater flexibility in what to include or not.Resource: