Improve plant growth system, add moisture level #4681

Merged
the-real-herowl merged 12 commits from kno10/VoxeLibre:pumpkin-melon-growth-1 into master 2024-11-10 02:11:40 +01:00
Member
  • pumpkins/melons check only one neighbor each time
  • plants grow faster when surrounded by wet farmland
  • plants grow slower when surrounded by the same crop. A good layout is rows
  • ABM intervals vary slightly (e.g., 5.4015) to not have them all run at once
  • added a user setting to control plant growth speed
  • closes: #4683 by removing the average light level and using sky light (or artificial light) instead (obsoletes closes #4649)
  • simplify catch-up logic in LBM, drop the need for checking last tick metadata

The values are chosen to be similar to what I read that minecraft apparently uses.
With optimum water and sufficient light, there is a 100% chance of growing in our code when ticked, but with worse conditions it can take much longer (as in minecraft, hydration matters now). In contrast to minecraft, we allow pumpkins and melons to grown on any solid node, not only on certain dirt nodes (because that is what real pumpkins would do); growth speed is still affected by wet soil nearby.

Note: this is expected to reduce crop growth; in particular of melons and pumpkins that will not have as much wet farmland around, many farms will trade "optimal" conditions for space efficiency.

- [x] pumpkins/melons check only one neighbor each time - [x] plants grow faster when surrounded by wet farmland - [x] plants grow slower when surrounded by the same crop. A good layout is rows - [x] ABM intervals vary slightly (e.g., 5.4015) to not have them all run at once - [x] added a user setting to control plant growth speed - [x] closes: #4683 by removing the average light level and using sky light (or artificial light) instead (obsoletes closes #4649) - [x] simplify catch-up logic in LBM, drop the need for checking last tick metadata The values are chosen to be similar to what I read that minecraft apparently uses. With optimum water and sufficient light, there is a 100% chance of growing in our code when ticked, but with worse conditions it can take much longer (as in minecraft, hydration matters now). In contrast to minecraft, we allow pumpkins and melons to grown on any solid node, not only on certain dirt nodes (because that is what real pumpkins would do); growth speed is still affected by wet soil nearby. Note: this is *expected* to reduce crop growth; in particular of melons and pumpkins that will not have as much wet farmland around, many farms will trade "optimal" conditions for space efficiency.
kno10 force-pushed pumpkin-melon-growth-1 from a023de65b3 to b552dacb15 2024-10-09 23:21:39 +02:00 Compare
kno10 force-pushed pumpkin-melon-growth-1 from b552dacb15 to 02202c65b2 2024-10-10 09:12:47 +02:00 Compare
kno10 force-pushed pumpkin-melon-growth-1 from 02202c65b2 to 8cbe01ed81 2024-10-10 14:38:16 +02:00 Compare
kno10 force-pushed pumpkin-melon-growth-1 from 8cbe01ed81 to d5aaa8faae 2024-10-10 16:25:41 +02:00 Compare
kno10 force-pushed pumpkin-melon-growth-1 from d5aaa8faae to 6bdc8aa409 2024-10-10 16:45:17 +02:00 Compare
kno10 changed title from WIP: moisture level for growing speeds to Improve plant growth system, add moisture level 2024-10-10 16:47:45 +02:00
kno10 added the
nodes
configurability
enhancement
labels 2024-10-10 16:49:27 +02:00
kno10 force-pushed pumpkin-melon-growth-1 from 6bdc8aa409 to 061af6a76a 2024-10-10 17:06:22 +02:00 Compare
kno10 added this to the 0.89.0 milestone 2024-10-10 21:09:11 +02:00
Author
Member

@kneekoo maybe you can help test this?

@kneekoo maybe you can help test this?
Contributor

They grow a bit too fast. One beetroot plant matured so fast, that the farmland under it didn't even manage to get hydrated.

mature beetroot on dry farmland

As seen, I tried both types of farms - single-crop and alternating rows. So far there doesn't seem to be an advantage to grow crops in alternative rows. As you can see, the first mature wheat is ready on the left side (single-crop), while none of the other crops in the alternating-rows farm are mature. But it didn't take too much (30-45 seconds) for a potato crop to mature on the right side.

single-crop farm vs alternating-rows farm

Of course there's also a delay between the two farms, first I planted the single-crop one, and then the other. So that's a factor, although it didn't take me long to plant the seeds on both.

They grow a bit too fast. One beetroot plant matured so fast, that the farmland under it didn't even manage to get hydrated. ![mature beetroot on dry farmland](https://i.imgur.com/8eDEccv.jpeg) As seen, I tried both types of farms - single-crop and alternating rows. So far there doesn't seem to be an advantage to grow crops in alternative rows. As you can see, the first mature wheat is ready on the left side (single-crop), while none of the other crops in the alternating-rows farm are mature. But it didn't take too much (30-45 seconds) for a potato crop to mature on the right side. ![single-crop farm vs alternating-rows farm](https://i.imgur.com/lZzlkYZ.jpeg) Of course there's also a delay between the two farms, first I planted the single-crop one, and then the other. So that's a factor, although it didn't take me long to plant the seeds on both.
Author
Member

Or hydration checks are too infrequent. ;-) But yes, I also find growth rates a bit high, but the parameters were chosen to be similar to the MC wiki.

The catch-up logic may still be giving plant growth an undesired "boost", afaict MC does not have such a thing.

Or hydration checks are too infrequent. ;-) But yes, I also find growth rates a bit high, but the parameters were chosen to be similar to the MC wiki. The catch-up logic may still be giving plant growth an undesired "boost", afaict MC does not have such a thing.
kno10 added the
Testing / Retest
label 2024-10-13 17:06:30 +02:00
kno10 force-pushed pumpkin-melon-growth-1 from 10dc367dad to bbb8036fbf 2024-10-14 01:05:10 +02:00 Compare
Author
Member

@kneekoo please try again, fixed some more issues (in particular, only crop of the same growth stage was considered)

I also modified wettening and drying.

@kneekoo please try again, fixed some more issues (in particular, only crop of the same growth stage was considered) I also modified wettening and drying.
Contributor

Here's the second test, with the latest changes. The first mature crop was 1 beetroot, after about 4 minutes. That's under 5 hours in-game, so we're still very fast with crop growth.

As far as hydration goes, I think it's fine. What happened in my first screenshot was really our crops maturing way too fast.

mature crop in 4 minutes

To make testing easier (spending less time to watch over crop growth), what do you think about the following? Is this easy to pull off?

  • add the game time to a newly planted crop, as metadata
  • when the crop reaches its final stage, output the time difference in the log
Here's the second test, with the latest changes. The first mature crop was 1 beetroot, after about 4 minutes. That's under 5 hours in-game, so we're still very fast with crop growth. As far as hydration goes, I think it's fine. What happened in my first screenshot was really our crops maturing way too fast. ![mature crop in 4 minutes](https://i.imgur.com/K6x41vX.jpeg) To make testing easier *(spending less time to watch over crop growth)*, what do you think about the following? Is this easy to pull off? - add the game time to a newly planted crop, as metadata - when the crop reaches its final stage, output the time difference in the log
rudzik8 requested changes 2024-10-14 16:14:50 +02:00
rudzik8 left a comment
Member

Code-wise, I like the changes. The code is much simpler. Didn't test in-game yet though.

Here's the review, with a lot of questions attached.

Code-wise, I like the changes. The code is much simpler. Didn't test in-game yet though. Here's the review, with a lot of questions attached.
@ -43,0 +44,4 @@
n.x = pos.x + x
local ndef = minetest.registered_nodes[minetest.get_node(n).name]
local soil = ndef and ndef.groups.soil
if soil and soil >= 2 then
Member

IIRC ndef.groups can be nil too (=> potential crash because of nil indexing)

IIRC `ndef.groups` can be nil too (=> potential crash because of nil indexing)
Author
Member

The minetest developers seem to think otherwise: https://github.com/minetest/minetest/pull/15260#discussion_r1795615907
If that happens, most likely some mod has been messing with the node registry in a hackish way.
Current minetest.get_item_group will also fail, so not having groups will break a lot of things before reaching this code.

The minetest developers seem to think otherwise: https://github.com/minetest/minetest/pull/15260#discussion_r1795615907 If that happens, most likely some mod has been messing with the node registry in a hackish way. Current `minetest.get_item_group` will also fail, so not having groups will break a lot of things before reaching this code.
Member

Good to know. Perhaps we should clean-up all of our def and def.groups and def.groups[...] stacks too as we progress with the refactors and rewrites.

Good to know. Perhaps we should clean-up all of our `def and def.groups and def.groups[...]` stacks too as we progress with the refactors and rewrites.
kno10 marked this conversation as resolved
@ -43,0 +45,4 @@
local ndef = minetest.registered_nodes[minetest.get_node(n).name]
local soil = ndef and ndef.groups.soil
if soil and soil >= 2 then
local m = (soil > 2 or soil == 2 and (minetest.get_meta(n):get_int("wet") or 0) > 0) and 3 or 1
Member

Isn't soil > 2 or soil == 2 equivalent to soil >= 2? And besides, this is already checked in the if statement above.

Isn't `soil > 2 or soil == 2` equivalent to `soil >= 2`? And besides, this is already checked in the `if` statement above.
Author
Member

This is soil > 2 OR (soil == 2 and (minetest.get_meta(n):get_int("wet") or 0) > 0) but I did not add the extra brackets.
For soil = 3 (wet), the meta data is not accessed.

This is `soil > 2` OR (`soil == 2 and (minetest.get_meta(n):get_int("wet") or 0) > 0`) but I did not add the extra brackets. For soil = 3 (wet), the meta data is not accessed.

get_int() should return 0 when a key is not set.

Also you could add the extra parentheses to make the intent clear to people who are not used to logical operator precedence rules.

get_int() should return 0 when a key is not set. Also you could add the extra parentheses to make the intent clear to people who are not used to logical operator precedence rules.
kno10 marked this conversation as resolved
@ -49,0 +63,4 @@
local function get_same_crop_penalty(pos)
local name = minetest.get_node(pos).name
local plant = plant_nodename_to_id[name]
if not plant then return "unregistered plant" end
Member

Why can't we just return nil here?

Why can't we just return nil here?
Author
Member

Trying to provoke a more meaningful error message. But it does not print the string, we might as well just use nil, yes.

Trying to provoke a more meaningful error message. But it does not print the string, we might as well just use nil, yes.
Member

IMO error-handling in Lua mostly comes down to nil-handling, as inconvenient as that may be. I advocate for keeping this practice, as it's better than checking for a full string or for the return type just to know whether it failed or not.

IMO error-handling in Lua mostly comes down to nil-handling, as inconvenient as that may be. I advocate for keeping this practice, as it's better than checking for a full string or for the return type just to know whether it failed or not.
kno10 marked this conversation as resolved
@ -83,3 +123,2 @@
-- stages: Number of stages to advance (optional, defaults to 1)
-- ignore_light: if true, ignore light requirements for growing
-- low_speed: grow more slowly (not wet), default false
-- ignore_light_water: if true, ignore light and water requirements for growing
Member

First, I wanted to shout "THE DOCS!", but there are none. Still, this is a change to the API, so we might need to do something about it. Or not, if nobody used low_speed.

First, I wanted to shout "THE DOCS!", but there are none. Still, this is a change to the API, so we might need to do something about it. Or not, if nobody used `low_speed`.
Author
Member

low_speed was never user-facing API, but only used for the catch-up logic. The only outside use is IIRC bonemeal, and it does not even set the parameter to false.

`low_speed` was never user-facing API, but only used for the catch-up logic. The only outside use is IIRC bonemeal, and it does not even set the parameter to `false`.
rudzik8 marked this conversation as resolved
@ -107,0 +136,4 @@
local odds = floor(25 / (get_moisture_level(pos) * get_same_crop_penalty(pos))) + 1
for i = 1,stages do
-- compared to MC, our ABM runs half as often, hence we use double the chance
if random() * odds >= 2 then stages = stages - 1 end
Member

Why reference MC in the code?
(Also, this change might be up to discussion)

Why reference MC in the code? (Also, this change might be up to discussion)
Author
Member

The factor 2 means half as much work for the same outcome.
But this is non-intuitive, and hence needs a comment to explain why this is supposed to be >= 2 and not a typo.

The factor 2 means half as much work for the same outcome. But this is non-intuitive, and hence needs a comment to explain why this is supposed to be `>= 2` and not a typo.
kno10 marked this conversation as resolved
@ -118,0 +145,4 @@
if step == nil then return false end
minetest.set_node(pos, {
name = plant_info.names[step + stages] or plant_info.full_grown,
param = node.param, param2 = node.param2,
Member

Nitpick: inconsistent linebreaks.

**Nitpick:** inconsistent linebreaks.
Author
Member

readability over automatic code formatting. Fold short lines that are trivial.

readability over automatic code formatting. Fold short lines that are trivial.
Member

I won't push on this, but it just doesn't feel right to me, that's all. When I explode a table construction field-by-field, I usually format it field-by-field too.

I won't push on this, but it just doesn't feel right to me, that's all. When I explode a table construction field-by-field, I usually format it field-by-field too.
kno10 marked this conversation as resolved
@ -361,0 +326,4 @@
local floorname = minetest.get_node(floorpos).name
local floordef = minetest.registered_nodes[floorname]
if not floordef then return end
if (floordef.groups.grass_block or 0) == 0 and (floordef.groups.dirt or 0) == 0 and (floordef.groups.soil or 0) < 2 then return end -- not suitable for growing
Member
[Same concern.](https://git.minetest.land/VoxeLibre/VoxeLibre/pulls/4681/files#issuecomment-84913)
rudzik8 marked this conversation as resolved
@ -416,0 +386,4 @@
})
-- The average light levels were unreliable
-- LBM added in fall 2024
Member

Are we giving individual code lines dates now? Shouldn't git blame fit these purposes?

Are we giving individual code lines dates now? Shouldn't `git blame` fit these purposes?
Author
Member

This hints that this LBM maybe can be removed in 2026 without having to do the roundtrip; and blame may only give you the last modification, not when it was first introduced.

This hints that this LBM maybe can be removed in 2026 without having to do the roundtrip; and blame may only give you the last modification, not when it was first introduced.
Member

That's, of course, if it will be removed. XD

That's, of course, if it *will* be removed. XD
kno10 marked this conversation as resolved
@ -123,1 +91,4 @@
end
-- Revert to dirt if wetness is 0, and no plant above
local nn = minetest.get_node_or_nil(vector.offset(pos, 0, 1, 0))
local nn_def = nn and minetest.registered_nodes[nn.name] or nil
Member

What's the reasoning behind putting or nil here?

What's the reasoning behind putting `or nil` here?
Author
Member

emphasize that the value might be nil, and we need to handle nil below.

emphasize that the value might be nil, and we need to handle nil below.
kno10 marked this conversation as resolved
@ -275,3 +275,3 @@
},
is_ground_content = true,
groups = {handy = 1, shovely = 1, axey = 1, building_block = 1},
groups = {dirt = 2, handy = 1, shovely = 1, axey = 1, building_block = 1},
Member

Explanation?

Explanation?
Author
Member

https://minecraft.wiki/w/Tutorials/Pumpkin_and_melon_farming

The attempt to grow a fruit happens when the mature stem would grow again (to "phase 9") and is not already adjacent to an instance of its fruit. First one of the four sides is chosen. If this space is suitable (empty with dirt, coarse dirt, rooted dirt, grass block, farmland, podzol, mycelium, moss block, mud or muddy mangrove roots beneath) the fruit is created. Bone meal does not force fruit production.

Line 329 would otherwise need special treatment for Muddy Mangrove:

			if (floordef.groups.grass_block or 0) == 0 and (floordef.groups.dirt or 0) == 0 and (floordef.groups.soil or 0) < 2 then return end -- not suitable for growing

mcl_core:dirt has dirt = 1
mcl_core:podzol dirt has dirt = 2
mcl_core:dirt_with_grass has dirt = 2
mcl_core:mycelium has dirt = 2
mcl_core:coarse_dirt has dirt = 3

So a value of 2 seemed to be most appropriate to me, and adding the dirt group to muddy mangrove seemed cleaner than adding a special case to the if statement.

https://minecraft.wiki/w/Tutorials/Pumpkin_and_melon_farming > The attempt to grow a fruit happens when the mature stem would grow again (to "phase 9") and is not already adjacent to an instance of its fruit. First one of the four sides is chosen. **If this space is suitable (empty with dirt, coarse dirt, rooted dirt, grass block, farmland, podzol, mycelium, moss block, mud or m*uddy mangrove roots* beneath) the fruit is created.** Bone meal does not force fruit production. Line 329 would otherwise need special treatment for Muddy Mangrove: ``` if (floordef.groups.grass_block or 0) == 0 and (floordef.groups.dirt or 0) == 0 and (floordef.groups.soil or 0) < 2 then return end -- not suitable for growing ``` mcl_core:dirt has dirt = 1 mcl_core:podzol dirt has dirt = 2 mcl_core:dirt_with_grass has dirt = 2 mcl_core:mycelium has dirt = 2 mcl_core:coarse_dirt has dirt = 3 So a value of 2 seemed to be most appropriate to me, and adding the dirt group to muddy mangrove seemed cleaner than adding a special case to the if statement.
Member

Well, my main concern is the usage of group:dirt in our mapgen code (not to mention that it could be used in crafting recipes too).

grep -Hrn 'group:dirt':

ITEMS/mcl_core/functions.lua:643:	local nn=minetest.find_nodes_in_area_under_air(pos1, pos2, {"group:dirt"})
ITEMS/mcl_sculk/init.lua:7:local spread_to = {"mcl_core:stone","mcl_core:dirt","mcl_core:sand","mcl_core:dirt_with_grass","group:grass_block","mcl_core:andesite","mcl_core:diorite","mcl_core:granite","mcl_core:mycelium","group:dirt","mcl_end:end_stone","mcl_nether:netherrack","mcl_blackstone:basalt","mcl_nether:soul_sand","mcl_blackstone:soul_soil","mcl_crimson:warped_nylium","mcl_crimson:crimson_nylium","mcl_core:gravel"}
MAPGEN/tsm_railcorridors/init.lua:1100:	place_on = {"group:sand","group:grass_block","mcl_core:water_source","group:dirt","mcl_core:dirt_with_grass","mcl_core:gravel","group:material_stone","mcl_core:snow"},
MAPGEN/mcl_terrain_features/init.lua:169:	place_on = {"group:sand", "group:dirt", "group:stone"},
MAPGEN/mcl_terrain_features/init.lua:184:		return makelake(pos,5,"mcl_core:lava_source",{"group:material_stone", "group:sand", "group:dirt"},"mcl_core:stone",pr)
MAPGEN/mcl_terrain_features/init.lua:189:	place_on = {"group:dirt","group:stone"},
MAPGEN/mcl_terrain_features/init.lua:204:		return makelake(pos,5,"mcl_core:water_source",{"group:material_stone", "group:sand", "group:dirt","group:grass_block"},"mcl_core:dirt_with_grass",pr)
MAPGEN/mcl_terrain_features/init.lua:225:		return makelake(pos,3,"mcl_core:water_source",{"group:material_stone", "group:sand", "group:dirt","group:grass_block","mcl_mud:mud"},"mcl_mud:mud",pr,true)
MAPGEN/mcl_structures/jungle_temple.lua:6:	place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass"},
MAPGEN/mcl_structures/pillager_outpost.lua:9:	place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass","group:sand"},
MAPGEN/mcl_structures/woodland_mansion.lua:9:	place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass"},
MAPGEN/mcl_structures/geode.lua:24:		local nn = minetest.find_nodes_in_area(p1,p2,{"group:material_stone","group:dirt","mcl_core:gravel"})
MAPGEN/mcl_structures/ruined_portal.lua:14:	place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass","group:grass_block","group:sand","group:grass_block_snow","mcl_core:snow"},
MAPGEN/mcl_structures/api.lua:165:	local replace = {"air","group:liquid","mcl_core:snow","group:tree","group:leaves","group:plant","grass_block","group:dirt"}
Well, my main concern is the usage of `group:dirt` in our mapgen code (not to mention that it could be used in crafting recipes too). **`grep -Hrn 'group:dirt'`:** ```lua ITEMS/mcl_core/functions.lua:643: local nn=minetest.find_nodes_in_area_under_air(pos1, pos2, {"group:dirt"}) ITEMS/mcl_sculk/init.lua:7:local spread_to = {"mcl_core:stone","mcl_core:dirt","mcl_core:sand","mcl_core:dirt_with_grass","group:grass_block","mcl_core:andesite","mcl_core:diorite","mcl_core:granite","mcl_core:mycelium","group:dirt","mcl_end:end_stone","mcl_nether:netherrack","mcl_blackstone:basalt","mcl_nether:soul_sand","mcl_blackstone:soul_soil","mcl_crimson:warped_nylium","mcl_crimson:crimson_nylium","mcl_core:gravel"} MAPGEN/tsm_railcorridors/init.lua:1100: place_on = {"group:sand","group:grass_block","mcl_core:water_source","group:dirt","mcl_core:dirt_with_grass","mcl_core:gravel","group:material_stone","mcl_core:snow"}, MAPGEN/mcl_terrain_features/init.lua:169: place_on = {"group:sand", "group:dirt", "group:stone"}, MAPGEN/mcl_terrain_features/init.lua:184: return makelake(pos,5,"mcl_core:lava_source",{"group:material_stone", "group:sand", "group:dirt"},"mcl_core:stone",pr) MAPGEN/mcl_terrain_features/init.lua:189: place_on = {"group:dirt","group:stone"}, MAPGEN/mcl_terrain_features/init.lua:204: return makelake(pos,5,"mcl_core:water_source",{"group:material_stone", "group:sand", "group:dirt","group:grass_block"},"mcl_core:dirt_with_grass",pr) MAPGEN/mcl_terrain_features/init.lua:225: return makelake(pos,3,"mcl_core:water_source",{"group:material_stone", "group:sand", "group:dirt","group:grass_block","mcl_mud:mud"},"mcl_mud:mud",pr,true) MAPGEN/mcl_structures/jungle_temple.lua:6: place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass"}, MAPGEN/mcl_structures/pillager_outpost.lua:9: place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass","group:sand"}, MAPGEN/mcl_structures/woodland_mansion.lua:9: place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass"}, MAPGEN/mcl_structures/geode.lua:24: local nn = minetest.find_nodes_in_area(p1,p2,{"group:material_stone","group:dirt","mcl_core:gravel"}) MAPGEN/mcl_structures/ruined_portal.lua:14: place_on = {"group:grass_block","group:dirt","mcl_core:dirt_with_grass","group:grass_block","group:sand","group:grass_block_snow","mcl_core:snow"}, MAPGEN/mcl_structures/api.lua:165: local replace = {"air","group:liquid","mcl_core:snow","group:tree","group:leaves","group:plant","grass_block","group:dirt"} ```
Author
Member

ITEMS/mcl_core/functions.lua: different biome, and matches the purpose of group:dirt?
ITEMS/mcl_sculk/init.lua: sculk spread - does it matter? already lists mcl_core:dirt explicitly, so could list all.
MAPGEN/mcl_structures/api.lua: IMHO its fine to replace muddy mangrove dirt in foundations (plus, there is a rewrite of this anyway).
MAPGEN/mcl_structures/geode.lua: probably never happens? and matches the purpose of group:dirt?
MAPGEN/mcl_structures/jungle_temple.lua: different biome and matches the purpose of group:dirt?
MAPGEN/mcl_structures/pillager_outpost.lua: different biome and matches the purpose of group:dirt?
MAPGEN/mcl_structures/ruined_portal.lua: probably is fine and matches the purpose of group:dirt?
MAPGEN/mcl_structures/woodland_mansion.lua: different biome and matches the purpose of group:dirt?
MAPGEN/mcl_terrain_features/init.lua: lavapool - probably is fine, the pool replaces it with a stone boundary.
MAPGEN/mcl_terrain_features/init.lua: water_lake - this one may be most "problematic". The code seems to rely on there not being dirt in mangrove swamps, but only mud; but OTOH the muddy mangrove roots is supposed to be mud + mangrove roots?
MAPGEN/tsm_railcorridors/init.lua: mineshafts could generate below

So which way should we go, special case in the if, add another group, add dirt to muddy mangrove?

ITEMS/mcl_core/functions.lua: different biome, and matches the purpose of group:dirt? ITEMS/mcl_sculk/init.lua: sculk spread - does it matter? already lists mcl_core:dirt explicitly, so could list all. MAPGEN/mcl_structures/api.lua: IMHO its fine to replace muddy mangrove dirt in foundations (plus, there is a rewrite of this anyway). MAPGEN/mcl_structures/geode.lua: probably never happens? and matches the purpose of group:dirt? MAPGEN/mcl_structures/jungle_temple.lua: different biome and matches the purpose of group:dirt? MAPGEN/mcl_structures/pillager_outpost.lua: different biome and matches the purpose of group:dirt? MAPGEN/mcl_structures/ruined_portal.lua: probably is fine and matches the purpose of group:dirt? MAPGEN/mcl_structures/woodland_mansion.lua: different biome and matches the purpose of group:dirt? MAPGEN/mcl_terrain_features/init.lua: lavapool - probably is fine, the pool replaces it with a stone boundary. MAPGEN/mcl_terrain_features/init.lua: water_lake - this one may be most "problematic". The code seems to rely on there not being dirt in mangrove swamps, but only mud; but OTOH the muddy mangrove roots is supposed to be mud + mangrove roots? MAPGEN/tsm_railcorridors/init.lua: mineshafts could generate below So which way should we go, special case in the if, add another group, add dirt to muddy mangrove?

What about group:soil?

What about group:soil?
Author
Member

soil is where you can plant on; but this is about the neighbor floor where pumpkin and melon fruit end up. This does not have to be soil.

soil is where you can plant on; but this is about the neighbor floor where pumpkin and melon fruit end up. This does not have to be soil.
Member

I advocate for a separate group for this. IMO something like allow_growth would fit well.

I advocate for a separate group for this. IMO something like `allow_growth` would fit well.
kno10 marked this conversation as resolved
Author
Member

@kneekoo

  • add the game time to a newly planted crop, as metadata
  • when the crop reaches its final stage, output the time difference in the log

I probably would not leave this in the codebase, but this should be easy to do for development. One could also just measure second stage to final growth and interpolate the time of the first stage.

@kneekoo > - add the game time to a newly planted crop, as metadata > - when the crop reaches its final stage, output the time difference in the log I probably would not leave this in the codebase, but this should be easy to do for development. One could also just measure second stage to final growth and interpolate the time of the first stage.
Contributor

I probably would not leave this in the codebase, but this should be easy to do for development. One could also just measure second stage to final growth and interpolate the time of the first stage.

Yes, this wouldn't make sense to be merged. Once we get the timing right, it can be removed.

> I probably would not leave this in the codebase, but this should be easy to do for development. One could also just measure second stage to final growth and interpolate the time of the first stage. Yes, this wouldn't make sense to be merged. Once we get the timing right, it can be removed.
Author
Member

Regarding growth, there is a mismatch between "game time" and the "clock time" (already used before) computed by get_intervals_counter.
E.g., the fastest and slowest wheat (with optimal planting)
plant_wheat from first to last stage: gametime 84092 clock time 136188.95001411

This is likely what is accelerating plant growth.

Regarding growth, there is a mismatch between "game time" and the "clock time" (already used before) computed by `get_intervals_counter`. E.g., the fastest and slowest wheat (with optimal planting) plant_wheat from first to last stage: gametime 84092 clock time 136188.95001411 This is likely what is accelerating plant growth.
Author
Member

The catch-up logic is not using the same time scale as the others:
local time_multiplier = time_speed > 0 and (86400 / time_speed) or 0
This causes plants to grow too fast AFAICT.
I think I will remove the time speed support - we have a separate setting for controlling growth speed.
Reacting to a dynamic change of the time_speed setting properly would need the ABMs to be adjusted accordingly, and I am not sure if players that set time_speed to 0 want to stop plant growth, too. Or whether they just want to disable the day-night cycle.
A separate control is probably better, what do you think?
We could have the grow speed to default to "time_speed / 72", but it will still not react to on the fly changes.

The catch-up logic is not using the same time scale as the others: `local time_multiplier = time_speed > 0 and (86400 / time_speed) or 0` This causes plants to grow too fast AFAICT. I think I will remove the time speed support - we have a separate setting for controlling growth speed. Reacting to a dynamic change of the `time_speed` setting properly would need the ABMs to be adjusted accordingly, and I am not sure if players that set `time_speed` to 0 want to stop plant growth, too. Or whether they just want to disable the day-night cycle. A separate control is probably better, what do you think? We could have the grow speed to default to "time_speed / 72", but it will still not react to on the fly changes.
Author
Member

Expected average growth times for optimal setups now:

  • beetroot: 913s (15 min) in optimal layout and water
  • carrot: 1421s (23-24 min) in optimal layout and water
  • wheat: 1421s (23-24 min) in optimal layout and water
  • potatos: 1421s (23-24 min) in optimal layout and water

Factor for worse setups:

  • alternating rows, wet: 1 to 1.67 (edges 1.33, corners 1.67)
  • single crop, wet: 2 to 2.67
  • alternating rows, dry: 2.33 to 3.67
  • single crop, dry: 4.67 to 7.33

To have, e.g., 80% grown fully you will need to wait longer than the average. I guess that something like 35 minutes will be good for an "optimal" field, and maybe one hour for harvesting single-crop fields?

Growth speeds can be increased with a user setting now.

screenshot_20241016_004841.png

Expected *average* growth times for optimal setups now: - beetroot: 913s (15 min) in optimal layout and water - carrot: 1421s (23-24 min) in optimal layout and water - wheat: 1421s (23-24 min) in optimal layout and water - potatos: 1421s (23-24 min) in optimal layout and water Factor for worse setups: - alternating rows, wet: 1 to 1.67 (edges 1.33, corners 1.67) - single crop, wet: 2 to 2.67 - alternating rows, dry: 2.33 to 3.67 - single crop, dry: 4.67 to 7.33 To have, e.g., 80% grown fully you will need to wait longer than the average. I guess that something like 35 minutes will be good for an "optimal" field, and maybe one hour for harvesting single-crop fields? Growth speeds can be increased with a user setting now. ![screenshot_20241016_004841.png](/attachments/b12c6a7d-44d2-4685-ad94-a0c12ce5b35b)
Author
Member
  • IMHO we can omit the new LBM, because it only removes metadata from crops - and most crops get replaced frequently anyway. Only gourd stems (pumpkins, melons) may retain the metadata longer, and its not that much metadata, it probably does not matter
  • I still do not like the catch-up logic. If a crop is unlucky and does not receive random ticks, it probably should not catch up later. I'd rather move the catch-up logic to the LBM only, essentially compute the number of missed ticks from on the LBM dtime_s (maybe I can look up the distribution to add some randomness). This will make the ABM even more lightweight, and we can also drop the metadata when the node was last ticked.
- [x] IMHO we can omit the new LBM, because it only removes metadata from crops - and most crops get replaced frequently anyway. Only gourd stems (pumpkins, melons) may retain the metadata longer, and its not that much metadata, it probably does not matter - [x] I still do not like the catch-up logic. If a crop is unlucky and does not receive random ticks, it probably should not catch up later. I'd rather move the catch-up logic to the LBM only, essentially compute the number of missed ticks from on the LBM `dtime_s` (maybe I can look up the distribution to add some randomness). This will make the ABM even more lightweight, and we can also drop the metadata when the node was last ticked.
the-real-herowl reviewed 2024-11-02 20:02:05 +01:00
the-real-herowl left a comment
Owner

Code otherwise looks good.

Needs testing only and probably would be fine for 0.88, especially that there are other PRs with uncertain status yet in the milestone.

(I don't expect major issues here)

Code otherwise looks good. Needs testing only and probably would be fine for 0.88, especially that there are other PRs with uncertain status yet in the milestone. (I don't expect major issues here)
@ -28,0 +30,4 @@
local ndef = minetest.registered_nodes[minetest.get_node(n).name]
local soil = ndef and ndef.groups.soil
if soil and soil >= 2 then
local m = (soil > 2 or (soil == 2 and minetest.get_meta(n):get_int("wet") > 0)) and 3 or 1

Is this meta field private (while we're at it)?

Is this meta field private (while we're at it)?
Author
Member

Should not be used/messed with by anyone else. I assume it should be named _vl_farming:wet then or something like this?

Should not be used/messed with by anyone else. I assume it should be named `_vl_farming:wet` then or something like this?
kno10 marked this conversation as resolved
@ -361,0 +307,4 @@
or (dir == 3 and vector.offset(stempos, 0, 0, 1))
or vector.offset(stempos, 0, 0, -1)
if minetest.get_node(neighbor).name ~= "air" then return end -- occupied
-- check for suitable floor: grass, dirt, or soil

Actually, why are other blocks unsuitable? Logically, stuff like all kinds of stone and wood should be fine too.

Actually, why are other blocks unsuitable? Logically, stuff like all kinds of stone and wood should be fine too.
Author
Member

According to https://minecraft.wiki/w/Melon#Farming:

fail if the chosen adjacent block is not empty or the block beneath is not an appropriate block (dirt, coarse dirt, rooted dirt, grass block, farmland, podzol, mycelium, moss block, mud or muddy mangrove roots)

But from my understanding of biology, neither a pumpkin nor a melon requires the floor underneath the fruit to be natural. It just kind of makes the farm more "realistic" if you surround your pumpkin plants with dirt and not stone.
IMHO we can divert from MC here and allow any walkable node underneath.
(Which would also resolve the open discussion on whether we should make muddy mangrove roots "dirt", or whether we should add a group of suitable nodes)

According to <https://minecraft.wiki/w/Melon#Farming>: > fail if the chosen adjacent block is not empty or the block beneath is not an appropriate block (dirt, coarse dirt, rooted dirt, grass block, farmland, podzol, mycelium, moss block, mud or muddy mangrove roots) But from my understanding of biology, neither a pumpkin nor a melon requires the floor underneath the fruit to be natural. It just kind of makes the farm more "realistic" if you surround your pumpkin plants with dirt and not stone. IMHO we can divert from MC here and allow any `walkable` node underneath. (Which would also resolve the open discussion on whether we should make muddy mangrove roots "dirt", or whether we should add a group of suitable nodes)

Agreed.

Agreed.
kno10 marked this conversation as resolved
@ -120,3 +89,1 @@
meta:set_int("wet", wet-1)
end
end
meta:set_int("wet", wet - 1)

Seems like the field is not private... and there's no reason for it not to be, right?

Seems like the field is not private... and there's no reason for it not to be, right?
kno10 marked this conversation as resolved
the-real-herowl modified the milestone from 0.89.0 to 0.88.0 – Back on Track 2024-11-02 20:02:41 +01:00

Is #4649 incorporated here?

Is #4649 incorporated here?
Author
Member

Is #4649 incorporated here?

Kind of: this replaces the "intervals" catch-up logic because it was flawed anyway, so the typo is also removed.

> Is #4649 incorporated here? Kind of: this replaces the "intervals" catch-up logic because it was flawed anyway, so the typo is also removed.
kno10 force-pushed pumpkin-melon-growth-1 from b9fcd16b11 to 590606b667 2024-11-02 21:30:24 +01:00 Compare
Author
Member

@the-real-herowl Rebased + incorporated the feedback, meta is private now.

@the-real-herowl Rebased + incorporated the feedback, meta is private now.
Author
Member

Regarding the "wet" meta attribute. I wonder if we should also just drop this altogether.
I think the case where nearby water is removed can largely be ignored. But when a node was turned wet by rain, it will retain the "wet" property for a bit longer. While this makes it marginally more realistic (typically, rain alone is not quite enough...), this probably does not affect actual plant growth gameplay much, and no player will rely on rain. Visually, they will also become dry on the next tick. So at best, it will be noticeable by someone benchmarking the benefit of rain...
Oh, and if a player is changing the water placement, it will dry up slower; but again players will likely not take that long to relocate water supply to notice...
How about if we further simplify the code and use a binary wetness only?

Regarding the "wet" meta attribute. I wonder if we should also just drop this altogether. I think the case where nearby water is removed can largely be ignored. But when a node was turned wet by rain, it will retain the "wet" property for a bit longer. While this makes it marginally more realistic (typically, rain alone is not quite enough...), this probably does not affect actual plant growth gameplay much, and no player will rely on rain. Visually, they will also become dry on the next tick. So at best, it will be noticeable by someone benchmarking the benefit of rain... Oh, and if a player is changing the water placement, it will dry up slower; but again players will likely not take that long to relocate water supply to notice... How about if we further simplify the code and use a binary wetness only?
kno10 added 1 commit 2024-11-03 00:03:02 +01:00

Another question: why haven't we been using param2 for this?

Another question: why haven't we been using param2 for this?
Author
Member

param2 would likely have been another option. I guess the meta was chosen because it was already used for the old average light level anyway.

param2 would likely have been another option. I guess the meta was chosen because it was already used for the old average light level anyway.
Author
Member

I pushed this further simplification last night, seems to work fine.
Feel free to pick all but the 'Remove "wet" metadata altogether' commit if you disagree.
Now farming does not use meta anymore at all.

I pushed this further simplification last night, seems to work fine. Feel free to pick all but the 'Remove "wet" metadata altogether' commit if you disagree. Now farming does not use meta anymore at all.
First-time contributor

Seems to be working great from my testing. As an average player there are no obvious bugs.

Seems to be working great from my testing. As an average player there are no obvious bugs.
the-real-herowl approved these changes 2024-11-10 01:58:59 +01:00
the-real-herowl removed the
Testing / Retest
label 2024-11-10 01:59:18 +01:00

We may want to return to some "wetness" mechanics one day, but for now I don't see much benefit in it.

I haven't found any issues while testing, and others testing it besides me didn't report anything either.

We may want to return to some "wetness" mechanics one day, but for now I don't see much benefit in it. I haven't found any issues while testing, and others testing it besides me didn't report anything either.
the-real-herowl merged commit 2b7b7f1872 into master 2024-11-10 02:11:40 +01:00
kno10 deleted branch pumpkin-melon-growth-1 2024-11-10 11:37:38 +01:00
Sign in to join this conversation.
No reviewers
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#4681
No description provided.