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

Shortcut/Macro System #3516

Merged
merged 8 commits into from Jun 26, 2017
Merged

Shortcut/Macro System #3516

merged 8 commits into from Jun 26, 2017

Conversation

@ruler501
Copy link
Contributor

@ruler501 ruler501 commented Jun 21, 2017

Added a system for recording shorcuts and executing them a set number of times. Likely still needs some UX work and automated tests. Solves the issue raised in #2147 with something very similar to the proposed solution.

Had a lot of help from @bhomer7 designing and testing it. We tested Kitchen Finks/Viscera Seer/Vizier of Remedies, Whispers of the Muse/Paradox Engine, Vizier of Remedies/Devoted Druid, Haze of Rage/Paradox Engine and all were able to work, though it can take a while to execute large numbers of iterations.

@@ -138,6 +138,7 @@ public void testLandDoesNotLooseOtherAbilities() {
/*
NOTE: this test is currently failing due to bug in code. See issue #3072
*/
/*

This comment has been minimized.

@drmDev

drmDev Jun 21, 2017
Contributor

Why are these tests being removed? They fail currently due to bugs in code and should be preserved until we resolve the issues. If you believe the tests are inaccurate, i.e. an issue with the testing framework itself, then @ignore annotation is more appropriate than commenting out tests (otherwise we cannot find them again)

@@ -138,6 +138,7 @@ public void testLandDoesNotLooseOtherAbilities() {
/*
NOTE: this test is currently failing due to bug in code. See issue #3072
*/
/*

This comment has been minimized.

@drmDev

drmDev Jun 21, 2017
Contributor

Why are these tests being removed? They fail currently due to bugs in code and should be preserved until we resolve the issues. If you believe the tests are inaccurate, i.e. an issue with the testing framework itself, then @ignore annotation is more appropriate than commenting out tests (otherwise we cannot find them again)

Copy link
Contributor

@drmDev drmDev left a comment

Uncomment the failing tests. Use @ignore annotation if issue with Testing Framework causing the failures, or keep them as is if they are failing due to bug in code.


protected void waitForResponse(Game game) {
if(!recordingMacro && pullResponseFromQueue(game)) return;

This comment has been minimized.

@Simown

Simown Jun 21, 2017
Contributor

Not a fan of these conditions and execution on the same line. Condition at least should go on a new line indented - it's hard to see code flow especially with the return inline here.

@drmDev
drmDev approved these changes Jun 22, 2017
}

public String toString() {
String res = new String();

This comment has been minimized.

@ingmargoudt

ingmargoudt Jun 23, 2017
Contributor

why a new String() here?

@ruler501
Copy link
Contributor Author

@ruler501 ruler501 commented Jun 23, 2017

There are a couple problems remaining in this.
Card selection in pop ups doesn't work(for instance scrying to bottom with viscera seer/kitchen finks) but will work if you just let it stay on top which might be reasonable.
If someone interrupts it doesn't stop. Currently coming up with a design to prompt user if they want to continue if something unexpected happens and how to detect that.

These are probably okay as long as players are aware that if they select a card while recording and that card isn't a choice later on that the macro won't work and that the players ask if anyone will want to interrupt and if so how many iterations are okay to run.

@ruler501 ruler501 changed the title Shortcut/Macro System(Preliminary) Shortcut/Macro System Jun 24, 2017
@spjspj
Copy link
Contributor

@spjspj spjspj commented Jun 24, 2017

Out of curiosity, does this work with casting something from hand? Like say having paradox engine in play and shrieking drake? (Infinite mana, infinite storm count)

@ruler501
Copy link
Contributor Author

@ruler501 ruler501 commented Jun 24, 2017

Yes it was testing with Haze of Rage, Paradox Engine, and mana rocks to cast it with buyback. So cast, storm trigger resolves, copies resolve, paradox engine resolves, original resolves and returns to hand, repeat. As a warning it can get quite slow after 50 or so iterations just because of how the mage engine works, but storm 50 is usually enough.
It will not support picking different targets each time(such as if you had shrieking drake, panharmonicon to bounce all your creatures). Some less obvious cases where this comes up is when scrying. If you want to scry to bottom it won't work since you have to select a different card each time. If you hit done though it will work(tested with viscera seer/kitchen finks)

@LevelX2
Copy link
Contributor

@LevelX2 LevelX2 commented Jun 26, 2017

Thanks for your contribution.

@LevelX2 LevelX2 merged commit 9b31a5a into magefree:master Jun 26, 2017
@PrometeusGod
Copy link

@PrometeusGod PrometeusGod commented Jun 27, 2017

Does it do the Eggs Loop Combo?

@bhomer7
Copy link
Contributor

@bhomer7 bhomer7 commented Jun 27, 2017

Yes, it will do the Eggs Loop Combo. Tested using Pyrite Spellbomb, Faith's Reward, and Conjurer's Bauble, making mana with both Krark Clan Ironworks with Chromatic Stars or with Lotus Blooms.
As long as the combo can be done with an unchanging set of pieces of cardboard, with the same targets at each step, the combo can be recorded and played back.

@PrometeusGod
Copy link

@PrometeusGod PrometeusGod commented Jun 28, 2017

About the Eggs Combo.
What if it draws you a card or mills a card every loop?

@bhomer7
Copy link
Contributor

@bhomer7 bhomer7 commented Jun 30, 2017

The final kill process of the Eggs Loop Combo can be automated. The setup, where new cards have to be added from your library to your hand to the battlefield to the graveyard and back cannot be automated. The set of cards to be included in the loop must be completely present and selectable during the first iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants