Improve plant growth system, add moisture level #4681
No reviewers
Labels
No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
feature request
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: VoxeLibre/VoxeLibre#4681
Loading…
Reference in New Issue
No description provided.
Delete Branch "kno10/VoxeLibre:pumpkin-melon-growth-1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
a023de65b3
tob552dacb15
b552dacb15
to02202c65b2
02202c65b2
to8cbe01ed81
8cbe01ed81
tod5aaa8faae
d5aaa8faae
to6bdc8aa409
WIP: moisture level for growing speedsto Improve plant growth system, add moisture level6bdc8aa409
to061af6a76a
@kneekoo maybe you can help test this?
They grow a bit too fast. One beetroot plant matured so fast, that the farmland under it didn't even manage to get hydrated.
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.
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.
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.
10dc367dad
tobbb8036fbf
@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.
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.
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?
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
IIRC
ndef.groups
can be nil too (=> potential crash because of nil indexing)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.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.@ -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
Isn't
soil > 2 or soil == 2
equivalent tosoil >= 2
? And besides, this is already checked in theif
statement above.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.
@ -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
Why can't we just return nil here?
Trying to provoke a more meaningful error message. But it does not print the string, we might as well just use nil, yes.
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.
@ -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
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
.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 tofalse
.@ -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
Why reference MC in the code?
(Also, this change might be up to discussion)
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.@ -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,
Nitpick: inconsistent linebreaks.
readability over automatic code formatting. Fold short lines that are trivial.
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.
@ -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
Same concern.
@ -416,0 +386,4 @@
})
-- The average light levels were unreliable
-- LBM added in fall 2024
Are we giving individual code lines dates now? Shouldn't
git blame
fit these purposes?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.
That's, of course, if it will be removed. XD
@ -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
What's the reasoning behind putting
or nil
here?emphasize that the value might be nil, and we need to handle nil below.
@ -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},
Explanation?
https://minecraft.wiki/w/Tutorials/Pumpkin_and_melon_farming
Line 329 would otherwise need special treatment for Muddy Mangrove:
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.
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: 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?
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.
I advocate for a separate group for this. IMO something like
allow_growth
would fit well.@kneekoo
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.
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.
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 settime_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.
Expected average growth times for optimal setups now:
Factor for worse setups:
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.
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.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)?
Should not be used/messed with by anyone else. I assume it should be named
_vl_farming:wet
then or something like this?@ -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.
According to https://minecraft.wiki/w/Melon#Farming:
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.
@ -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?
Is #4649 incorporated here?
Kind of: this replaces the "intervals" catch-up logic because it was flawed anyway, so the typo is also removed.
b9fcd16b11
to590606b667
@the-real-herowl Rebased + incorporated the feedback, meta is private now.
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?
Another question: why haven't we been using param2 for this?
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.
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.
Seems to be working great from my testing. As an average player there are no obvious bugs.
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.