seastan / dragncards

Multiplayer online card game platform written in Elixir, Phoenix, and React. Drag and Drop provided by React-Beautiful-DND.
https://dragncards.com/
79 stars 41 forks source link

Passive ability offDo runs twice bug #96

Closed morvael closed 3 months ago

morvael commented 3 months ago

offDo should never run without onDo first. Similarly for onDo - it may run once and then never without offDo first.

Currently I have experienced: onDo, offDo, offDo.

seastan commented 3 months ago

I think this is currently working as intended. If onDo or offDo is triggering twice in a row, it indicates a mistake in the condition, which should check for changes.

morvael commented 3 months ago

Yeah, as we discussed it it's a difference in approach.

I expected: image While this is: image

If you don't want to change current apporach of passive, maybe consider adding "continous" ability, which is like "passive" but with in-built check to allow onDo and offDo only run alternatively.

morvael commented 3 months ago

Can be also called "while" but not be tied to inPlay/side.

seastan commented 3 months ago

Actually, it does look like it's intended to only trigger when there's been a change from the condition not being met -> being met:

  def apply_passive_rule(rule, game_old, game_new, trace) do
    onBefore = evaluate(game_old, rule["condition"], trace ++ ["game_old", Jason.encode!(rule["condition"])])
    onAfter = evaluate(game_new, rule["condition"], trace ++ ["game_new", Jason.encode!(rule["condition"])])

    cond do
      !onBefore && onAfter ->
        evaluate(game_new, rule["onDo"], trace ++ ["ON_DO"])
      onBefore && !onAfter ->
        evaluate(game_new, rule["offDo"], trace ++ ["OFF_DO"])
      true ->
        game_new
    end
  end

Do if onDo or offDo is triggering multiple times in a row there must be a bug somewhere.

morvael commented 3 months ago

Maybe the same one I get with trigger type automation, which comically is fixed by adding extra log line?

seastan commented 3 months ago

This ended up being a timing issue. Two trigger automations both listening to the same path. The first automation to respond also happened to modify the same path, causing the second one to double trigger (first the nested trigger, then the original trigger).

I think it probably best practice to avoid having an automation that can alter the value it's listening to. It's obviously easy to run into infinite loops, but even if you set up the proper guards to protect against that you can still run into strange behavior such as this.

seastan commented 3 months ago

In theory I could allow people to define a priority to their automations, but I'd have to think and discuss the best way to do that.

morvael commented 3 months ago

Even priority will not help much, though it would be helpful to set order in which triggers work (just sort all responding by their int priority value), which sometimes might be useful.

In my experience there's no other way to handle what I tried to achieve other than:

a) adding protection manually,

b) rechecking all triggers again for the same event after executing one of them.