Lstebner/achievement i am rich #77

lstebner · 2021-05-29T01:06:20Z

This PR implements 3 new achievements in a series referred to as "I am Rich". this closes issue #73 , and can be tested here https://farmhand-git-lstebner-achievement-i-am-rich-jeremyckahn.vercel.app/

this PR also adds a few new utils which were used to implement this feature

percentageString util to convert a floating percent to a string value
getSalePriceMultiplier looks at achievements and returns a multiplier value

completed achievements:

incomplete achievements:

increased price in sell view:

unit tests:

vercel · 2021-05-29T01:06:22Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/jeremyckahn/farmhand/CPA11Rgg7gTdCo6gWejukcUwBm3T
✅ Preview: https://farmhand-git-lstebner-achievement-i-am-rich-jeremyckahn.vercel.app

jeremyckahn

This is off to a great start! I added comments throughout, but let me know if anything is unclear and we can discuss further.

src/data/achievements.js

src/data/achievements.test.js

src/reducers.js

jeremyckahn · 2021-05-29T16:14:16Z

src/reducers.js

+    salePriceMultiplier = 1.1
+  } else if (state.completedAchievements['i-am-rich-3']) {
+    salePriceMultiplier = 1.05
+  }


This comment is aimed at your question in the getItemCurrentValue util, but makes more sense to answer here.

should valueAdjustments have a "global" adjustment or something for any permanent global increases?

We should avoid using valueAdjustments to calculate sale bonuses, as that object is intended to only define market fluctuations. With that said, my intuition is to move this salePriceMultiplier calculation to a helper function in src/utils.js and use that helper both here and in getItemCurrentValue. What do you think of that approach?

i had a feeling valueAdjustments was something market specific. can we update the variable name? (is that easy? to like, marketValueAdjustments?). or add some documentation around it. i didn't see any explanation for the variable anywhere. whatever you prefer @jeremyckahn . i can also do the work if you let me know which you prefer.

onto the util...that makes sense to me! that will scale nicely later on.

lots of unit tests, and a util or two later i think i got there!

i had a feeling valueAdjustments was something market specific. can we update the variable name? (is that easy? to like, marketValueAdjustments?). or add some documentation around it. i didn't see any explanation for the variable anywhere. whatever you prefer @jeremyckahn . i can also do the work if you let me know which you prefer.

Whoops, I saw this after the fact! Sorry about that. I've effectively addressed this here: https://github.com/jeremyckahn/farmhand/pull/77/files#r641988878

tl;dr: Yes, we should definitely rename valueAdjustments to something like marketValueAdjustments for the relative value calculations, but we'll also need to having some sort of "grand total" value that is presented to the user as well.

src/data/achievements.test.js

jeremyckahn

Awesome progress so far! There's a few issues that need to be addressed, but I'm really excited to see how well this is coming along!

jeremyckahn · 2021-05-29T20:41:58Z

src/constants.js

+  1: 0.05,
+  2: 0.1,
+  3: 0.25,
+}


I definitely prefer bundling these into a collection vs. multiple const variables, it fits well with the rest of this PR! In this case, I think it would be more straightforward to use an Array here, as the keys are just number indices and it'd be more natural for the code that works with I_AM_RICH_BONUSES to be zero-based. What do you think about changing this Object to being an Array?

jeremyckahn · 2021-05-29T20:43:06Z

src/data/achievements.js

+    )} bonus.`,
+    condition: state => state.revenue >= goal,
+    reward: state => state,
+  }))(),


These are looking much better! Nice work! 👏

src/data/achievements.test.js

jeremyckahn · 2021-05-29T20:55:03Z

src/data/achievements.js

+    description: `Earn ${dollarString(goal)}`,
+    rewardDescription: `All sales receive a ${percentageString(
+      I_AM_RICH_BONUSES[1],
+      '%'


It doesn't look like the '%' parameter is needed here. Was that left over from an earlier iteration?

yup! a formatNumber attempt hah. thanks for noticing that

src/data/achievements.test.js

src/utils.js

jeremyckahn · 2021-05-29T22:04:57Z

src/components/Item/Item.js

@@ -145,6 +146,8 @@ export const Item = ({

  const avatar = <img {...{ src: items[id] }} alt={name} />

+  adjustedValue *= getSalePriceMultiplier(completedAchievements)


Rather than mutating adjustedValue here, I think we're going to need to add an additional number to represent adjustedValue * getSalePriceMultiplier(completedAchievements). The reason for this is that adjustedValue, at least for non-Shop items, specifically defines the value of an item adjusted for current market values. Modifying it here will break the calculations at these points deeper in the component:

farmhand/src/components/Item/Item.js

Lines 204 to 206 in 26d249b

<PurchaseValueIndicator

{...{ id, value: adjustedValue, valueAdjustments }}

/>

farmhand/src/components/Item/Item.js

Lines 233 to 235 in 26d249b

<SellValueIndicator

{...{ id, value: adjustedValue, valueAdjustments }}

/>

This is a bit confusing because #73 makes the semantics of this variable a bit unclear. It may make sense to rename adjustedValue to something like marketAdjustedValue, since the current name is too vague now. I think "adjustedValue" is good for representing "the final number that is presented to the user," in case you need some help with the variable naming here.

i see now, thanks for pointing that out. i think it actually makes the most sense to add a sellPrice variable and only adjust the "Sell price" field. i would like to see adjustedValue updated as you said, but there are many other similar variables that i would also want to update like previousDatAdjustedValue and historicalValueAdjustments so I think it's best to leave those all for now. how does that sound?

Oh yeah, those variables are functionally affected by this feature at all, and changing their name is actually quite a bit more involved because they are part of the persisted game data. They're named slightly more vaguely than I'd like right now, but it's not worth changing them in this PR.

jeremyckahn · 2021-05-29T22:10:01Z

src/utils.js

@@ -192,6 +193,12 @@ export const dollarString = number => formatNumber(number, '$0,0')
 */
 export const integerString = number => formatNumber(number, '0,0')

+/**
+ * @param {number} a float


Per JSDoc syntax, this needs to reference the name of the variable. So, this will need to be something like:

* @param {number} number A float

oops, number/number! good catch

src/utils.js

lstebner · 2021-05-30T03:24:41Z

@jeremyckahn pushed another round of updates for all your latest comments. thanks for the detailed review!

jeremyckahn

Thanks for the latest iteration! This is looking great and I think we're almost there. I saw a few mostly minor regression issues to address, but I think the hard problems are solved at this point!

src/components/Item/Item.js

src/constants.js

src/data/achievements.js

src/utils.js

jeremyckahn · 2021-05-31T16:35:52Z

The code looks good to me, but I just tried to test out the new achievement rewards and they don't seem to factor in the sales bonus. 😕

Here's where I tested out the new feature: https://farmhand-git-lstebner-achievement-i-am-rich-jeremyckahn.vercel.app/

Here's the save file I used to test this out: https://gist.github.com/jeremyckahn/4dc3bd35f6e2db3c47b24dcc62a5db27

(You can save that JSON data to a local file and then import it into the game for testing.)

You'll see there that Carrots have a boosted value of $26.25, but once sold, the player only gets the non-boosted value of $25. I'll take a look to see what part of the code might need to be revisited to fix this too.

jeremyckahn · 2021-05-31T16:52:13Z

The code looks good to me, but I just tried to test out the new achievement rewards and they don't seem to factor in the sales bonus. 😕

Here's where I tested out the new feature: https://farmhand-git-lstebner-achievement-i-am-rich-jeremyckahn.vercel.app/

Here's the save file I used to test this out: https://gist.github.com/jeremyckahn/4dc3bd35f6e2db3c47b24dcc62a5db27

(You can save that JSON data to a local file and then import it into the game for testing.)

You'll see there that Carrots have a boosted value of $26.25, but once sold, the player only gets the non-boosted value of $25. I'll take a look to see what part of the code might need to be revisited to fix this too.

Based on some quick experimentation, it looks like this change might be all that's needed here:

diff --git a/src/reducers.js b/src/reducers.js
index a10d58a..617bc03 100644
--- a/src/reducers.js
+++ b/src/reducers.js
@@ -1142,12 +1142,11 @@ export const sellItem = (state, { id }, howMany = 1) => {
           castToMoney(adjustedItemValue * LOAN_GARNISHMENT_RATE)
         )
       : 0
-    const garnishedProfit = adjustedItemValue - loanGarnishment
+    const garnishedProfit =
+      adjustedItemValue * getSalePriceMultiplier(completedAchievements) -
+      loanGarnishment
     loanBalance = moneyTotal(loanBalance, -loanGarnishment)
-    saleValue = moneyTotal(
-      saleValue * getSalePriceMultiplier(completedAchievements),
-      garnishedProfit
-    )
+    saleValue = moneyTotal(saleValue, garnishedProfit)
   }
 
   if (saleIsGarnished) {

The tests pass with this, and I'm seeing the correct behavior in-game with it!

this adds a series of 3 new achievements based on farm revenue. the reward is a permanent increase to all sale prices. more info about achievements can be found in the wiki https://github.com/jeremyckahn/farmhand/wiki/Achievements

jeremyckahn

I've QA'ed and verified this on https://farmhand-git-lstebner-achievement-i-am-rich-jeremyckahn.vercel.app/, and everything seems to be working perfectly! Thank you @lstebner for sticking with this and bringing it through to completion. This feature adds some much-needed late-game content and lays the groundwork for more interesting mechanics down the line. Awesome work! LGTM! 🚀

lstebner mentioned this pull request May 29, 2021

Achievement: I am rich #73

Closed

jeremyckahn requested changes May 29, 2021

View reviewed changes

vercel bot deployed to Preview May 29, 2021 19:14 View deployment

vercel bot deployed to Preview May 29, 2021 19:24 View deployment

vercel bot deployed to Preview May 29, 2021 19:34 View deployment

vercel bot deployed to Preview May 29, 2021 20:34 View deployment

vercel bot deployed to Preview May 29, 2021 20:53 View deployment

lstebner commented May 29, 2021

View reviewed changes

src/data/achievements.test.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview May 29, 2021 20:59 View deployment

vercel bot deployed to Preview May 29, 2021 21:30 View deployment

jeremyckahn requested changes May 29, 2021

View reviewed changes

vercel bot deployed to Preview May 30, 2021 03:24 View deployment

jeremyckahn self-requested a review May 30, 2021 15:55

jeremyckahn requested changes May 30, 2021

View reviewed changes

src/components/Item/Item.js Show resolved Hide resolved

src/constants.js Show resolved Hide resolved

src/data/achievements.js Outdated Show resolved Hide resolved

src/utils.js Outdated Show resolved Hide resolved

src/utils.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview May 30, 2021 16:27 View deployment

lstebner force-pushed the lstebner/achievement-i-am-rich branch from 03e29fb to de7221b Compare May 30, 2021 16:32

vercel bot deployed to Preview May 30, 2021 16:32 View deployment

lstebner marked this pull request as ready for review May 30, 2021 16:33

vercel bot deployed to Preview May 30, 2021 16:45 View deployment

jeremyckahn self-requested a review May 31, 2021 15:59

vercel bot deployed to Preview May 31, 2021 17:00 View deployment

feat: [closes #73] new achievement: I am Rich

72f2301

this adds a series of 3 new achievements based on farm revenue. the reward is a permanent increase to all sale prices. more info about achievements can be found in the wiki https://github.com/jeremyckahn/farmhand/wiki/Achievements

lstebner force-pushed the lstebner/achievement-i-am-rich branch from a17d66f to 72f2301 Compare May 31, 2021 17:21

vercel bot deployed to Preview May 31, 2021 17:21 View deployment

jeremyckahn approved these changes May 31, 2021

View reviewed changes

lstebner merged commit 6bed24f into develop May 31, 2021

lstebner deleted the lstebner/achievement-i-am-rich branch May 31, 2021 17:30

jeremyckahn mentioned this pull request Jun 29, 2021

"I am Rich" achievements affect resale items #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lstebner/achievement i am rich #77

Lstebner/achievement i am rich #77

lstebner commented May 29, 2021 •

edited

vercel bot commented May 29, 2021 •

edited

jeremyckahn left a comment

jeremyckahn May 29, 2021

lstebner May 29, 2021

lstebner May 29, 2021

jeremyckahn May 29, 2021

jeremyckahn left a comment

jeremyckahn May 29, 2021

jeremyckahn May 29, 2021

jeremyckahn May 29, 2021

lstebner May 30, 2021

jeremyckahn May 29, 2021

lstebner May 30, 2021

jeremyckahn May 30, 2021

jeremyckahn May 29, 2021

lstebner May 30, 2021

lstebner commented May 30, 2021 •

edited

jeremyckahn left a comment

jeremyckahn commented May 31, 2021

jeremyckahn commented May 31, 2021

jeremyckahn left a comment

+: 0.05,
+: 0.1,
+: 0.25,
+              }

		@@ -145,6 +146,8 @@ export const Item = ({

		const avatar = <img {...{ src: items[id] }} alt={name} />

		adjustedValue *= getSalePriceMultiplier(completedAchievements)

	<PurchaseValueIndicator
	{...{ id, value: adjustedValue, valueAdjustments }}
	/>

	<SellValueIndicator
	{...{ id, value: adjustedValue, valueAdjustments }}
	/>

Lstebner/achievement i am rich #77

Lstebner/achievement i am rich #77

Conversation

lstebner commented May 29, 2021 • edited

vercel bot commented May 29, 2021 • edited

jeremyckahn 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

jeremyckahn 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

lstebner commented May 30, 2021 • edited

jeremyckahn left a comment

Choose a reason for hiding this comment

jeremyckahn commented May 31, 2021

jeremyckahn commented May 31, 2021

jeremyckahn left a comment

Choose a reason for hiding this comment

lstebner commented May 29, 2021 •

edited

vercel bot commented May 29, 2021 •

edited

lstebner commented May 30, 2021 •

edited