Rulify Pokebilities and Shared Power #8254
Conversation
|
(I will try fixing these warnings tomorrow.) |
|
I would prefer not to have a I think you can just iterate I'm somewhat iffy about the |
config/formats.ts
Outdated
| @@ -626,8 +626,7 @@ export const Formats: FormatList = [ | |||
| `• <a href="https://www.smogon.com/forums/threads/3679692/">Pokébilities</a>`, | |||
| ], | |||
|
|
|||
| mod: 'pokebilities', | |||
| ruleset: ['Standard', 'Dynamax Clause'], | |||
| ruleset: ['Standard', 'Dynamax Clause', 'Pokebilities Rule'], | |||
There was a problem hiding this comment.
You don't need to add Rule after something like that, btw. I don't think it adds anything?
We already have disambiguation where pokebilities is the rule and gen8pokebilities is the format.
|
Thanks for the rapid feedback. I will look at these issues more thoroughly later on.
It seems like this could work in some cases, but in others it seems necessary to maintain a separate variable. E.g. in neutralizinggas's onEnd, it is used to restore volatiles that may have been removed. But perhaps there is a better way to manage this?
It may be able to function as 'Pokebilities' but I thought it would be clearer to distinguish that this is a rule in the name, especially because this a variable that will be directly shown to regular users through the !tier/!om command, custom tour deltas, etc and will also be referenced by some users. Both of the following statements could be meaningful in different contexts:- /challenge [Gen 8] Camomons@@@[Gen 8] Pokebilities /challenge [Gen 8] Camomons@@@Pokebilities Rule Maybe there is a better name for this, but I specifically wanted to avoid 'mod' because it confuses users about what the difference between a rule and an actual mod is. |
|
I actually think "Mod" is a good name. What don't you like about it? |
|
Hate to do this but Pokebilities is being removed as a format because its tenure as OMotM is up; this doesn't really interfere with the PR outside of merge conflicts I guess, but Pokebilities was never really intended to stay on permanently. After this rule is added Ransei and I will probably discuss re-adding it, but yeah. |
Co-authored-by: Guangcong Luo <guangcongluo@gmail.com>
Yes, this aspect shouldn't be unsafe currently - I think it just results in minor bugs like Cloud Nine not working properly in some cases. It doesn't seem like it could be optimised much more for single ability cases (and seems like it would become slower if it used volatile string checks). Should the sim/field.ts change be spun off into a mod called "multipleabilities" or something that could at least be shared (or inherited, if necessary) between these OMs?
In a vacuum, I agree that it would be a good name. However, in this context, where where "mod" already refers to a different but relevant concept (Format's mod property) both of which are relevant to end users, with the rule names directly exposed to them, I think it can create unnecessary confusion. For example, if a user wants a tour like Megas for All + Mix and Mega, we have to explain that it can't be made in any functional way because each of these formats has a different "mod" that implements how mega-evolution works in different ways, and each battle can only have one "mod", which can't changed from the one the base format uses through rule alterations. It might be difficult to understand at first, but eventually they will be able to grasp which types of tours are likely to be able to work by learning which formats use different mods. However, if they were later to see a tour like Pokebilities Shared Power Hackmons Cup with "This tournament includes: Added rules - Pokebilities Mod, Shared Power Mod" displayed, it would interfere with developing this understanding, since it would create the wrong impression that "mods" CAN be changed through rule changes, and that a battle CAN include multiple "mods" simultaneously (we have seen this kind of misunderstanding occur repeatedly with "Inverse Mod"). So we would end up having to try to explain that even though this thing is actually a rule, even though it's called a "mod"... I think if the rule is called a rule, it would be much more intuitive. It's a small thing and I can change it if there are other good style reasons for this that I don't understand, but I think it would be better to make a clear distinction here if at all possible. |
I always wondered why it did that... the events on the volatiles will never happen because their effectType is |
Hm. The main problem here is that we already use "mod" in both senses; we have "Inverse Mod" and "Sleep Clause Mod". So we wouldn't be making it more confusing. |
|
I think we could call |
You could make a rule called "Multiple Abilities" and check for its existence before doing an expensive search. |
Oh, I didn't think about the new rules having a sub-rule in common... That seems really good!
Thank-you! This seemed to disable and re-enable the effects of the abilities properly as far as I could test it (deeb730). There shouldn't be any references to m.pseudoAbilities left outside of the rules anymore.
I guess...I just didn't want to entrench that even more, especially since trying to refactor rule names after they become live would become a breaking change to tour codes. Sorry to go so off-topic, but would there be any hope of a separate PR refactoring the name of Format.mod and data/mod to something unambiguously different (maybe "variant" or "system"...?) being considered so that "mod" can refer exclusively to a type of rule?
It seems like the linter won't try to run if conflicts exist... Would it cause problems if I try to fix conflicts on my end by merging master into this branch? |
I understand the frustration, but I think this is excessive. Especially since I don't think "variant" or "system" are very clear. I think "data mod" or "dex mod" is clear enough in situations where they need to be disambiguated.
Yeah, that's the usual way to deal with merge conflicts. |
# Conflicts: # config/formats.ts # data/items.ts # data/mods/maxberries/items.ts # data/rulesets.ts
|
@Zarel I think this should have the functional changes you requested so far (m.pseudoAbilities not referenced outside of rules themselves, ruletable checks for 'multipleabilities' for performance and clarity). I can rename the rules to whatever is necessary to meet standards compliance. Does this look close to being able to move out of draft in this state? |
|
@Zarel @KrisXV I looked over a diff of the current Pokebilities mod files with their state immediately before being removed before (https://github.com/smogon/pokemon-showdown/tree/df2cb402b4b9f06fb525a6a0deb62971cd2c5782/data/mods/pokebilities), and the changes made to them should already be effective in this PR from merging previous changes to master. |
…ultiple-abilities # Conflicts: # data/mods/fusionevolutionuu/abilities.ts
|
Hi, I'm really sorry I never managed to find time to review this last time around. You seem to have a lot of random changes in SSB etc. Like changing |
|
@Zarel Thank-you!
The ability reference for Stubbornness seems like it also got fixed in another change in the meantime, but the null check was handled in a different way to before and when I resolved the merge conflict in this branch I must have redundantly included both of them, so I cleaned it up (29931d7). There are a lot of other small changes to SSB and other data files to enable ability effects to be referenced properly even when they aren't the Pokemon's real ability, but I didn't notice any other redundancies like that. |
WeWuzNidokangz
requested review from
AnnikaCodes,
monsanto and
mia-pi-git
as code owners
WeWuzNidokangz
force-pushed
the
mirror-showdown-multiple-abilities
branch
from
8ff5739
to
29931d7
Compare
# Conflicts: # config/formats.ts # data/mods/sharedpower/scripts.ts
|
AnnikaCodes, monsanto, mia-pi-git, sorry, I pushed what I thought was a merge commit from master as a regular commit to this branch, causing you to be automatically tagged for review as code owners, and I don't think I have the permission to unrequest reviews from you, please ignore. |
|
@Zarel Pokebilities was changed from OMotM to a regular OM but is still active on main, so this PR is still relevant. Merged again and updated a bit to incorporate some recent changes:- #8660: Added Pokebilities validation extensions that here work on the rule rather than just the format. #8661: Added a second randoms-based Shared Power-based format that has been converted to a rule here (the mod exists again on this branch but only to include the custom team generator for this.) |
# Conflicts: # config/formats.ts
# Conflicts: # data/abilities.ts # data/items.ts # data/moves.ts
# Conflicts: # config/formats.ts # data/mods/pokebilities/abilities.ts # data/mods/pokebilities/scripts.ts # sim/team-validator.ts
# Conflicts: # data/rulesets.ts
|
You seem to have made CI hang during the exhaustive runner tests, which is concerning. Once that's fixed, I'd like to see if @KrisXV has any review feedback before merging. |
|
Going to close this since it's been inactive for several years. Please feel free to reopen it if you're going to work on it again. |
Pokebilities is currently considered unsafe to format-stack in custom tours and challenges because when running without its mod, its battle effect callbacks create volatile pseudo-abilities that can in some cases 'leak' and may lead to stack overflows as they activate repeatedly. I previously submitted a much more focused PR (#8229) that tried to prevent this in a similar method to Shared Power, however, discussion in the comments broadened and Zarel suggested that a change which addressed this by making Pokebilities into a rule while data/ has native support for handling volatile pseudo-abilities could be considered. This is an attempt at that.
m.pseudoAbilities is equivalent to what was called m.innates in the Pokebilities mod and m.abils in the Shared Power mod. I picked this name because I thought it would be the clearest to contributors working on e.g. data/abilities without context who might not be familiar with the idea that there are formats in which Pokemon can have multiple abilities. Please offer feedback if there is a better name for this. I also ended up thinking that perhaps this variable should be moved outside of "m"(?) My expectation is that in the case that Partners in Crime (another OM in which Pokemon can have multiple abilities) becomes OMotM or LCotM, it should be able to use this variable as well.
After looking over files in data that should be affected by this change, I noticed several abilities in other mods that are likely to be currently unsafe with Pokebilities stacked into battles in the main branch aside from vanilla Mirror Armor, Trace and possibly Wandering Spirit:-
gen3/gen4: Re-implementations of Trace
ssb: Possibly "Stubbornness"
fusionevolutionuu: "Pillage", which seems to work similarly to Trace and be based on its implementation
Other changes are just to make pseudo-abilities work properly without requiring mods.
One other thing I am unsure about is how Transform should work with multiple abilities. I used the implementation from Pokebilities (erases all of own abilities; copies all of target's), which seemed logical as the general case. However, it seems as if Shared Power seemingly intentionally didn't previously work this way, only exchanging the real ability. The Shared Power thread didn't seem to offer any kind of design for how this should work that I could find (https://www.smogon.com/forums/threads/shared-power.3660877/). Does this need to be implemented differently inside the rule, and, if so, does it require a new callback method ('onAfterTransform')?