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

Add food_* groups to default edibles #2089

Merged
merged 5 commits into from
Apr 4, 2018
Merged

Add food_* groups to default edibles #2089

merged 5 commits into from
Apr 4, 2018

Conversation

tenplus1
Copy link
Contributor

@tenplus1 tenplus1 commented Mar 22, 2018

This adds the food_* groups to edible items within default so that food crafting is easier within mods.

Added {food_apple = 1} group to apple.
Added food_? groups to farming edibles.
Added food_* group to edible brown shroom
@tenplus1 tenplus1 changed the title Add food_* group to apple Add food_* groups to default edibles Mar 22, 2018
@Ezhh Ezhh added the Feature label Mar 22, 2018
@@ -25,17 +25,21 @@ farming.register_plant("farming:wheat", {
place_param2 = 3,
})

minetest.override_item("farming:wheat", {
groups = {food_wheat = 1, flammable = 2},
Copy link
Contributor

@paramat paramat Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheat wouldn't be used in food crafting, only flour.

@tenplus1
Copy link
Contributor Author

tenplus1 commented Mar 23, 2018

You'd be surprised how many recipes ask for wheat as an item, gotta get those whole grains :)

@paramat
Copy link
Contributor

paramat commented Mar 23, 2018

Ok.

@paramat
Copy link
Contributor

paramat commented Mar 24, 2018

I don't like this override though, should be done another way.

@Ezhh
Copy link
Contributor

Ezhh commented Mar 24, 2018

Did comment earlier in -hub that I don't like the override. That's really the only thing putting me off this. A mod needing to override what it registers itself is bad.

@paramat
Copy link
Contributor

paramat commented Mar 24, 2018

I'm also unsure about the whole PR, undecided.

@Ezhh
Copy link
Contributor

Ezhh commented Mar 31, 2018

I'm fine with this PR, if the override issue is resolved.

We've previously talked about adding more groups to help enable mods to be less game specific, and this seems like a good step toward that.

@tenplus1
Copy link
Contributor Author

tenplus1 commented Mar 31, 2018

The only other way to do this is to tweak the farming api for registering crops so that it adds custom groups.

this fixes the def.groups so they can be assigned to output craftitem.
remove wheat override cause register_plant api has working def.groups included instead
@tenplus1
Copy link
Contributor Author

tenplus1 commented Mar 31, 2018

Okies, when using farming.register_plant() it will now use 'def.groups' for the output crop (wheat, cotton etc.) so the override is no longer required :)

@paramat
Copy link
Contributor

paramat commented Apr 1, 2018

Much better.
I'm neutral on the PR.

@tenplus1
Copy link
Contributor Author

tenplus1 commented Apr 3, 2018

@paramat you could +1 it simply for the fact it fixes the farming custom groups in the api.

@paramat
Copy link
Contributor

paramat commented Apr 3, 2018

@tenplus1 i could also be neutral.

@Fixer-007
Copy link
Contributor

You are neutral because unsure, or there are negative side effects?

@paramat
Copy link
Contributor

paramat commented Apr 3, 2018

I can't see any negatives (apart from code bloat / complexity), just unsure if useful enough to be justified. I'll leave it to other devs to decide.

@SmallJoker SmallJoker merged commit 11b3407 into minetest:master Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants