Implement rooms (Bottomless Pool // Locker Room) #13786
Conversation
|
|
||
| // controlling player can be replaced so use event player now | ||
| Permanent permanent; | ||
| if (card instanceof MeldCard) { |
There was a problem hiding this comment.
Meld cards are badly tested by unit tests, so it's better to write room related tests for blinks, hand/stack/battlefield copy, etc.
Mage/src/main/java/mage/abilities/condition/common/RoomLeftHalfLockedCondition.java
Outdated
Show resolved
Hide resolved
| } else if (card instanceof SplitCard) { | ||
| // fused spells allowed (it uses main card) | ||
| if (card.getSpellAbility() != null && !card.getSpellAbility().getSpellAbilityType().equals(SpellAbilityType.SPLIT_FUSED)) { | ||
| if (card.getSpellAbility() != null && !card.getSpellAbility().getSpellAbilityType().equals(SpellAbilityType.SPLIT_FUSED) && !(card instanceof RoomCard)) { |
There was a problem hiding this comment.
Make sure all places with SPLIT_FUSED usage checked, same for SPLIT_LEFT and SPLIT_RIGHT:
|
Edge case for room names which I do not believe is handled by the current hacky workaround for name:
|
|
Opalescence - (Gatherer) (Scryfall) (EDHREC)
Diviner's Wand - (Gatherer) (Scryfall) (EDHREC)
Pithing Needle - (Gatherer) (Scryfall) (EDHREC)
|
|
(Also speaking of Opalescence - make sure that the mana value of the room reflects the mana value of the unlocked sides; a non-cast Room should have zero mana value and die with Opalesence in play, a half-unlocked room should have the mana value of that unlocked side, and a fully unlocked room should have the total mana value.) |
|
Also check other room cards — some of it can contain unique logic and can require additional changes to support it. |
I will check these specifically. I have done a lot of changes to the code with opalescence + bile blight for local testing so that works for sure. Also testing mirage mirror and similar copy effects have a lot of edge cases. I asked the judge reddit for what happens when a room loses all its abilities + opalescence + muraganda petroglyphs but, got no definite answer. |
|
Thanks to all for the extensive reviews. Apologies, I've got limited time to work on this so I expect I will proceed in bursts when I get a chance. I'll address all of these and add unit tests this week if possible. |
this is a good point, I'll probably pull in several more to this PR. |
Mage.Tests/src/test/java/org/mage/test/cards/cost/splitcards/RoomCardTest.java
Show resolved
Hide resolved
|
@xenohedron thanks for the comprehensive review. I (hopefully) addressed every comment in some form. Thanks for your patience! |
| public class RoomNameEffect extends ContinuousEffectImpl { | ||
|
|
||
| public RoomNameEffect() { | ||
| super(Duration.WhileOnBattlefield, Layer.PTChangingEffects_7, SubLayer.CharacteristicDefining_7a, |
There was a problem hiding this comment.
I had a look but sadly, it doesn't seem that clones work properly if I set the layer down. I suppose this is why I originally did this layer, as I did start with it in the lowest layer (also, I guess a characteristic defining ability is somewhat similar to name, in that it works everywhere).
I did add a test for a name altering clone, however, and it works fine. So probably it's alright?
Well if this is the way to get the results we need, then that's what it shall be for now
Mage/src/main/java/mage/abilities/common/UnlockThisDoorTriggeredAbility.java
Outdated
Show resolved
Hide resolved
| public class RoomNameEffect extends ContinuousEffectImpl { | ||
|
|
||
| public RoomNameEffect() { | ||
| super(Duration.WhileOnBattlefield, Layer.PTChangingEffects_7, SubLayer.CharacteristicDefining_7a, |
There was a problem hiding this comment.
Re: single effect - I did this mostly for readability as both are a bit wordy. If that's not a problem - I'll put them together - what do you think? I could do a couple sub methods perhaps. Neither of them really belong in the effects system but as discussed above I think this is a functional approach.
I think it should be a single RoomCharacteristicsEffect.
(It's never going to be correct to have one effect without the other, and some of the code is common.)
|
@oscscull need starting topic update with actual status: 1 - detail changes from that PR (implemented rooms, added supported multiple names, improved zone changes, etc). 2 - add x2 TODO lists: one for your current implementation (if you plan to add more changes) and one for next PRs or changes (what’s miss and must be added/improved after that PR by you or another devs). 3 - add info about fully or partly supported rules (if you added all rooms related rules or there are something miss). 4 - add info about tests (is it cover all new rules and rooms logic/combos or need some improves later). |
|
@xenohedron addressed these! |
|
Adding some screenshots for context |
6 participants
Part of #12534
TODO in this PR
TODO in later PRs
Supported Rules:
As far as I am aware, all room specific rules are covered in this PR. The one exception being (as above), that I don't think the name and mana values are set in the right layer. Relevent rule below
709.5. Some split cards are permanent cards with a single shared type line. A shared type line on such an object represents two static abilities that function on the battlefield. These are “As long as this permanent doesn’t have the ‘left half unlocked’ designation, it doesn’t have the name, mana cost, or rules text of this object’s left half” and “As long as this permanent doesn’t have the ‘right half unlocked’ designation, it doesn’t have the name, mana cost, or rules text of this object’s right half.” These abilities, as well as which half of that permanent a characteristic is in, are part of that object’s copiable values.
Testing:
I've added 16 total tests so far.
I've covered all situations I could think of off the top of my head, with some very edge cases. With that said, rooms have a lot of associated rules, so there's potentially some weird cases I haven't anticipated.