Skip to content

Conversation

@androosss
Copy link
Contributor

Implement Grenzo's Rebuttal

Copy link
Contributor

@xenohedron xenohedron left a comment

Choose a reason for hiding this comment

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

custom effect is overimplemented and needs to be fully reworked

}
// if player is in range they choose a creature to control
if (currentPlayer != null && game.getState().getPlayersInRange(controller.getId(), game).contains(currentPlayer.getId())) {
FilterArtifactPermanent filterArtifact = new FilterArtifactPermanent(
Copy link
Contributor

Choose a reason for hiding this comment

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

try using CardTypeAssigner in a custom target class, instead of reimplementing all the target choose code multiple times

Copy link
Contributor Author

@androosss androosss Apr 15, 2025

Choose a reason for hiding this comment

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

I didn't find a way for number of choices to be modular with CardTypeAssigner(like choosing same permanent for artifact and creature) so i made loop to reuse code like it was done in most recent effect like this in [[Mythos of Snapdax]].

@androosss androosss requested a review from xenohedron April 28, 2025 08:44
Player currentPlayer = game.getPlayer(playerList.get());
boolean firstIteration = true;

for (Player nextPlayer = game.getPlayer(playerList.getNext()); !currentPlayer.getId().equals(controllerId) || firstIteration; nextPlayer = game.getPlayer(playerList.getNext())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

okay, you fulfilled the literal request, but that's not the point

What I really care about here is that the loop can be trivially analyzed to know that it will always finish. That means using a foreach / iterator. for (Object object : Set objects) basically.

Copy link
Member

Choose a reason for hiding this comment

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

It’s special code style for infinite players list. There are some cards with playerList.getNext() and getPrev(). The main logic — stop infinite iterate on get starting player.

IMG_1461
IMG_1460

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i tried to find a way to use foreach but only public methods that circular list allows are modifying list. It could be done with selecting index manually with getting index of current player and then incrementing it but that doesn't seem clean to me.

Copy link
Member

@JayDi85 JayDi85 May 4, 2025

Choose a reason for hiding this comment

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

It's by design cause there are possible effects that can change players order from left to right, see Aeon Engine.

So after getting copy by game.getState().getPlayersInRange -- you must call getNext and getPrev with game param (so it will cycle in good direction). Also you can use that code to get a static players list with workable index access -- controllerId will have index 0, next player +1, etc

List<UUID> playersList = new ArrayList<>(game.getState().getPlayersInRange(controllerId, game, true));

Copy link
Member

@JayDi85 JayDi85 May 4, 2025

Choose a reason for hiding this comment

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

List<UUID> playersList = new ArrayList<>(game.getState().getPlayersInRange(controllerId, game, true));
for (int i = 0; i < playersList.size(); i++) {
  UUID currentPlayer = playersList.get(i);
  UUID nextPlayer = playersList.get(i + 1 < playersList.size() ? i + 1 : 0);
}

@JayDi85 JayDi85 changed the title [CNS] Implements grenzo's rebuttal [CNS] Implements Grenzo's Rebuttal May 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants