Renaissance preview #27

michalbe · 2018-10-02T12:07:54Z

Hello @jcbantuelle!
Again, thanks for awesome work with the best Domi implementation available (I played ~3k games this year on the platform together with my friends , and we think the official client sucks compared to yours).

I'm not sure if you're aware, but last week Rio Grande Games showed a preview of the upcoming expansion pack - Renaissance. For almost a week they added 11 new cards + Projects and Artifacts to the official client, but then they removed them. The expansion will be life in about a month.

Since I'm not a huge fan of their implementation, but I love new cards and mechanics, I implemented basic 11 cards of Renaissance (Mountain Village, Scholar, Experiment, Seer, Priest, Acting Troupe, Recruiter, Sculptor, Silk Merchant, Ducat and Villain) and a Villagers mechanics (think coin tokens for actions) in dominion-meteor.

I plan to go for Projects & Artifacts during the weekend. Hope you like them.

jcbantuelle

This work is amazing, thank you so much for contributing! I had no idea there was a new expansion coming out, it seems really fun. I'm super excited to get this work merged in, I love not having to do it all myself lol.

After reviewing the cards, I had a few minor notes of feedback based on rules interpretations that I'd like to resolve before merging. If you have any questions or disagree with any of it just let me know! Thank you again for sharing this, it's very exciting.

jcbantuelle · 2018-10-02T13:20:24Z

app/cards/renaissance/ducat.js

+        game.log.push(`&nbsp;&nbsp;<strong>${player_cards.username}</strong> gets +1 buy`)
+    }
+
+    buy_event(buyer) {


Looking at the Ducat card text, I think this is meant to be a gain event, not a buy event

jcbantuelle · 2018-10-02T13:31:07Z

app/cards/renaissance/experiment.js

+        }
+    }
+
+    buy_event(buyer) {


I think this is supposed to be a gain event instead of a buy event. Some logic will probably need to be put in place to avoid triggering an additional gain event on the gained Experiment. Off the top of my head, maybe setting some kind of flag on the turn object that can be detected for suppressing the next gain event, then unset.

app/cards/renaissance/mountain_village.js

jcbantuelle · 2018-10-02T13:34:29Z

app/cards/renaissance/mountain_village.js

+            card_drawer.draw(1)
+        }
+
+        game.turn.actions += 2


I'm not sure if this would ever matter, but from a super nitpicky standpoint I believe the actions would come before the card drawing since it's listed first on the card text

jcbantuelle · 2018-10-02T13:44:32Z

app/game/server/services/card_trasher.js

@@ -43,6 +43,15 @@ CardTrasher = class CardTrasher {
      let trash_event_processor = new TrashEventProcessor(this, trashed_card)
      trash_event_processor.process()
      this.put_card_in_trash(trashed_card)
+
+      let priests_in_play = _.filter(this.player_cards.in_play, function (card) {


From my interpretation of the Priest card text, I think the benefit applies whether the card is still in play or not, similar to how Bridge works in Intrigue. For example, if you used Throne Room on a Priest, you'd have 1 in play, but you should get the effect twice if you trash a card.

I think it would make sense to track a new field on the turn object whenever a priest is played, something like turn.priests, that starts off at 0 when a new turn object is created, and increment it once each time a priest is played.

jcbantuelle · 2018-10-02T13:48:13Z

app/cards/renaissance/priest.js

+    static trash_card(game, player_cards, selected_cards) {
+        let trashed_card = selected_cards[0]
+        if (trashed_card) {
+            let coin_cost = CostCalculator.calculate(game, trashed_card)


I don't think we need to track the cost of the trashed card, unless there's something I'm missing

jcbantuelle · 2018-10-02T13:52:45Z

app/cards/renaissance/sculptor.js

+
+        if (_.includes(_.words(selected_cards[0].top_card.types), 'treasure')) {
+            player_cards.villagers += 1
+            game.log.push(`&nbsp;&nbsp;<strong>${player_cards.username}</strong> gets +1 villager`)


I think this villager gain needs to account for possession

jcbantuelle · 2018-10-02T13:55:24Z

app/cards/renaissance/silk_merchant.js

+        }
+    }
+
+    buy_event(buyer) {


I think this triggers on gain, not buy

jcbantuelle · 2018-10-02T13:56:19Z

app/cards/renaissance/villain.js

+    play(game, player_cards) {
+        if (game.turn.possessed) {
+            possessing_player_cards = PlayerCardsModel.findOne(game._id, game.turn.possessed._id)
+            possessing_player_cards.coin_tokens += 1


I think Villain gains 2 coin tokens, not 1

jcbantuelle · 2018-10-02T13:56:36Z

app/cards/renaissance/villain.js

+            game.log.push(`&nbsp;&nbsp;<strong>${possessing_player_cards.username}</strong> takes a coin token`)
+            PlayerCardsModel.update(game._id, possessing_player_cards)
+        } else {
+            player_cards.coin_tokens += 1


This should also be a gain of 2 instead of 1

michalbe · 2018-10-02T14:07:00Z

Great comments, I'll fix all of those later today.

michalbe · 2018-10-02T20:20:57Z

@jcbantuelle fixed

jcbantuelle

Thanks for the quick turnaround on all my suggestions! I had just a couple of super minor items of note, mostly one-line tweaks. Once those are resolved, this looks good to merge in.

I really appreciate you sharing your additions with me, it's awesome having somebody to collaborate with on this project. I'm looking forward to playing with the new cards you've added, and am excited to see the rest of the set.

jcbantuelle · 2018-10-03T12:41:12Z

app/cards/renaissance/priest.js

+        if (trashed_card) {
+            let card_trasher = new CardTrasher(game, player_cards, 'hand', trashed_card.name)
+            card_trasher.trash()
+            game.turn.priests += 1


I think the priest effect occurs whether you trash a card or not. Typically when a card effect is conditional, it will say something like "Trash a card. If you do, gain x". In this case, I believe the trash mechanic is independent of the rest of turn effect, so this line can be moved up into the play method after the player chooses a card to trash.

jcbantuelle · 2018-10-03T12:42:23Z

app/cards/renaissance/priest.js

+
+    static trash_card(game, player_cards, selected_cards) {
+        let trashed_card = selected_cards[0]
+        if (trashed_card) {


I'm not too worried about this one way or another but Is it possible for this to be false? It looks like you can't hit this method unless you had a card to trash.

jcbantuelle · 2018-10-03T12:43:56Z

app/cards/renaissance/silk_merchant.js

+    }
+
+    trash_event(trasher) {
+        this.buy_or_trash_event(trasher)


I think this needs to be renamed to gain_or_trash_event

jcbantuelle · 2018-10-03T12:46:14Z

app/cards/renaissance/villain.js

+        if (game.turn.possessed) {
+            possessing_player_cards = PlayerCardsModel.findOne(game._id, game.turn.possessed._id)
+            possessing_player_cards.coin_tokens += 2
+            game.log.push(`&nbsp;&nbsp;<strong>${possessing_player_cards.username}</strong> takes a coin token`)


I believe the log message needs to be updated to reflect the 2 coin tokens that are gained now instead of one

jcbantuelle · 2018-10-03T12:46:21Z

app/cards/renaissance/villain.js

+            PlayerCardsModel.update(game._id, possessing_player_cards)
+        } else {
+            player_cards.coin_tokens += 2
+            game.log.push(`&nbsp;&nbsp;<strong>${player_cards.username}</strong> takes a coin token`)


I believe the log message needs to be updated to reflect the 2 coin tokens that are gained now instead of one

michalbe · 2018-10-06T14:25:33Z

@jcbantuelle thanks, fixed.

jcbantuelle · 2018-10-06T17:03:48Z

Merged, thanks again! Looking forward to the new expansion mechanics once you get to Projects and Artifacts. Just let me know if you have any implementation questions.

michalbe added 11 commits October 2, 2018 13:53

Renaissance - Mountain Village

8b43343

Renaissance - Scholar

3e48a00

Renaissance - Experiment

add6633

Renaissance - Seer

cdfaab2

Renaissance - Priest

18b739c

Renaissance - Villagers & Acting Troupe

38ad88e

Renaissance - Recruiter

02e0420

Renaissance - Sculptor

87b000f

Renaissance - Silk Merchant

e164b48

Renaissance - Ducat

3a9bb9a

Renaissance - Villain

d3d3318

jcbantuelle requested changes Oct 2, 2018

View reviewed changes

Renaissance - Fixes

6800b41

jcbantuelle requested changes Oct 3, 2018

View reviewed changes

Final fixes for Reneissance preview cards

5893dac

jcbantuelle merged commit a89637f into jcbantuelle:master Oct 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaissance preview #27

Renaissance preview #27

michalbe commented Oct 2, 2018

jcbantuelle left a comment

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

jcbantuelle Oct 2, 2018

michalbe commented Oct 2, 2018

michalbe commented Oct 2, 2018

jcbantuelle left a comment

jcbantuelle Oct 3, 2018

jcbantuelle Oct 3, 2018

jcbantuelle Oct 3, 2018

jcbantuelle Oct 3, 2018

jcbantuelle Oct 3, 2018

michalbe commented Oct 6, 2018

jcbantuelle commented Oct 6, 2018

Renaissance preview #27

Renaissance preview #27

Conversation

michalbe commented Oct 2, 2018

jcbantuelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbe commented Oct 2, 2018

michalbe commented Oct 2, 2018

jcbantuelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbe commented Oct 6, 2018

jcbantuelle commented Oct 6, 2018