-
Notifications
You must be signed in to change notification settings - Fork 3
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
WIP: Transition Implementation #253
Conversation
46f6149
to
479ea35
Compare
@jessesnyder I hope this implementation hasn't violated the expectations you might have had when implementing transition display for #234, and isn't too difficult to integrate into your work. @silviot It would probably be a good idea to write some tests for these new handlers. |
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 all looks good to me. In the large, I found it easy to follow. I made some comments about some statement-level things I found challenging, though by the end of the review some of these ceased to bother me because they came up a lot and were used consistently in the same way. There are a couple places where I think some comments would help.
The JS implementation looks good to me. It's more consistent with the code that's already there than what I had been working on, and (unsurprisingly) more closely reflects what is effectively very parallel new code on the server side, and I think this is a strength, as it's easier to identify equivalent concepts across the two different contexts.
|
||
transitions: | ||
- actor_end: 5 | ||
actor_start: 2 | ||
- actor_start: 2 |
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 thought: do we need to be using integer IDs, or are we just making life hard for no reason?
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.
There may be some place where we're doing an int(item_id)
, but I don't think there's anything in the code that requires integers. I'm sure it had some advantages in the original C code for OneHourOneLife though. That's just the convention Natalia used in her sample config, but we can test it out with some string ids. Using a mix will probably cause comparison errors when doing lookups, but as long as they are consistently integers or strings it should be OK.
@jessesnyder would you prefer I merge this now to make your work easier to rebase/merge or should I wait for @silviot to add tests and/or review further? |
This PR implements various aspects of stories #236, #241, and #237, and provides the basic functionality that's needed for the transition related aspects of #234.
It provides the following actions triggered by pressing the space bar:
item_transition
: executes a transition (if a transition exists between the player's item and map item)item_pick_up
: picks up the item in the current square (if it is portable)item_consume
: consumes the currently held item (if it has a caloric value)There's also a new action executed with by pressing the
d
key:item_drop
: drops the currently held item onto the current map square (if it the square is empty)We provide a simple display of the currently held item and a mechanism for determining transition visibility. A transition can set its
visible
attribute toalways
,never
, orseen
. This does not attempt to provide information about available transitions or the item on the current square in the UI.The client side can look up the currently available transition using
transition = player.getTransition()
and can determine if the transition has been seen by the current player by checkingtransitionsSeen.has(transition.id)
.If the server side encounters an error when executing an action it will send an
action_error
notification withplayer_id
,position
,player_item
,item
properties which can be used to reset the state on the client side. Resetting state is not yet implemented (and may not need to be implemented, since it will be reset eventually by the game state sync).Experiment changes:
remaining_uses
property which starts with the itemn_uses
value and is altered based on the transitionmodify_uses
property (as well as by consuming items, see note below).null
position, which generally means they are being held by a player.position
needs to be able to change along with the newremaining_uses
property.limit_quantity
property which determines ifGridWorld.replenish_items()
should remove items of the type or just add them based on the growth rate. Without this, items which had an initial quantity of0
would be removed immediately when a player created one via a transition.last_use
are stored under a key that looks like('last', actor_id, target_id)
('last_$actorId_$targetId'
in JS).Notes:
Right now picking up aConfirmed with @nataliavelez that the current approach is correctportable
item results in picking up all its uses, and consuming such an item results in consuming a single use (reducingremaining_uses
). I think this may be backwards, picking it up should create a newItem
of the same type with a single remaining use held by the player while reducing theremaining_uses
of the map item. The current way was a little easier to implement, so it might be OK for now. Perhaps multi-use items will generally be like theGooseberry Bush
and notportable
, so this doesn't really matter.crossable
property, and I'm not sure how it will be possible to have a non-crossable item that you can execute a transition on, since you currently need to be on the square of the object to perform a transition.position
sent with these action requests as the player position and performs actions relative to that. At some point we might want to verify that is or very recently was the player position if we're concerned about cheating.item_drop
,item_pick_up
, anditem_consume
outcomes before sending the message to the server (to avoid lag). If there's a problem with the action on the server it sends anaction_error
message, which could be used by the client to reset to the original/correct state, but it currently goes unused.item_transition
, so the updated game state from a transition won't be reflected in the client until the next game state update is sent from the server.