-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: first pieces for The Forest #475
Conversation
- add feature flag - add new view and insert into navigation - add purchaseForest reducer and ui event - add some strings for forest notifications - add some constants for forest sizes - add purchasable upgrade to shop
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/components/Farmhand/Farmhand.js
Outdated
@@ -214,6 +215,7 @@ const applyPriceEvents = (valueAdjustments, priceCrashes, priceSurges) => { | |||
* @property {number} experience | |||
* @property {string} farmName | |||
* @property {(?farmhand.plotContent)[][]} field | |||
* @property {(?farmhand.plotContent)[][]} forest |
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 think it makes sense for the forest to use the same model as the field, but am curious what you think @jeremyckahn
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.
To answer that, I think we need to determine if the forest will function like the Field does. In #474, you mentioned the idea of mushrooms (which might imply other forageable items). I think this is a great idea for a future iteration, but we should account for it in these early stages.
I think forest
could be a nested array similar to field
, but we'll have to give some thought to what farmhand.forestPlotContent
(or whatever we call it) might look like. In my comment, I suggested that there should be fewer tree slots than we have for Field plots. I think this would create some interesting challenges for the player, but it also gives us room to do something with the space between the trees. It could work something like this:
/**
* @typedef {Object} farmhand.forestForageable
* @property {'mushroom' | 'acorn' | ...} forageableId
*/
/**
* @typedef {Object} farmhand.forestPlotContent
* @property {string} itemId A tree seed
* @property {number} daysOld
* @property {[
* [?farmhand.forestForageable, ?farmhand.forestForageable, ?farmhand.forestForageable],
* [?farmhand.forestForageable, null, ?farmhand.forestForageable],
* [?farmhand.forestForageable, ?farmhand.forestForageable, ?farmhand.forestForageable]
* ]} forestForageables
*/
forestForageables
is a little hard to read, but the idea is that it's a 3x3 grid of farmhand.forestForageable
s but with null
in position (1, 1)
.
This is the not the only way to model this data, but this is how I'm thinking about it. What do you think @lstebner? Hopefully I didn't confuse things more... 😅
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.
that's a really interesting, and pretty clever, approach. i was imagining the tree plots and the foragable plots would just be two separate lists, but we would render them in such a way that they appeared intertwined with each other. i'm not really feeling like one approach is any better than the other here, though it feels simpler to me to manage 2 separate lists and just deal with rendering positioning for making it look how we want.
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.
We could choose to keep the data simpler (by not having null
in the middle of the sub-nested array) and use logic to enforce a valid structure, but to me that seems a bit more complex to manage. Using a complex data structure would also enable the type checker to enforce type safety. Either way, I think it'll take some experimentation to see what works best here.
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 think the key difference between our thoughts of the forest is you're imagining a pretty dense forest grid where potentially every space has something in it, like this: (where F
= foragable and T
= tree)
| F | F | F | F | F | F | F | F | F |
| F | T | F | F | T | F | F | T | F |
| F | F | F | F | F | F | F | F | F |
and i'm thinking of something that's a bit more spacious, and uneven, like this: (X
= blank space)
| F | X | X | F | F | X | X | X | F |
| X | T | X | F | X | F | X | T | X |
| X | F | X | X | F | X | F | X | F |
so i'm seeing it as either a grid where certain cells never have anything (maybe decor), some are valid foragable spots, and some are valid tree spots. or.. i'm just a simple set of two lists (treePlots
and foragablePlots
) which can each hold a specific typed object, and the rendering ends up positioning them how we want rather than enforcing "only __ can go in cell #" for each cell.
the sub-nested array you've proposed seems a little bit harder to manage to me because if you want to get at the foragable cell (3, 3) you would have to know it's actually a sub-cell of (0, 0), i think?
one more thing i want to mention is that although the trees have been drawn in 36x72 sprites, i was imagining the cells be 36x36 so that the trees can overlap 2 cells visually, which i think potentially makes things more interesting to look at, but it does mean there would be no point in spawning a foragable in the cell just above a tree cell because it wouldn't be noticeable. we could definitely make the cells 36x72 and avoid this, but we also might run out of vertical space a lot faster if we do that.
i still think all the ways we're talking about this data are valid, but hopefully that little illustration is helpful to compare how i think we're each thinking 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.
I see what you're saying. Given the design you're describing, I think it makes sense to go with a simpler data structure (just a nested list) to represent the entirety of the forest. Perhaps something like (farmhand.plantedTree | farmhand.forestForageable | null)[][]
? I think we'll want to have some game rules around how closely things can be placed (either by the player planting saplings or random foragable spawn locations). That's something that can be figured out in a later iteration.
Good thinking!
) | ||
}) | ||
|
||
describe('forest expansion', () => { |
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.
this is a copypasta of the purchaseField tests. since the same data model is being used i left the testCrop
uses, but it does read kinda funny... maybe it doesn't matter for now and we update it to use some actual saplings once those items are created. looking for some feedback here though
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 think it's worth nailing down the data model for the Forest feature before we build too much of it out. I proposed how that might work in my other comment. In any case, once we have that figured out I think it'll inform how these tests should work.
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.
Great start here @lstebner! There are a few open questions brought about by this PR, but that's to be expected for the start of a big feature like this.
it might be cool to have the forest unlock at a certain level instead of requiring purchase for the initial forest, so this is why i put in the INITIAL forest size constants. then once unlocked the upgrades can be purchased to expand it. If we go this route I think it should be a decently high level, like maybe 20. I think that would also help justify higher prices for the expansions.
I think that's a good idea. Let's go with that, especially since you've already laid the groundwork for it here. In this case, we should also hide the purchasable forest expansions until the Forest has been unlocked by experience.
src/components/Farmhand/Farmhand.js
Outdated
@@ -214,6 +215,7 @@ const applyPriceEvents = (valueAdjustments, priceCrashes, priceSurges) => { | |||
* @property {number} experience | |||
* @property {string} farmName | |||
* @property {(?farmhand.plotContent)[][]} field | |||
* @property {(?farmhand.plotContent)[][]} forest |
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.
To answer that, I think we need to determine if the forest will function like the Field does. In #474, you mentioned the idea of mushrooms (which might imply other forageable items). I think this is a great idea for a future iteration, but we should account for it in these early stages.
I think forest
could be a nested array similar to field
, but we'll have to give some thought to what farmhand.forestPlotContent
(or whatever we call it) might look like. In my comment, I suggested that there should be fewer tree slots than we have for Field plots. I think this would create some interesting challenges for the player, but it also gives us room to do something with the space between the trees. It could work something like this:
/**
* @typedef {Object} farmhand.forestForageable
* @property {'mushroom' | 'acorn' | ...} forageableId
*/
/**
* @typedef {Object} farmhand.forestPlotContent
* @property {string} itemId A tree seed
* @property {number} daysOld
* @property {[
* [?farmhand.forestForageable, ?farmhand.forestForageable, ?farmhand.forestForageable],
* [?farmhand.forestForageable, null, ?farmhand.forestForageable],
* [?farmhand.forestForageable, ?farmhand.forestForageable, ?farmhand.forestForageable]
* ]} forestForageables
*/
forestForageables
is a little hard to read, but the idea is that it's a 3x3 grid of farmhand.forestForageable
s but with null
in position (1, 1)
.
This is the not the only way to model this data, but this is how I'm thinking about it. What do you think @lstebner? Hopefully I didn't confuse things more... 😅
src/components/Forest/Forest.js
Outdated
export default function Consumer(props) { | ||
return ( | ||
<FarmhandContext.Consumer> | ||
{({ gameState, handlers }) => ( | ||
<Forest {...{ ...gameState, ...handlers, ...props }} /> | ||
)} | ||
</FarmhandContext.Consumer> | ||
) | ||
} |
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.
This pattern will certainly work, and it's what most of the components in the codebase still use. Recently, I've been getting into the habit of using the useContext
hook as it's a bit simpler and more modern.
Here's an example of it in use:
farmhand/src/components/SettingsView/RandomSeedInput.js
Lines 10 to 13 in 37f60ea
export const RandomSeedInput = ({ search = window.location.search }) => { | |
const { | |
handlers: { handleRNGSeedChange }, | |
} = useContext(FarmhandContext) |
Notice that it doesn't have a Consumer
component at the bottom of the file that most of the older components have.
Here's how it can be set up for testing:
farmhand/src/components/SettingsView/RandomSeedInput.test.js
Lines 11 to 17 in 37f60ea
const MockRandomSeedInput = props => ( | |
<FarmhandContext.Provider | |
value={{ handlers: { handleRNGSeedChange: mockHandleRNGSeedChange } }} | |
> | |
<RandomSeedInput {...props} /> | |
</FarmhandContext.Provider> | |
) |
Both approaches are valid, I just want to call out this alternative in case you like it better. 🙂
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.
ahh yes, good call on moving to useContext
. i actually hadn't noticed that in this repo yet, but i'm all for moving to it!
) | ||
}) | ||
|
||
describe('forest expansion', () => { |
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 think it's worth nailing down the data model for the Forest feature before we build too much of it out. I proposed how that might work in my other comment. In any case, once we have that figured out I think it'll inform how these tests should work.
thanks for the review @jeremyckahn . i'll get the |
Sounds good! My availability during the week is pretty limited now, so it might be easiest to stick with async communication via comments on this PR. I think we're set with a nested array to represent the Forest, but it's what the nested array contains that needs to be figured out. However, that's not something that necessarily needs to be set in stone now. We should minimally have an interface for dealing the data that's flexible enough to allow us to iterate on the model before we release the feature. |
@jeremyckahn just pushed some updates including some new typedefs as we discussed, and moving the forest to unlock from experience rather than a purchase. i used a simple constant for this instead of |
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.
Thanks for making the model updates @lstebner! I'd like revisit the decision to determine forest availability with just a constant. Given the size and complexity of this code base, it seems worth adhering to conventions to keep things manageable unless it's prohibitively difficult to do so.
This is coming along really well!
@@ -215,7 +218,7 @@ const applyPriceEvents = (valueAdjustments, priceCrashes, priceSurges) => { | |||
* @property {number} experience | |||
* @property {string} farmName | |||
* @property {(?farmhand.plotContent)[][]} field | |||
* @property {(?farmhand.plotContent)[][]} forest | |||
* @property {(farmhand.plantedTree | farmhand.forestForageable | null)[][]} forest |
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.
Awesome 😎
src/components/Farmhand/Farmhand.js
Outdated
get isForestUnlocked() { | ||
return levelAchieved(this.state.experience) >= UNLOCK_FOREST_LEVEL | ||
} |
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.
While this is simple and straightforward, it might be better on balance to have this determined in src/data/levels.js
. The centralized, linear nature of that file is very helpful for looking up level unlock rewards. It serves as self-documenting code, and I think it's worth sticking with that pattern unless it's infeasible to do so.
I'm thinking that we could have something like this in levels.js
:
levels[15] = {
unlocksStageFocusType: stageFocusType.FOREST,
}
And then we'd need to update getLevelEntitlements
to handle unlocksStageFocusType
similarly to unlocksTool
:
farmhand/src/utils/getLevelEntitlements.js
Lines 36 to 38 in 4c39401
if (unlocksTool) { | |
acc.tools[unlocksTool] = true | |
} |
What do you think @lstebner?
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.
yeah, that makes sense to me. i was on the fence, but i'll go ahead and make the change!
src/components/Forest/Forest.js
Outdated
export default function Consumer(props) { | ||
return ( | ||
<FarmhandContext.Consumer> | ||
{({ gameState, handlers }) => ( | ||
<Forest {...{ ...gameState, ...handlers, ...props }} /> | ||
)} | ||
</FarmhandContext.Consumer> | ||
) | ||
} |
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.
❤️
- remove constant used for forest unlock level - add unlocksStageFocusType level up processing capability - remove processLevelUp from sellItem because addExperience does it, but i'm not sure if this is a good idea or not admittedly
- fix level up + forest unlocked notification - update shovel unlocked notification to remove TODO
another round of updates is in @jeremyckahn, but I do have another question for you about the changes I just made. i added a new persisted state key called i also went ahead and made the update to |
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.
We're getting there!
i added a new persisted state key called
stagesUnlocked
, but the only thing I put in here is the forest since it's the only stage that is unlockable through experience. i'm not sure if this is the best name, or the best way of going about this, so I wanted your opinion.
I think it's worth changing this into a computed value. Doing so will avoid adding data to game save files and thus de-risk and simplify future maintenance.
I had a few other notes, but this is really coming together. Thanks for making code improvements as you go! 🙂
src/components/Farmhand/Farmhand.js
Outdated
@@ -420,7 +421,7 @@ export default class Farmhand extends FarmhandReducers { | |||
} | |||
|
|||
get isForestUnlocked() { | |||
return levelAchieved(this.state.experience) >= UNLOCK_FOREST_LEVEL | |||
return this.state.stagesUnlocked[stageFocusType.FOREST] |
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 seems like we might be able to avoid adding the stagesUnlocked
state by computing the level entitlements. I'm thinking something like this:
diff --git a/src/components/Farmhand/Farmhand.js b/src/components/Farmhand/Farmhand.js
index 5859e00a..dace1f6e 100644
--- a/src/components/Farmhand/Farmhand.js
+++ b/src/components/Farmhand/Farmhand.js
@@ -421,7 +421,7 @@ export default class Farmhand extends FarmhandReducers {
}
get isForestUnlocked() {
- return this.state.stagesUnlocked[stageFocusType.FOREST]
+ return this.levelEntitlements.stageFocusType === true
}
/**
diff --git a/src/index.js b/src/index.js
index a969917e..ff12417d 100644
--- a/src/index.js
+++ b/src/index.js
@@ -236,6 +236,7 @@
* @property {number} sprinklerRange
* @property {Object.<string, boolean>} items
* @property {Object.<string, boolean>} tools
+ * @property {Object.<string, boolean>} stageFocusType
*/
/**
diff --git a/src/utils/getLevelEntitlements.js b/src/utils/getLevelEntitlements.js
index 15a49daa..b1152d4d 100644
--- a/src/utils/getLevelEntitlements.js
+++ b/src/utils/getLevelEntitlements.js
@@ -20,11 +20,12 @@ export const getLevelEntitlements = memoize(
sprinklerRange: INITIAL_SPRINKLER_RANGE,
items: {},
tools: {},
+ stageFocusType: {},
}
// Assumes that levels is sorted by id.
levels.find(
- ({ unlocksShopItem, unlocksTool, id, increasesSprinklerRange }) => {
+ ({ unlocksShopItem, unlocksTool, unlocksStageFocusType, id, increasesSprinklerRange }) => {
if (increasesSprinklerRange) {
acc.sprinklerRange++
}
@@ -37,6 +38,10 @@ export const getLevelEntitlements = memoize(
acc.tools[unlocksTool] = true
}
+ if (unlocksStageFocusType) {
+ acc.stageFocusType[unlocksStageFocusType] = true
+ }
+
return id === levelNumber
}
)
diff --git a/src/utils/getLevelEntitlements.test.js b/src/utils/getLevelEntitlements.test.js
index 39021fbd..1f267b09 100644
--- a/src/utils/getLevelEntitlements.test.js
+++ b/src/utils/getLevelEntitlements.test.js
@@ -16,6 +16,7 @@ describe('getLevelEntitlements', () => {
tools: {
SHOVEL: true,
},
+ stageFocusType: {},
})
})
})
What do you think @lstebner?
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.
nice one, just made this change! everything seems to be working so I think I got it all right, but definitely let me know if something seems off or missed
@@ -36,8 +36,10 @@ export const processLevelUp = (state, oldLevel) => { | |||
getRandomLevelUpRewardQuantity(i), | |||
true | |||
) | |||
} else if (levelObject && levelObject.unlocksTool) { | |||
state.toolLevels = unlockTool(state.toolLevels, levelObject.unlocksTool) |
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.
Thank you for improving 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.
nit: Remove the new, commented, non-documentation lines in this file.
src/game-logic/reducers/sellItem.js
Outdated
// this processLevelUp call actually doesn't have to happen because addExperience | ||
// will call it and this results in double processing. although it doesn't cause | ||
// any negative side effects, we could remove it.. eh? | ||
//state = processLevelUp(state, oldLevel) |
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 think it makes sense to drop this in favor of calling processLevelUp
from addExperience
like you're suggesting here, assuming it doesn't break anything. Good thinking! Let's go with it. :)
export const SHOVEL_UNLOCKED = | ||
"You've unlocked a new tool for the field, The **Shovel**!" |
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.
Thank you for moving this here!
src/utils/index.js
Outdated
export const unlockTool = (state, toolType) => { | ||
const { toolLevels } = state | ||
|
||
if (toolLevels[toolType] === toolLevel.UNAVAILABLE) { | ||
return { | ||
...state, | ||
toolLevels: { | ||
...toolLevels, | ||
[toolType]: toolLevel.DEFAULT, | ||
}, | ||
} | ||
} | ||
|
||
return currentToolLevels | ||
return state | ||
} | ||
|
||
/* | ||
* @param {farmhand.state} state | ||
* @param {farmhand.stageFocusType} stageType | ||
* @return {farmhand.state} | ||
*/ | ||
export const unlockStage = (state, stageType) => { | ||
return { | ||
...state, | ||
stagesUnlocked: { | ||
...state.stagesUnlocked, | ||
[stageType]: true, | ||
}, | ||
} |
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.
These are much better than what was here before! Now they are functionally reducers, so they should probably be moved to individual files in src/game-logic/reducers/
.
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.
ah yeah. i've moved unlockTool
but with the change to use entitlements it seems that unlockStage
isn't even needed any more so i've simply removed that one
src/utils/index.test.js
Outdated
@@ -1076,3 +1084,37 @@ describe('getGrowingPhase', () => { | |||
expect(getGrowingPhase(crop)).toEqual(phase) | |||
}) | |||
}) | |||
|
|||
describe('unlockTool', () => { |
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.
Great idea to add tests for 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.
i was embarrassed to find that they were missing 🙊
- remove stagesUnlocked in favor levelEntitlements - removed commented out lines - move unlockTool from utils to reducers
excellent, this feels much better now!
of course, and thanks for all the back and forth getting this one into the right shape! |
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.
Excellent work, @lstebner! Thanks for sticking with this throughout all the feedback and iterations. I think this is a great base to build the rest of the Forest feature on top of!
LGTM! 👏
test.each(['Seeds', 'Supplies', 'Upgrades'])( | ||
'the %s tab exists', | ||
tabLabel => { | ||
expect(screen.getByText(tabLabel)).toBeInTheDocument() | ||
} | ||
) |
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.
🤩
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 added this to src/game-logic/reducers/index.js
in aa95ef4.
What this PR does
add the beginnings of the forest behind a feature flag
How this change can be validated
Questions or concerns about this change
As I was doing this change I started to think it might be cool to have the forest unlock at a certain level instead of requiring purchase for the initial forest, so this is why i put in the INITIAL forest size constants. then once unlocked the upgrades can be purchased to expand it. If we go this route I think it should be a decently high level, like maybe 20. I think that would also help justify higher prices for the expansions.
Additional information