-
Notifications
You must be signed in to change notification settings - Fork 548
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
Fix ChangeZoneEffect ordering to library; Aetherspouts prompt #5798
base: master
Are you sure you want to change the base?
Conversation
Fix cards not showing up for Aetherspouts.
final CardCollection reversed = new CardCollection(tgtCards); | ||
Collections.reverse(reversed); | ||
tgtCards = reversed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tool4ever i think the extra Reverse seems a bit ugly ... can we do something about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't too happy with that part either, but I couldn't find an elegant way to reverse a CardCollectionView. If there's a better option though I'll swap it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tool4ever probably needs to look into the orderMoveToZoneList
and orderCardsByTheirOwners
functions, that for destination=Library already reverses the CardCollection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this seems more of a convenience thing to select the order opposite from the zone changes it should probably belong to GUI code (PlayerControllerHuman)
or just switch the "bottom" + "top" labels around? :p
either way then all places in effect classes could be cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a question of whether you want the output of orderMoveToZoneList
to mean "the player wants the cards in this order" versus "this is the order you should move cards one-by-one into this zone and position". Whatever is settled on, it would probably be good idea to add a Javadoc comment explaining the function's behavior when ordering cards for the top of a deck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second one makes more sense?
So all loops can be for each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know, we probably need more info like @tehdiplomat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would have a different UI element thats a Scry type. Where you can reorder a library AND send to a different zone (including bottom of the library). I know people have had similar issues with Sylvan Library, where the ordering and the place on library didn't up matching their expectations. It seems weird to reverse the array when we already have labels to say which order things will be in.
Resolves #5466.
While looking for cards to test that issue, I ran into Aetherspouts, which I think badly needs a rework for UX purposes. Currently, it prompts you for the ordering of the cards, then prompts you one card at a time whether to send the card to the top or bottom. This causes the order on top to be reversed, much like Brainsurge was doing here, since it puts cards on top of the deck one at a time.
It also didn't specify which card you were moving for each individual prompt, meaning you had to memorize the order you put them in if you want to decide case by case. I fixed this last issue by adding support for
ShowCurrentCard
to generic choices, but I think the implementation probably needs to be redone at some point. Possibly to one where you split into piles for the top and bottom first, then order them.