-
Notifications
You must be signed in to change notification settings - Fork 30
feat: add weeds and compost #311
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
Conversation
- weeds grow randomly overnight in unoccupied plots - weeds can be turned into compost through crafting - compost acts like fertilizer
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This is very cool! I love the idea. I know you're still working on this so I won't dig into the code yet. I've got some initial play testing feedback (some of which you probably have in mind already):
|
- added purchaseable composter - weeds are no longer considered crops - lower value on weeds - added itemType 'WEEDS' - refactored harvestPlot a bit to make it easier to understand for weeds vs crops
f1c994f
to
c92808e
Compare
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 work so far! We've already talked about a few things here on Discord, but I wanted to get it all in once place here on the PR. Hopefully none of my suggestions are too onerous to deal with.
The art looks great to me!
src/enums.js
Outdated
'NONE', | ||
'STANDARD', | ||
'RAINBOW', | ||
'COMPOST', |
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.
Would it make sense to drop the COMPOST
fertilizer type and instead craft STANDARD
fertilizer with pulled weeds? I'm thinking that would be simplify the implementation and perhaps be a little more straightforward for players too.
src/data/items.js
Outdated
type: WEEDS, | ||
}) | ||
|
||
export { weeds } |
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.
Totally not a big deal, but if you have to go back in here it might be worth exporting the weeds
variable directly from its definition above to keep consistent with other item definitions.
src/data/items.js
Outdated
const weeds = freeze({ | ||
id: 'weeds', | ||
name: 'Weeds', |
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.
What do you think about changing these terms to be be singular rather than plural, to be consistent with the other items? So, weed
/Weed
instead of weeds
/Weeds
.
src/utils.js
Outdated
export const getPlotImage = plotContent => { | ||
if (plotContent) { | ||
if (getPlotContentType(plotContent) === itemType.CROP) { | ||
console.log(getCropLifeStage(plotContent), getCropId(plotContent)) |
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.
Reminder to drop this before merging. 🎗️
src/enums.js
Outdated
'TOMATO', | ||
'WATERMELON', | ||
'WHEAT', | ||
'WEEDS', |
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.
What do you think about switching from plural "weeds" semantics to singular ("weed") for both the cropType
here and the itemType
below in order to remain consistent with the other cropType
and itemType
semantics?
src/enums.js
Outdated
'NONE', | ||
'STANDARD', | ||
'RAINBOW', | ||
'COMPOST', |
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.
If we drop the mechanic of using compost as fertilizer directly, I think we can drop this addition.
src/components/Plot/Plot.js
Outdated
|
||
if (plotContent.fertilizerType === fertilizerType.STANDARD) { | ||
if ( | ||
[fertilizerType.STANDARD, fertilizerType.COMPOST].includes( |
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.
If we drop fertilizerType.COMPOST
, we can revert this change.
… into new Recycling area of workshop
…ngredients, and consider weeds ripe so they are highlighted when using scythe
thank you for the excellent and thorough review @jeremyckahn ! you had a ton of great suggestions and I believe I made them all (except one that I responded to). i also made an additional change to support multiple images for weeds and changed the graphic in general so make sure you give that code a look and let me know if it's ok or there's a better way to do it. after playing a bit i actually kinda like the compost image, but am open to modifying it if you have ideas. |
Co-authored-by: Jeremy Kahn <[email protected]>
I found a separate issue that may also be related to shoveling: To reproduce this issue, load crashing-farmhand-data.zip, go to the Field, select the Hoe, and click on the field. This may or may not be related to shoveling some days prior, but I haven't been able to debug much yet. |
99fdcaa seems to be addressing the weed animation issue! Nice work on that. I'm still able to reproduce the crashing issue, but that seems like the last remaining bug. |
This should be fixed in 760bcb4! Please review and make sure it's working correctly for you. I noticed some tests are failing due to 99fdcaa, so we'll just need to determine whether these are legitimate breakages or just outdated tests. |
thank you! i tested with the save you provided and it doesn't crash anymore 🎉
these should be fixed now! |
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 found another bug. 😩
Unfortunately the Plot
code is a little fragile, as we're seeing now. We are getting close though!
src/game-logic/reducers/minePlot.js
Outdated
state = modifyFieldPlotAt(state, x, y, () => { | ||
return { | ||
isShoveled: true, | ||
wasJustShoveled: 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.
Good idea!
useEffect(() => { | ||
if ( | ||
!initialIsShoveledState && | ||
plotContent?.isShoveled && | ||
plotContent?.oreId | ||
) { | ||
setWasJustShoveled(true) | ||
} | ||
}, [initialIsShoveledState, plotContent]) |
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 I know why this was here now. I found a newly-introduced bug wherein the "mined" animation replays when you leave the Field and then come back:
Screencast.from.07-20-2022.09.19.21.PM.webm
We'll have to figure out a solution for this before we can merge. 😕
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 this is ready to go now! I just pushed some cleanup and refactoring work, so please review that and let me know if it looks good to you. If it does, feel free to merge and run a minor release since this change affects the persisted data structure.
Awesome work here! Thanks for sticking with this and knocking out all the bugs. And congrats on implementing a brand new feature! 🎉
note: this is a work in progress. the art is especially mediocre at this point but it's mostly working haha
What this PR does
add "weeds" and "compost"
How this change can be validated
Questions or concerns about this change
Additional information