HearthSim/SabberStone

You only use ATK.Effect when tag == GameTag.ATK, what is the reason?

Closed this issue · 5 comments

LawrenceHan commented

@darkfriend77

Would you mind to guide me a little bit?
In the file:https://github.com/HearthSim/SabberStone/blob/master/SabberStoneCore/src/Auras/AdaptiveEffect.cs#L99

You only use ATK.Effect when tag == GameTag.ATK, what is the reason?

rnilva commented

What you see is one example of bad design. Effect around GameTag.ATK is specialized for optimization purpose and separated, but I didn't fully revise AdaptiveEffect for this. Also, the if clause for ATK is derived from the observation that most of AdaptiveEffect is related to the ATK tag.

LawrenceHan commented

Thanks @rnilva
But your answer leads to more questions:

  1. In https://github.com/HearthSim/SabberStone/blob/master/SabberStoneCore/src/Enchants/GenericEffect.cs#L368
    There more SelfContainer other than ATK, in this case, is Health. Is it for the same purpose (optimization)? Because they are basically the same (use AuraEffects for fast access and modify).
  2. In https://github.com/HearthSim/SabberStone/blob/master/SabberStoneCore/src/Enchants/Enchants.cs#L165
    There are many helper constructors, are they for the same reason?
rnilva commented

In short, yes. Design principle around Effect and Attr was not fully established at the time of developing and implementations are quite messed up as you saw. I noticed that there should be an adaptor-like interface for applying effects and sort, and accessing fields using ref shows better performance. This is why these Attrs are invented. Because of the confusing design, I even re-implemented these parts for my own use of this repo but I haven't just pushed it.

  1. Effects static class is a helper for implementing Task things when we implement cards rather than for internal uses. You can easily find that these constructors are mostly used for card implementations. Personally having the class is useful for maintenance as I can easily change the implementation of Effect related things without changing card implementation scripts.
LawrenceHan commented

Thank you.
One last question (for now :D):
About your re-implementation, is it convenient for you (or safe) to share the code with me?
And please feel free to close this issue.

rnilva commented

I'd like to do, but it would take some time for some reasons but it's not because for some security protocol, just I have left this project untouched for a long time 😞