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

Edit pile cleanup: Deck tags, first pass: folder 'a', part 1 #6489

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dracontes
Copy link
Contributor

A fairly extensive pass: 4566 changed files on my side, though overlap with previous PRs resulted in some edit redundancy.

The gist:

  • More reordering of lines around Deck*: tags
  • Propagation and clean up of Deck*: tags, notably tagging token scripts and various extra deck cards.
  • Minor edits

@@ -6,4 +6,6 @@ K:Flying
T:Mode$ Phase | Phase$ Upkeep | ValidPlayer$ You | TriggerZones$ Battlefield | Execute$ TrigEnergy | TriggerDescription$ At the beginning of your upkeep, you get {E}{E}{E} (three energy counters).
SVar:TrigEnergy:DB$ PutCounter | Defined$ You | CounterType$ ENERGY | CounterNum$ 3
A:AB$ ChangeZoneAll | Cost$ PayEnergy<8> | ChangeType$ Creature.StrictlyOther | SorcerySpeed$ True | Origin$ Battlefield | Destination$ Hand | SpellDescription$ Return all other creatures to their owners' hands. Activate only as a sorcery.
DeckHas:Ability$Counters
DeckHints:Ability$Counters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially the hints seem too generic for energy imo 🤔

I guess Ability$Proliferate is an alternative but honestly just because it's possible we really shouldn't flood every counter producer with that (especially in cases like this card which already works not too bad on its own)
Not sure what could be the right boundary - maybe only the really strong synergy ones like Simic Ascendancy?

FWIW Ability$Energy is also used on 3 cards so that's useless and should probably be removed instead (or is Energy enough of an Archetype to bother with completing it?)

Perhaps we can try and write down some rough decision guidance while working through these PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair on being more judicious about it. I have been more inclined to hint proliferate on cards that distribute counters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing keywords with issues to address, I tagged:

  • scripts with awaken with the creature type and keyword ability
  • scripts with a level up cost of 3 mana or more with proliferate

As for energy, I feel something like Ability$Counters.Energy to go alongside Ability$Counters.P1P1 could be more useful. Of course, some character other than a period can be used as a separator for this layer of classification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmn working towards some Ability groups like that would also let you use logic where a rather generic tag can be fulfilled with more specific ones based on it 🥇

DeckHints:Ability$Counters ➡️ DeckHas:Ability$Counters.Energy + DeckHas:Ability$Counters
but
DeckHints:Ability$Counters.Energy ➡️ DeckHas:Ability$Counters.Energy

I already added the basics for that in #564

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay then, I'll propagate that with (I hope) the required judiciousness.

Also, while we're on Ability groups, I've had the vague notion that Mana.Prismatic/Mana.Colors/Mana.AnyColor for the likes of Birds of Paradise could be useful to go along with Mana.Colorless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm: from looking over the code, when we're dealing with multiple kinds of counters (for instance, Phyrexian Hydra ) the correct script snippet is Ability$Counters.M1M1|Counters.Poison, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that should fit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fair bit. I think I've got the M1M1 and P1P1 counters adequately covered. Now it's charge counters, maybe time counters and a smattering of other aesthetic ones afterward. And then it's making the pass to recheck Proliferate for relevance.

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

Successfully merging this pull request may close these issues.

2 participants