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

Improvements needed for testing suite #8973

Open
5 tasks
Alex-Vasile opened this issue May 18, 2022 · 23 comments
Open
5 tasks

Improvements needed for testing suite #8973

Alex-Vasile opened this issue May 18, 2022 · 23 comments
Labels
Developers Discussion Discussion about redesign or changes enhancement refactoring Developers topics about code and programming tracking tasks Todo list for big tasks

Comments

@Alex-Vasile
Copy link
Contributor

Alex-Vasile commented May 18, 2022

The following are based on my work on enabling checkAllCommands(). Checking off the current issues will allow for tests to be written without the need of try-catch blocks.

I'm also using it as a general tracking on for other issues.

  • ability to check if a creature can attack at all and a specific target.
  • ability to list all available targets and to check if a specific target is valid.
  • Change implementation of certain abilities and effects that disallow certain abilities and actions to NOT be used so that they can work with checkPlayableAbility. See Spells shown as castable even when they can't be #8960
  • More documentation of the constants and functions used.
  • checkPlayableAbility needs to be able to check is spells are castable when other spells are on the stack.
@Alex-Vasile Alex-Vasile added enhancement tracking tasks Todo list for big tasks refactoring Developers topics about code and programming labels May 18, 2022
@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

ability to check if a creature can attack at all and a specific target.
ability to list all available targets and to check if a specific target is valid.

It's impossible to fully implement it due MTG rules and game lifecycle. Some targets require info about previous targets (example: target 3 creatures with diff colors). It's like some conditional mana or another things with modifying effects like x2 mana on taps -- you can't find full game state until make a real action.

So it uses same technics as mana abilities with net mana methods: highlights max possible actions on playable and use real action on play. It's OK to highlights some rare non playable actions. But it's BAD to not highlights real playable.

P.S. I saw your post about finding of all affecting effects for specific card/permanent (but don't remember the issue). It's has same problem -- you can't find effects list until look ahead for next game cycle (e.g. until really apply all effects).

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

checkPlayableAbility needs to be able to check is spells are castable when other spells are on the stack.

It's must works fine with stack and casts too. Cast command equal to activate play ability. Please, make bug report on broken use cases with checkPlayableAbility (bugs with checkPlayableAbility affects AI too).

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

BTW "look ahead" is a useful feature and it supported by xmage engine (see game state and rollbacks/simulations). Can be used in many things like effects dependence like Blood Moon #4202, full ETB effects support, etc. But it can be slowly and buggy :-) At least I hold the idea that solutions to all those problems could be tried through look ahead future.

@Alex-Vasile
Copy link
Contributor Author

ability to check if a creature can attack at all and a specific target.
ability to list all available targets and to check if a specific target is valid.

It's impossible to fully implement it due MTG rules and game lifecycle. Some targets require info about previous targets (example: target 3 creatures with diff colors). It's like some conditional mana or another things with modifying effects like x2 mana on taps -- you can't find full game state until make a real action.

So it uses same technics as mana abilities with net mana methods: highlights max possible actions on playable and use real action on play. It's OK to highlights some rare non playable actions. But it's BAD to not highlights real playable.

For the testing purposes, a look ahead function might be acceptable. (simulate the game until when the action is supposed to happen, get the result, then restore the state and return the result). But I don't like the idea of look ahead implemented in the game (I will reply in more detail in one of my next messages).

P.S. I saw your post about finding of all affecting effects for specific card/permanent (but don't remember the issue). It's has same problem -- you can't find effects list until look ahead for next game cycle (e.g. until really apply all effects).

I believe that you're thinking of #9225 where I want to implement the equivalency check.

@Alex-Vasile
Copy link
Contributor Author

checkPlayableAbility needs to be able to check is spells are castable when other spells are on the stack.

It's must works fine with stack and casts too. Cast command equal to activate play ability. Please, make bug report on broken use cases with checkPlayableAbility (bugs with checkPlayableAbility affects AI too).

I don't understand this message

@Alex-Vasile
Copy link
Contributor Author

BTW "look ahead" is a useful feature and it supported by xmage engine (see game state and rollbacks/simulations). Can be used in many things like effects dependence like Blood Moon #4202, full ETB effects support, etc. But it can be slowly and buggy :-) At least I hold the idea that solutions to all those problems could be tried through look ahead future.

I believe that look ahead could be used to solve those issues, but I think that it would be a hack. And an ugly and costly one at that. I prefer trying the approach mentioned at the bottom of #4202 (comment)

Why I don't like look ahead (Aside from it being a workaround (which will add technical debt that will be even harder to fix later on if further changes are needed):

  • It will have a large performance penalty with the way that gamesstate is currently stored. To include the look ahead feature would mean that each action would be associated by a significant amount of state copying, which would have a significant impact on server performance.

We don't rollback game states so much as save copying and return to a copy. If state was instead saved in a way that only actions were stored and actions could be perfectly undone, then the performance issue would go away. (But I still wouldn't like it since it's a workaround).

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

I don't understand this message

Call showAvailableAbilities in diff use cases to see playable things in the logs. It uses same code as checkPlayableAbility. And it must works fine.

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

I prefer trying the approach mentioned at the bottom
4. Let the permanent enter the battlefield. So "enters the battlefield" replacement effects act on base of the already modified characteristics of the entering permanent.

It's look ahead feature as is (put to battlefield, apply effects, get characteristics). Current implementation is a hack (some effects manually looking for permanents in ETB list).

It will have a large performance penalty with the way that gamesstate is currently stored

Yes, it will. So it can't be used in every corner of the engine.

We don't rollback game states so much

There are many work with game states/rollbacks under the hood -- any mana or ability usage uses it.

If state was instead saved in a way that only actions were stored and actions could be perfectly undone,

Nope, it's impossible due MTG rules (one player action can go to multiple inner actions and changes, see State Based Actions as example). Moreover one simple action can change everything in game state (change effects list, effects order, affected permanents, etc). So you can't revert it. It's like a black box -- you take a current state, put it to black box and take a new game state. It's can be changed from any place of the card/ability/effect/engine.

Improved/optimized game states can be used for replays only (e.g. store visible data and changes between states cause replays don't need real game data for calculations).

@Alex-Vasile
Copy link
Contributor Author

Alex-Vasile commented Sep 29, 2022

I prefer trying the approach mentioned at the bottom
4. Let the permanent enter the battlefield. So "enters the battlefield" replacement effects act on base of the already modified characteristics of the entering permanent.

It's look ahead feature as is (put to battlefield, apply effects, get characteristics). Current implementation is a hack (some effects manually looking for permanents in ETB list).

I won't comment further since I don't know enough to say something true.

It will have a large performance penalty with the way that gamesstate is currently stored

Yes, it will. So it can't be used in every corner of the engine.

We don't rollback game states so much

There are many work with game states/rollbacks under the hood -- any mana or ability usage uses it.

Sorry, english phrasing. What I mean is "Our code doesn't rollback/unwind actions. It simply saves many states in memory and then reverts to one of those". The engine can't "undo" an action, it can only save before an action and revert to that save.

If state was instead saved in a way that only actions were stored and actions could be perfectly undone,

Nope, it's impossible due MTG rules (one player action can go to multiple inner actions and changes, see State Based Actions as example). Moreover one simple action can change everything in game state (change effects list, effects order, affected permanents, etc). So you can't revert it. It's like a black box -- you take a current state, put it to black box and take a new game state. It's can be changed from any place of the card/ability/effect/engine.

I don't see why it is impossible. The MTG rules are deterministic. I make an action and then the rules go through their list of what happens. At any point that there's a choice to be made the choice taken can be saved so that it can be undo. The same with randomness, we only need to save the current state of the random number generator.

No part of the game is a black box. All of it is completely known and will always act the same way (we control the random number generator, and even if we didn't we could just save the result).

Improved/optimized game states can be used for replays only (e.g. store visible data and changes between states cause replays don't need real game data for calculations).

I think that we could change the engine so that it handles progress forward and backwards through time by actually being able to undo actions. And the resulting engine could then also trivially handle replays since it would simply read the next action from a file rather than memory.

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

If state was instead saved in a way that only actions were stored and actions could be perfectly undone

The MTG rules are deterministic. I make an action and then the rules go through their list of what happens.

You are talking about diff things.

Yes, you can take starting game state, apply some actions list and get new game state (same result all the times -- only AI and some rare effects uses random and it can be fixed by store a random seed with game state).

BUT you can't take current game state, revert some actions list and get a starting state -- it's impossible. You can't put the toothpaste back in the tube. But xmage needs it, so it store full game states to take it back on needs.

@Alex-Vasile
Copy link
Contributor Author

Yes, you can take starting game state, apply some actions list and get new game state (same result all the times -- only AI and some rare effects uses random and it can be fixed by store a random seed with game state).

BUT you can't take current game state, revert some actions list and get a starting state -- it's impossible. You can't put the toothpaste back in the tube. But xmage needs it, so it store full game states to take it back on needs.

But again. I don't see why. (I could very well be missing something). Every action in the game (including the random ones once the randomness is fixed) is deterministic, for any current state+action there exists only a single next state.

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

Here the topic: https://stackoverflow.com/questions/29310983/what-application-game-state-delta-compression-techniques-algorithm-do-exist

  1. Answer 1 about full state (current xmage);
  2. Answer 2 about computed state (starting state + commands = new state).

@Alex-Vasile
Copy link
Contributor Author

Here the topic: https://stackoverflow.com/questions/29310983/what-application-game-state-delta-compression-techniques-algorithm-do-exist

  1. Answer 1 about full state (current xmage);
  2. Answer 2 about computed state (starting state + commands = new state).

Thanks. I'll give it a read when I get a chance, and reply here.

@awjackson
Copy link
Contributor

awjackson commented Sep 29, 2022

It's look ahead feature as is (put to battlefield, apply effects, get characteristics). Current implementation is a hack (some effects manually looking for permanents in ETB list).

I recently took a look at this, and what you are describing is not how xmage currently works. There is a commented out call to applyEffects in ZonesHandler (so maybe xmage did work the way you say in the past) but right now we don't apply continuous effects to not-yet-permanents at all. That's why Theros Gods are always affected by ETB replacement effects as if they were creatures, no matter how little devotion you have (the Blind Obedience bug). I asked on a judge forum and the God/Blind Obedience interaction was not affected by the 2017 rules change. xmage gets it wrong, but it would be wrong even if we were comparing xmage to the pre-2017 rules!

Also, "look forward in time to when the permanent will be on the battlefield" is not actually how the rules work. Consider the God/Blind Obedience example when you have exactly 4 devotion. If you simply looked forward in time, the God would ETB tapped because you will have 5 devotion once it is on the battlefield. But that's not how it works--I checked with multiple judges and the God in this case is supposed to enter untapped!

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

It uses (workaround with manually look for ETB permanents). Search code for getPermanentsEntering() and setEnterWithCounters as example.

shot_220930_010157

@awjackson
Copy link
Contributor

Entering with counters is a replacement effect. I'm talking about layered continuous effects (gain/lose types, gain/lose abilities etc.) We don't apply those at all to permanents that aren't yet on the battlefield (but we should at least for some of them, even under the old rules)

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

We don't apply those at all to permanents that aren't yet on the battlefield (but we should at least for some of them, even under the old rules)

Yes. It's the original of the "blood moon problem". Many continues effects looking for battlefield permanents only, so it can't apply it before real permanent. After changing code to looking for battlefield + etb permanents -- it's works fine. I tested it last years as one of the approach for blood moon problem (and it works) -- but I don't like that solution -- it require many changes for old and new cards (replace getbattlefield calls to another one with additional param).

@awjackson
Copy link
Contributor

We don't apply those at all to permanents that aren't yet on the battlefield (but we should at least for some of them, even under the old rules)

Yes. It's the original of the "blood moon problem". Many continues effects looking for battlefield permanents only, so it can't apply it before real permanent. After changing code to looking for battlefield + etb permanents -- it's works fine. I tested it last years as one of the approach for blood moon problem (and it works) -- but I don't like that solution -- it require many changes for old and new cards (replace getbattlefield calls to another one with additional param).

It's the only way to do it that produces the correct results in all cases (e.g. Gods when you're right at the threshold devotion value)

@awjackson
Copy link
Contributor

awjackson commented Sep 29, 2022

But again. I don't see why. (I could very well be missing something). Every action in the game (including the random ones once the randomness is fixed) is deterministic, for any current state+action there exists only a single next state.

Because determinism going forward in time doesn't mean you can go backward in time. "Current state + action = new state" might only have one answer for new state, but "current state - action = previous state" might have multiple possible answers for previous state.

Concrete example: Battlefield consists of a Silvercoat Lion with a Dark Favor on it and a Walking Corpse with a Divine Favor on it. You cast Wrath of God. The state after the action is deterministic--you now have two creature cards and two enchantment cards in the graveyard. But you can't subtract the action "cast Wrath of God" and reconstruct the previous state--the information of which Aura was on which creature has been lost as a result of the action.

@Alex-Vasile
Copy link
Contributor Author

The information of which Aura was on which creature has been lost as a result of the action.

That isn't true. On way to solve it (likely most computationally intensive way but with fewest extra states): reprocess the entire history and find the last thing that the aura was attached to. Then put that thing on the battlefield first and then the aura onto it

@JayDi85
Copy link
Member

JayDi85 commented Sep 29, 2022

There are possible workaround for "current state - action = previous state". You must store starting state + actions history. If you want to restore to prev state then you must take and recalculate all actions from starting to the needed. It's give some performance impact in long games, so it can be improved by "key game state + actions history" (key game state is a state in the middle of the game).

@Alex-Vasile
Copy link
Contributor Author

There are possible workaround for "current state - action = previous state". You must store starting state + actions history. If you want to restore to prev state then you must take and recalculate all actions from starting to the needed. It's give some performance impact in long games, so it can be improved by "key game state + actions history" (key game state is a state in the middle of the game).

Exactly. I wouldn't even consider it a workout. It's a change of phrasing/perspective. The "state" is not just the current game state, but the entire history of the game. See my reply above with same idea

@Alex-Vasile
Copy link
Contributor Author

Here the topic: https://stackoverflow.com/questions/29310983/what-application-game-state-delta-compression-techniques-algorithm-do-exist

  1. Answer 1 about full state (current xmage);
  2. Answer 2 about computed state (starting state + commands = new state).

When you say that 1 is how xmage currently works, you must be referring to the client-server communication, because that's not how XMage handles rollbacks. AFAIK from the code the whole state is copied and stored, not deltas. Even deltas would help out immensely (e.g. no more copying all those lands in commander games).

Yes 2. is close to what I am proposing for the new rollback handling in order for it to be much more efficient (it will have massive impacts on the AI).

@JayDi85 JayDi85 added the Developers Discussion Discussion about redesign or changes label May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developers Discussion Discussion about redesign or changes enhancement refactoring Developers topics about code and programming tracking tasks Todo list for big tasks
Projects
None yet
Development

No branches or pull requests

3 participants