Skip to content

Add eggfruit and ratatouille - #24

Open
spixi wants to merge 1 commit into
minetest-mods:masterfrom
spixi:master
Open

Add eggfruit and ratatouille#24
spixi wants to merge 1 commit into
minetest-mods:masterfrom
spixi:master

Conversation

@spixi

@spixi spixi commented Dec 1, 2018

Copy link
Copy Markdown

This adds another crop, the eggfruit, and a new food item to this mod.

@SmallJoker SmallJoker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks quite good already. Did you also run optipng or pngcrush on your textures yet? On servers every byte matters :)

Comment thread cooking.lua Outdated
Comment thread cooking.lua Outdated
Comment thread eggfruit.lua
Comment thread eggfruit.lua Outdated
Comment thread eggfruit.lua Outdated
Comment thread eggfruit.lua Outdated
Comment thread eggfruit.lua Outdated
Comment thread eggfruit.lua Outdated
Comment thread locale/de.po Outdated
Comment thread readme.md Outdated
@spixi

spixi commented Dec 9, 2018

Copy link
Copy Markdown
Author

Hi @SmallJoker,

thank you for your review. I tried to fix most issues and also did some refactoring in tomato.lua and added translations.

What do you think?

@SmallJoker

Copy link
Copy Markdown
Member

Very nice, thank you for these changes. @sofar might want to have a quick look this this PR? He's more familiar with this code than me (even though it's quite simple).

@SmallJoker

Copy link
Copy Markdown
Member

@sofar This PR is ready for merge. If there are no objections I will merge it soon.

@sofar

sofar commented Feb 3, 2020

Copy link
Copy Markdown
Member

Uh

Well in large the new addition seems fine, but, I don't understand why the tomato code needed to change. Sorry I didn't see this before, but, I think I'd like to make sure that we're not merging something that shouldn't be there.

Also, translation changes should be separate commits. This is all squashed into 1 commit - fine for a new plant addition and a cooking recipe, but not for that many changes IMHO.

Comment thread tomato.lua

crops.tomato_die = function(pos)
minetest.set_node(pos, { name = "crops:tomato_plant_6", param2 = 1 })
minetest.swap_node(pos, { name = "crops:tomato_plant_6", param2 = 1 })

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

set_node erases the metadata, which is intentional here. This is probably wrong.

Comment thread tomato.lua
end
})

for stage = 1, 4 do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

separate commit for this entire section, please

Comment thread readme.md
effort.

The watering can can be made as follows:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you post some screenshots of how this looks in-game? The PNG's, obviously. Did you draw the images yourself?

Comment thread eggfruit.lua
--[[

Copyright (C) 2015 - Auke Kok <sofar@foo-projects.org>
2018 - Marius Spix <marius.spix@web.de>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fine, but, you didn't claim or license the PNG files in the LICENSE file. This is required - make sure to document the origin of all the textures and choose an appropriate license (if it's the same license as the other images, perfect. If it's not, we can discuss)

@spixi

spixi commented Feb 16, 2025

Copy link
Copy Markdown
Author

What about this PR? Is this repo still actively maintained?

@sofar

sofar commented Feb 16, 2025

Copy link
Copy Markdown
Member

There's open review comments that were asked to be addressed. If you need help or something in the comments isn't clear, feel free to ask.

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.

3 participants