Skip to content
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

Cattail can be planted in shallow water #68664

Merged
merged 4 commits into from Oct 16, 2023

Conversation

Light-Wave
Copy link
Contributor

Summary

Content "Cattail can be planted in shallow water"

Purpose of change

#68616 made cattail not plantable on land. This PR makes cattail farmable again. It also makes it so that different plants can have different terrain requirements for farming.

Describe the solution

"seed_data" has a new optional field, "required_terrain_flag". This is set to "PLANTABLE" by default. In order to plant a seed somewhere, the terrain or furniture there needs to have the given flag.
If this flag is set to anything other than "PLANTABLE", the terrain will not turn into dirt after the seed is planted. This is to prevent the water to turn into dirt when you plant stuff in it.
Places that previously checked for the "PLANTABLE" tag now instead checks for the tag specified by the "seed_data".
Cattail seeds are set to have "required_terrain_flag": "SHALLOW_WATER".
Documentation is updated to match.
A new hardcoded message string is added for when you're trying to plant a seed in an invalid spot; add_msg( _( "This type of seed can not be planted in this location." ) );. This might need translation.

Describe alternatives you've considered

Give shallow water "examine_action": "dirtmound" so that seeds can be planted. I felt this would clutter up the UI, but right now the seeds can not be planted manually, and have to instead be planted using the zone manager, which might be confusing.
Create aquatic farmland as a furniture item instead. This could be given the "examine_action": "dirtmound" to let players plant the crops manually. I felt like this would add extra pointless work for the player, but I'm not sure if it might be a better idea. Using a furniture instead of a terrain would also make it so I don't need a special case in order to not turn the planted terrain into dirt if we use special terrain.
Hide seeds that can't be planted from the seed selection menu. Felt like extra work, and would confuse the player. My hope is right now that they will try to plant cattail seeds in soil, get the error message that "This type of seed can not be planted in this location.", read the item description and notice that it says it must be planted in shallow water, and figure things out from there. Thus, hiding invalid seeds might be more confusing than helpful.
If this were to be used for non-seed like beehives, perhaps it would be a better idea to hide invalid seeds and change the message to "You can not plant <item_name_plural> in this location.".
Right now, if you define an area of dirt to be used to farm cattail using the zone manager, the player will still try to tilt the earth, but be unable to plant the seeds. Perhaps tilting of earth should not be done for farmlands where the seeds does not have "required_terrain_flag": "PLANTABLE"
Instead of always trying to tile an area, let seed_data define a construction_group it will try to use to make the area plantable. Seems like a much better solution, but also sounds like a bunch of extra work. Maybe later.

Testing

From my testing, it seems to work.

Here I designated an area to be a cattail farm:
Capture

Here I am after having farmed the area:
Capture2

Harvesting also seems to work fine:
Capture3

Asking an NPC to work the field also seems to work fine. It also still works to plant normal crops both by yourself and with NPCs:
Capture5

Additional context

@harakka requested a ping.
Also pinging @AmarinReyny since they asked about this.
If anyone knows if any extra steps are needed to translate the hardcoded message I added, feel free to let me know. Translation is magic to me.

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` [Markdown] Markdown issues and PRs labels Oct 15, 2023
@AmarinReyny
Copy link

Thank you!!!

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Oct 15, 2023
@harakka
Copy link
Contributor

harakka commented Oct 15, 2023

Looks good, thanks for doing this.
In longer term more complex checks (like SHALLOW_WATER but no CURRENT) would be nice, but like we discussed I think EOCs would be a good way to evaluate those conditions (so we don't need to implement separate sets of allowed and forbidden flags etc) and we don't have enough muscle for that yet. I'll keep an eye on that.

@Light-Wave
Copy link
Contributor Author

Looks good, thanks for doing this. In longer term more complex checks (like SHALLOW_WATER but no CURRENT) would be nice, but like we discussed I think EOCs would be a good way to evaluate those conditions (so we don't need to implement separate sets of allowed and forbidden flags etc) and we don't have enough muscle for that yet. I'll keep an eye on that.

It should be possible to add a new tag called CATTAIL_PLANTABLE and use that instead. That being said, cattail currently grows naturally in water with currents in-game.

@AmarinReyny
Copy link

Would it not be easier to use terrain IDs instead of flags? If needed, maybe there could be terrain groups representing multiple types of terrain.

Then again, I have no idea how to code C++ or anything like that, so I am aware that I might be completely wrong about this.

@Light-Wave
Copy link
Contributor Author

Would it not be easier to use terrain IDs instead of flags? If needed, maybe there could be terrain groups representing multiple types of terrain.

I considered that, but since you can add custom tags if you want to, and add them to the terrain or furniture you want to be able to grow a specific plant, the solution as it is is essentially equivalent. You already have full control over which terrains should and should not be plantable with which seeds.

@AmarinReyny
Copy link

Would it not be easier to use terrain IDs instead of flags? If needed, maybe there could be terrain groups representing multiple types of terrain.

I considered that, but since you can add custom tags if you want to, and add them to the terrain or furniture you want to be able to grow a specific plant, the solution as it is is essentially equivalent. You already have full control over which terrains should and should not be plantable with which seeds.

... Wait, we can do that? Awesome! Thank you for letting me know!

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 15, 2023
@Maleclypse Maleclypse merged commit 43d4862 into CleverRaven:master Oct 16, 2023
25 checks passed
@ThePainkiller
Copy link

Does this change require a new world to make effect? Because interacting with any terrain with the SHALLOW_WATER flag only returns the usual consume/pour in container/pour in ground menu, even if my cattail seeds say they can be planted on shallow water.
Tried with t_water_sh, t_water_murky, and t_water_moving_sh while having cattails in inventory.

@ehughsbaird
Copy link
Contributor

Give shallow water "examine_action": "dirtmound" so that seeds can be planted. I felt this would clutter up the UI, but right now the seeds can not be planted manually, and have to instead be planted using the zone manager, which might be confusing.

detahramet pushed a commit to detahramet/Cataclysm-DDA that referenced this pull request Nov 6, 2023
* commit

* Make examine action not able to plant incorrect seed

* Update seed.json

* retrigger checks
@BjoHart
Copy link
Contributor

BjoHart commented Nov 23, 2023

Give shallow water "examine_action": "dirtmound" so that seeds can be planted. I felt this would clutter up the UI, but right now the seeds can not be planted manually, and have to instead be planted using the zone manager, which might be confusing.

Is there a way to check your inventory for the seeds, and only if you have seeds in your inventory, the option to plant the seeds in the shallow water will apear?
To not clutter up the UI to much...

@kevingranade
Copy link
Member

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/how-to-plant-cattail-seeds/29081/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants