Aiming to multiplayer (and better structure)

[Max mtg]

I think it would be handy to move PlayerZones inside the player structure to allow matches between more than 2 opponents. Don't know when it's going to happen, buy formats like archenemy and commander... I think they would be welcome by players - that's a lot of work on AI and UI, I don't even imagine how much. But I believe, the one of the first steps might be attempted already now.


I think this code needs some refactoring. So I would love to pull these lines to player classes with all the dependencies... well, if noone minds. And if anyone does, this is a good place to express your opinion.

Code: Select all

    public FGameState() {
        getZoneNamesToPlayerZones().put(Constant.Zone.Graveyard + getHumanPlayer(), getHumanGraveyard());
        getZoneNamesToPlayerZones().put(Constant.Zone.Hand + getHumanPlayer(), getHumanHand());
        getZoneNamesToPlayerZones().put(Constant.Zone.Library + getHumanPlayer(), getHumanLibrary());
        getZoneNamesToPlayerZones().put(Constant.Zone.Battlefield + getHumanPlayer(), getHumanBattlefield());
        getZoneNamesToPlayerZones().put(Constant.Zone.Exile + getHumanPlayer(), getHumanExile());
        getZoneNamesToPlayerZones().put(Constant.Zone.Command + getHumanPlayer(), getHumanCommand());

        getZoneNamesToPlayerZones().put(Constant.Zone.Graveyard + getComputerPlayer(), getComputerGraveyard());
        getZoneNamesToPlayerZones().put(Constant.Zone.Hand + getComputerPlayer(), getComputerHand());
        getZoneNamesToPlayerZones().put(Constant.Zone.Library + getComputerPlayer(), getComputerLibrary());
        getZoneNamesToPlayerZones().put(Constant.Zone.Battlefield + getComputerPlayer(), getComputerBattlefield());
        getZoneNamesToPlayerZones().put(Constant.Zone.Exile + getComputerPlayer(), getComputerExile());
        getZoneNamesToPlayerZones().put(Constant.Zone.Command + getComputerPlayer(), getComputerCommand());

        getZoneNamesToPlayerZones().put(Constant.Zone.Stack + null, getStackZone());

even the comments clearly advise to do this

Code: Select all

    // These fields should be moved to the player class(es) and implementation(s), and the getters
    // should be moved there.  PMD complains of too many fields, and it is right.

    // The battlefields are different because Card.comesIntoPlay() is called when a card is added by
    // PlayerZone.add(Card).
    private PlayerZone humanBattlefield = new PlayerZone_ComesIntoPlay(Constant.Zone.Battlefield, getHumanPlayer());
    private PlayerZone humanHand = new DefaultPlayerZone(Constant.Zone.Hand, getHumanPlayer());
    private PlayerZone humanGraveyard = new DefaultPlayerZone(Constant.Zone.Graveyard, getHumanPlayer());
    private PlayerZone humanLibrary = new DefaultPlayerZone(Constant.Zone.Library, getHumanPlayer());
    private PlayerZone humanExile = new DefaultPlayerZone(Constant.Zone.Exile, getHumanPlayer());
    private PlayerZone humanCommand = new DefaultPlayerZone(Constant.Zone.Command, getHumanPlayer());


[Rob Cashwalker]

Encapsulating the Players as much as possible leads to the dark side... where visions of human vs human dance through your head.... At the very least, it opens up AI vs AI. At the very best, we should be able to do the multiplayer thing.

For Commander, the first thing we need is the CommandZone to be active and we can enable that feature immediately before any of the other stuff.

[Max mtg]

I don't even know how deep the rabbit hole is. The dark side you've metioned needs not only encapsulation, but game state syncronization over network - and this is VERY far from what we can do now.

I will also have to change some APIs to reduce public static calls to AllZone and its Util class, that is:
CardList grave = AllZoneUtil.getPlayerGraveyard(card.getController());
=>
CardList grave = card.getController().getCardsIn(Constant.Zone.Graveyard);

int amount = AllZoneUtil.getPlayerCardsInPlay(source.getController(), "Pyromancer's Swath").size();
=>
int amount = source.getController().getCardsIn(Zone.Battlefield, "Pyromancer's Swath").size();

[friarsol]

It's a very deep hole. I was about to start some work on Phasing but if you are going to synchronize all of the GetCardsInZone stuff to be moved into Player and to simplify the number of ways to call them, I can wait till after that's all moved to start adding Phasing code.

[Max mtg]

That's quite soon
There remain only some 350 compile errors (out of 1030)

[Forger]

While this is being talked about, I want to throw in the suggestion of Hotseat Mode, not as hard as real multiplayer and especially if there is one day to be 2vs2 might be fun, you and a buddy against the AI.
Still even just 1vs1 being able to replace the AI from time to time would be a nice option too.

Like the good old days, all offline!

[Max mtg]

Lots of code is written with assumption that there are only two players, one human and one computer. So multiplayer is not as near feature as we would like it to be.
Encapsulation of zones into player class is only a single step.

[Max mtg]

Commited. If any bugs (there definitelly should be some) report them here or assign to me in mantis.

[friarsol]

Thanks for organizing this Max.

Is there any reason that AllZoneUtil.getCardsIn() doesn't call Player.getCardsIn()? It would be nice if there was only one entry point into this data.

[Max mtg]

Thanks for organizing this Max.

Is there any reason that AllZoneUtil.getCardsIn() doesn't call Player.getCardsIn()? It would be nice if there was only one entry point into this data.

That took me about a day, but I like the result.

Yes, I don't want to create a Cardlist (inside Player.getCardsIn) for each zone, just prefer to add Card arrays - that are created anyway inside Zone.getCards() - into a single, once created list.

I've noticed a lot of places, where the same data is converted to array, and then to list and back to array and again to list...
just look at calls to forge.gui.GuiUtils.getChoice(String, T...) a lot of callers have a list, but have to pass array into call, which will be turned back to list later inside ListChooser. I consider such transformations waste of both memory and CPU time.

To avoid similiar problems I used direct calls in mentioned methods.

[friarsol]

Mostly I'm concerned because I wanted to be able to do Phasing, but now there isn't just one entry into this area, and I don't want to hack away at the code to get it functional.

If there is only one entry to get the data points (specifically cards in the Battlefield), then it'll be easier to filter out Phased out cards.

[Max mtg]

The main and preferred entry points are:

Code: Select all

forge.AllZoneUtil.getCardsIn(Zone) // to get all cards regardless of their controller;
forge.Player.getCardsIn(Zone) // to get all cards controlled by the player (from which you call getCardsIn)

both functions have overloads that accept a List<Zone> as parameter

[Max mtg]

Mostly I'm concerned because I wanted to be able to do Phasing, but now there isn't just one entry into this area, and I don't want to hack away at the code to get it functional.

If there is only one entry to get the data points (specifically cards in the Battlefield), then it'll be easier to filter out Phased out cards.

There never was only one entry point into cards inside a zone. You could always obtain a zone and query cards from it directly... besides a NUMBER or calls in AllUtilsZone - there was a function for each combination of zone and single player or all players.

[friarsol]

There never was only one point into cards inside a zone. You could always obtain a zone and query cards from it directly.

Well, then we should fix that too. A single entry point will make sure that every function that wants access is getting it the same way.

[Max mtg]

There never was only one point into cards inside a zone. You could always obtain a zone and query cards from it directly.

Well, then we should fix that too. A single entry point will make sure that every function that wants access is getting it the same way.

Thanks, I have more interesting things to do.

Let's better eliminate global references to HumanPlayer and ComputerPlayer. This is much more severe than possibility to access the same data in multiple ways.