Passive Effects Implementation

[zicklag]

Hey @olefasting I'm opening this discussion to talk about how to go about implementing passive effects with more control over the player.

The idea is to implement the following effects: "movement_multiplier", the turtle shell, and a "stuck_in_bubble" effect.

I started messing around with an idea where passive effects are essentially "middleware" for player events.

Passive effects are triggered on player events just like they already were, but now consume any events they are triggered on, and they return a vector of events to be inserted in it's place.

If there are multiple effects on one player, the effects output by one effect will be received by effects after it. This allows each effect, in turn, to modify, and possibly emit it's own events.

Here are the turtle shell and movement multiplier effect implementations:

turtle_shell:

The turtle shell event no longer needs to emit a damage block event, it can just delete any damage events that hit the shell.

    {
      "name": "Turtle Shell",
      "function_id": "turtle_shell",
      "activated_on": [
        "receive_damage"
      ]
    }

pub fn effect_function(
    world: &mut World,
    player_entity: Entity,
    _item_entity: Option<Entity>,
    event: PlayerEvent,
    _opts: &serde_json::Value,
) -> Vec<PlayerEvent> {

    if let PlayerEvent::ReceiveDamage { is_from_left, .. } = event {
        let player = world.get::<Player>(player_entity).unwrap();

        // If the attack is blocked, remove the damage event
        if player.is_facing_left != is_from_left {
            return vec![];
        }
    }

    // If the attack wasn't blocked, return the event, unchanged
    println!("returning event unchanged: {:?}", event);
    return vec![event];
}

movement_multiplier:

To make the movement multiplier effect work, the update_player_state system will no longer set the physics body velocity, but will instead emit UpdatePhysicsBody events. These events can then be intercepted by the movement_multiplier effect and modified.

This also shows that effects can now have options as free-form JSON, which allows us to customize effects options without hard-coding the expected input format.

        {
          "name": "Bubble Slowed",
          "function_id": "movement_multiplier",
          "function_options": {
            "x": -0.3,
            "y": 0.8
          },
          "activated_on": ["update_physics_body"],
          "duration": 3.0,
          "sprite": {
            "texture": "big_bubble",
            "autoplay_id": "ground",
            "animations": [
              {
                "id": "ground",
                "row": 0,
                "frames": 1,
                "fps": 1
              }
            ],
            "offset": {
              "x": -35,
              "y": -10
            }
          }
        }

#[derive(Serialize, Deserialize)]
struct EffectOptions {
    x: f32,
    y: f32,
}

pub fn effect_function(
    _world: &mut World,
    _player_entity: Entity,
    _item_entity: Option<Entity>,
    event: PlayerEvent,
    opts: &serde_json::Value,
) -> Vec<PlayerEvent> {
    let opts: EffectOptions = serde_json::from_value(opts.clone()).unwrap();

    if let PlayerEvent::UpdatePhysicsBody { velocity } = event {
        vec![PlayerEvent::UpdatePhysicsBody {
            velocity: velocity * Vec2::new(opts.x, opts.y),
        }]
    } else {
        vec![event]
    }
}

Does this look like it's going in the right direction?

[olefasting]

Hello! I just noticed this as I pushed send on a second mail to you. I forgot to answer the most important question you had in your original email, as I answered when I was in a bit of a rush....

As I outlined in that mail, we should probably make a few extensions to accommodate what you want to do, specifically by adding a PlayerEvent that is triggered when an Item and/or a PassiveEffect ends on a player, either by expiring or otherwise. This way, you can do changes that requires cleanup, as a temp solution to things like speed (which cause gameplay bugs if not cleaned up) and a more permanent solution for things like adding a bubble sprite (which only cause a visual bug if not cleaned up)

As I also mentioned in said mail, I will follow your recommendation from the first mail, about creating a grouping of effect functions into an effect type that is referenced in items, in stead of referencing function ids. This new effect type will, in addition to grouping effect functions into effects, hold modifiers to player attributes like jump force, move speed, and so on, so that we avoid bugs if effect end event is not triggered or some other circumstance cause cleanup not to be triggered. We could also extend this to hold things like sprites that should be overlaid the subject of the effect and other stuff that will be used commonly....

I said in the mail that we would only expose this to json, as we don't want to have trait implementations from scripts. This was an error on my part, however, as this would not be done with a trait but with a struct type. We will probably still have the functions in there, to some extent, though, as they allow reuse of common effect functions between effects...

[zicklag]

Awesome, thanks for the extra explanation. I just finished reading your Email.

PlayerAttributes was something I missed, so being able to modify that could definitely help. I think that was the biggest blocker I had, I didn't realize there was a way to stop the default fish movement from the effect.

The new approach to effects I was testing sounds like it's unnecessary, now, with the way that you describe, so I'll start seeing what I can do.

For adding sprites to effects, do you like the idea of adding a simple sprite field that will spawn when the effect is applied and despawn when the effect expires? I opened a fairly simple PR for that: #414.

I thought about adding the sprite to the character's sprite set, but then I ran into the issue when removing it, because there's no unique IDs for PassiveEffectInstance that could be used to associate the added sprite to the effect that added it. Using an Entity for the added sprite made it easy to store the Entity in the passive effect after it was spawned and have a way to remove the sprite when it was done.

I'm not sure if it'd be better to just add the effect sprite to the player's sprite set, though.

[olefasting]

Yeah, this sounds good for now. I am almost done with the changes that I outlined, but they are in the refactor branch I am currently working on, so they won't be added to the main branch until either later this weekend or during easter, however.

As for the sprite, it should probably live on player, to avoid having to keep track of two entities. Either that, or move passive effects to their own entities, entirely, and just keep their entity ref on player. A bit unsure about this one, as passive effects live on player and should not outlive the player but in some cases it might be nice to have its sprite fade out, or something similar, if the player dies or is despawned.
A bit ambivalent on this one but for now I'll just merge your PR to master and I'll make a decision on this in main.

If you want to in stead work on the branch I am working on, when I add these changes, it can be found at github.com/olefasting/fishfight under the backend_api branch. You can then submit PRs to that branch on my repo, in stead....

[zicklag]

OK, I'll just go ahead and wait on writing more for effects for now, and wait until you get those changes done.

I was going to work on the Critters implementation, too, so I can start on that and then come back to effects once you get any refactoring done. 👍

[olefasting]

Yeah, critters will not be affected by the refactor in any major way, so there you can probably just go ahead and go crazy :)

Good discussions, though. As this part of the game has seen little use and little design, outside me implementing it, this was very constructive....

[zicklag]

Sure thing! Glad I wasn't a bother. 😄

I often feel a little weird getting into other people's code because I'm still figuring it out and I'm thinking, "do they want it like this, or maybe like this...?". 🙂

[zicklag]

Another question: do you like the function_options concept and having the function_options being passed directly as an argument to the function?

       {
          "name": "Bubble Slowed",
          "function_id": "movement_multiplier",
          "function_options": {
            "x": -0.3,
            "y": 0.8
          },
      ...
      }

The thing I don't like about it is that we have to deserialize the serde_json::Value that we pass into the effect_function every time the effect function is called, but I'm not sure yet if there's a good way around it.

[olefasting]

This will probably not be needed as we do all the changes we discussed here. If options should be needed, it would rather be at runtime to supply context to the effect function. Probably not necessary, though.
If this is still needed, though, it should probably be implemented by adding a HashMap<String, GenericValueEnum>, where GenericValueEnum is an enum with only tuple variants, holding one type each, representing the various generic types. If you set serde to handle this untagged (#[serde(untagged)]) serde will allow the json values of the map to be any of the types within the enum variants.

So the following will allow the map (in json) to contain values of either String, SomeStruct, Vec2 or Rect type:

#[serde(Serialize, Deserialize)]
#[serde(untagged)]
enum GenericValueEnum {
    String(String),
    SomeStruct(SomeStruct),
    Vec2(Vec2),
    Rect(Rect),
]

Beware of ambiguities and resulting unexpected behavior when using this, however....