Skip to content
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

Merged
merged 7 commits into from
Aug 3, 2023
Merged

Conversation

alecpm
Copy link
Contributor

@alecpm alecpm commented Aug 2, 2023

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 to always, never, or seen. 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 checking transitionsSeen.has(transition.id).

If the server side encounters an error when executing an action it will send an action_error notification with player_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:

  • Items include a remaining_uses property which starts with the item n_uses value and is altered based on the transition modify_uses property (as well as by consuming items, see note below).
  • Items can have a null position, which generally means they are being held by a player.
  • Item is now semi-mutable because item position needs to be able to change along with the new remaining_uses property.
  • Items include a limit_quantity property which determines if GridWorld.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 of 0 would be removed immediately when a player created one via a transition.
  • Transitions indicated as 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 a portable item results in picking up all its uses, and consuming such an item results in consuming a single use (reducing remaining_uses). I think this may be backwards, picking it up should create a new Item of the same type with a single remaining use held by the player while reducing the remaining_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 the Gooseberry Bush and not portable, so this doesn't really matter. Confirmed with @nataliavelez that the current approach is correct
  • Right now we're not respecting the 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.
  • Right now the server blindly accepts the 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.
  • The client side currently updates the grid to reflect the item_drop, item_pick_up, and item_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 an action_error message, which could be used by the client to reset to the original/correct state, but it currently goes unused.
  • No "predictive" action is taken on the client side for 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.
  • Needs tests.

@alecpm alecpm requested review from silviot and jessesnyder August 2, 2023 06:45
@alecpm alecpm changed the base branch from master to one-hour-one-life August 2, 2023 06:46
@alecpm alecpm force-pushed the issues/236-241-transitions branch from 46f6149 to 479ea35 Compare August 2, 2023 06:52
@alecpm
Copy link
Contributor Author

alecpm commented Aug 2, 2023

@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.

@alecpm alecpm marked this pull request as draft August 2, 2023 07:30
@alecpm alecpm changed the title WIP Transition Implementation WIP: Transition Implementation Aug 2, 2023
Copy link
Contributor

@jessesnyder jessesnyder left a 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.

dlgr/griduniverse/experiment.py Show resolved Hide resolved
dlgr/griduniverse/experiment.py Show resolved Hide resolved
dlgr/griduniverse/experiment.py Show resolved Hide resolved
dlgr/griduniverse/experiment.py Show resolved Hide resolved
dlgr/griduniverse/experiment.py Outdated Show resolved Hide resolved
dlgr/griduniverse/experiment.py Outdated Show resolved Hide resolved

transitions:
- actor_end: 5
actor_start: 2
- actor_start: 2
Copy link
Contributor

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?

Copy link
Contributor Author

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 jessesnyder self-requested a review August 2, 2023 18:41
@alecpm
Copy link
Contributor Author

alecpm commented Aug 2, 2023

@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?

@alecpm alecpm marked this pull request as ready for review August 2, 2023 19:01
@alecpm alecpm merged commit afb6e07 into one-hour-one-life Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants