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

Flower spread ABM: Optimise #2016

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@paramat
Copy link
Member

commented Jan 15, 2018

Flower spread ABM: Optimise

Match maximum spread density to maximum mapgen density for flowers.
Place 3 flora nodes at once instead of 1.
Change ABM chance value to 300 to match previous spread rate.
ABM becomes 3 times less intensive.
///////////

screenshot_20180121_152516

^ Testing with interval 1 and starting from 1 flower, explosive spread

The 'flower spread' ABM in the flowers mod is surprisingly intensive according to profiling, see #2022 (comment)
The ABM is intensive because for each ABM 'action' on one plant node it:

  • Does 'find nodes in area' for a 9x9x9 node volume, counting flora nodes, if there are more than 3 no spread occurs.
  • Does 'find nodes in area under air' for the same volume to find surface nodes to possibly spead to.
  • If conditions are met only 1 new plant is placed, after all that.

The idea of this PR is to place 3 new plants at this point instead of just 1, greatly increasing the exponential rate of spread (quadrupling instead of doubling), which will allow us to reduce the 'chance of action' of this ABM. I have reduced the chance by a factor of 3, the ABM becomes 3 times less intensive as well as making spread slightly faster.

The other change is detailed below in #2016 (comment) Flora spreads until it reaches a maximum density, the density was out of date (unchanged since mgv6-only days) and far too low, this PR raises it to match maximum mapgen density of flora.

With slightly faster spread and higher maximum density farming flowers becomes faster and requires less farmed area.

//////////////
For a possible later PR:
I am also considering the idea to restrict spread to surface nodes identical to the surface node of the parent plant, this will stop plants spreading into biomes they are not native to. Let me know if this will break any mods or worlds.

soil_pos[1] = soils[math.random(num_soils)]
soil_pos[2] = soils[math.random(num_soils)]
while soil_pos[2] == soil_pos[1] do
soil_pos[2] = soils[math.random(num_soils)]

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jan 15, 2018

Member

Possible infinite loop if there's only one soil.

This comment has been minimized.

Copy link
@paramat

paramat Jan 16, 2018

Author Member

Well spotted.

This comment has been minimized.

Copy link
@paramat

paramat Jan 20, 2018

Author Member

Avoided this by simplifying and not worrying about duplicated positions.

This comment has been minimized.

Copy link
@paramat

paramat Jan 21, 2018

Author Member

Duplicated positions are unlikely with the typical large areas of soils, avoiding them requires lots of additional code.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

A further change needed: The max density of 4 per 9x9 area is out of date, flowers can be at a higher density. I'll find the highest density and reset the maximum. This will allow more flowers to be placed at once (maybe 3), making the optimisation more effective.

/////////////
Another issue (see the many in #2022):
Flora group nodes do not spread if there are more than 3 of them in the 9x9x9 volume, since grasses count towards the total they are now very often boosting the total over 3 and preventing flowers from spreading when flowers are a valuable resource for dyes.
I might have to have a flora node only count neighbours of it's own kind, a new 'flower' group would be needed for this.
But this can be a later commit.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

local function register_flower(seed, name)
	minetest.register_decoration({
		deco_type = "simple",
		place_on = {"default:dirt_with_grass"},
		sidelen = 16,
		noise_params = {
			offset = -0.02,
			scale = 0.04,
			spread = {x = 200, y = 200, z = 200},
			seed = seed,
			octaves = 3,
			persist = 0.6
		},
		biomes = {"grassland", "deciduous_forest", "floatland_grassland"},
		y_min = 1,
		y_max = 31000,
		decoration = "flowers:"..name,
	})
end

Typical maximum density for one flower type is therefore -0.02 + 0.04 = 0.02.
With 8 flower types: 0.02 * 8 = 0.16 flowers per node (1 flower per 6 nodes).
Over 9 ^ 2 node area: 0.16 * 9 ^ 2 = 12.96 roughly 13 flowers instead of 3.

Now if i make the density limit lower than 13 but have multiple flowers added we can tune this to produce an average maximum of 13.
For example a limit of 11 but place 3 flowers at a time, the final spread step is:
9 + 3 = 12
10 + 3 = 13
11 + 3 = 14
Average result 13.

@paramat paramat force-pushed the paramat:floraspread branch from 0f2c779 to b3563bc Jan 20, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

Updated as detailed, in testing.

@paramat paramat added the Performance label Jan 20, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2018

Profiled ABM time per server step (90ms) in grassland with dense grasses and mid-density flowers.
MTG master: Max 6.5ms Av 3.5ms.
This PR: Max 4.1ms Av 1.4ms.
Still the most intensive ABM but only by a factor of 1.4 over the next (by averages).
Average is almost down to 1/3rd.

Needs retesting after update.

@paramat paramat changed the title Flora spread: Optimise Flower spread ABM: Optimise Jan 21, 2018

@paramat paramat removed the WIP label Jan 21, 2018

if not light or light < 13 or
-- Desert sand is in the soil group
minetest.get_node(soil).name == "default:desert_sand" then
return

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jan 21, 2018

Member

Don't return here. Leave this case empty and set the node in else

This comment has been minimized.

Copy link
@paramat

paramat Jan 21, 2018

Author Member

Ugh a return in a loop, sorry, big mistake.

This comment has been minimized.

Copy link
@paramat

paramat Jan 21, 2018

Author Member

Fixed, testing.

@paramat paramat added the WIP label Jan 21, 2018

@paramat paramat force-pushed the paramat:floraspread branch from b3563bc to 01326c7 Jan 21, 2018

@paramat paramat removed the WIP label Jan 21, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2018

Updated profiling shows no change:

Profiled ABM time per server step (90ms) in grassland with dense grasses and mid-density flowers.
MTG master: Max 6.5ms Av 3.5ms.
This PR: Max 4.4ms Av 1.5ms.
Still the most intensive ABM but only by a factor of 1.5 over the next (by averages).
Average is almost down to 1/3rd.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2018

I'm not going to accept this ABM unchanged, the action alone can take up to 9ms, 10% of a server step, it's also insane as it is (see the issue) and i would prefer it removed completely and we use seeds.
However that will likely not be accepted so i will compromise and optimise it. There is no harm in placing 3 instead of 1 at a time and this makes it 3 times less intensive as well as increasing the speed of farming flowers.

@benrob0329

This comment has been minimized.

Copy link

commented Jan 27, 2018

I'd be fine with using seeds.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2018

I suggested seeds for flowers a year or so ago and most MTG devs were opposed =)

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2018

@paramat paramat added WIP and removed WIP labels Jan 30, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2018

Time to spread from 1 plant to 128 with interval 1s:

PR placing 1 plant at a time and with chance 96 (previous behaviour but with PR maximum density limit)
16m05s

PR with chance 288
11m59s

So the PR results in faster spread while being 3 times less intensive.

@SmallJoker SmallJoker force-pushed the minetest:master branch from 119c55d to 496a1a2 Jan 31, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2018

There have been requests for faster flower farming, but should the spread rate be equal to before? I can tune it to be. There is a maximum density, so flora will reach this maximum density a little sooner but the eventual result is the same.

@JurajVajda

This comment has been minimized.

Copy link

commented Feb 1, 2018

Since you already made the performance improvement using the same spreading speed i would say keep the spreading speed as it was before - not that fast anyway. Other option could be slower spreading with addition of bone meal in the near future.

@Ezhh

This comment has been minimized.

Copy link
Member

commented Feb 1, 2018

I would prefer no big increase (a little is certainly fine). People also have said they'd like fertiliser of some kind added, which could be reserved for nice speed increases.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2018

Ok good a decision is made, will tune.

@paramat paramat added the WIP label Feb 1, 2018

@paramat paramat force-pushed the paramat:floraspread branch from 01326c7 to 1c7691c Feb 1, 2018

@paramat paramat removed the WIP label Feb 1, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2018

After lengthy tuning.

Time to spread from 1 plant to 128 in empty flat grassland with interval 1s:
PR with chance 300
13m29s
(MTG master is 16m05s)
19% faster spread.

I tried chance values over 300 to get a closer match but these made the first spread action far too rare, i had to abort the tests due to nothing happening. Close enough.

@JurajVajda

This comment has been minimized.

Copy link

commented Feb 1, 2018

Sounds good, thanks!

@paramat paramat added the WIP label Feb 4, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

Testing by Ezhh has shown density becomes higher than 13 per 9x9, will tune.

@Ezhh

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

9*9 areas for testing show far higher than the anticipated average spread:

ekkflowers

We're getting up to 23 flowers per 9*9, which I feel is far too dense.

This was with interval = 1, and no other changes.

@paramat paramat force-pushed the paramat:floraspread branch from 1c7691c to 193b8db Feb 4, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

Average maximum density per 9x9 seemed to be 21.
So i multiply the coded limit of 11 by 13 / 21, result = 7.
Used 7 here:

+	-- Maximum mapgen flower density is 13 per 9x9 area, the limit below is
+	-- tuned by trial and error to result in the same maximum density.
+	-- Setting this limit theoretically does not work in practice.
+	if #minetest.find_nodes_in_area(pos0, pos1, "group:flora") > 7 then
 		return
 	end

Ran spread until placing almost stopped (max density).
Then added code to print number per 9x9 (already calculated in the code) and print a running average of that.
Result:

flora in 9x9 11
av flora in 9x9 13.617611741161
flora in 9x9 12
av flora in 9x9 13.617395944504
flora in 9x9 11
av flora in 9x9 13.617046818727
flora in 9x9 11
av flora in 9x9 13.616697786076
flora in 9x9 12
av flora in 9x9 13.616482197626
flora in 9x9 13
av flora in 9x9 13.6164
flora in 9x9 18
av flora in 9x9 13.61698440208
flora in 9x9 15
av flora in 9x9 13.617168754999
flora in 9x9 13
av flora in 9x9 13.617086498734
flora in 9x9 12
av flora in 9x9 13.616871002132

Close, will also try a limit of 6 but expect that will be too low.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

The same with a coded limit of 6:

flora in 9x9 7
av flora in 9x9 12.024097874444
flora in 9x9 12
av flora in 9x9 12.024094896824
flora in 9x9 14
av flora in 9x9 12.024339016555
flora in 9x9 11
av flora in 9x9 12.024212476838
flora in 9x9 9
av flora in 9x9 12.023838932806
flora in 9x9 13
av flora in 9x9 12.02395949117
flora in 9x9 11
av flora in 9x9 12.023833045196
flora in 9x9 14
av flora in 9x9 12.024077046549
flora in 9x9 10
av flora in 9x9 12.023827160494
flora in 9x9 15
av flora in 9x9 12.024194543883

13.6 is closer to 13 than 12, so 7 is best, and the PR is already 7.

@paramat paramat removed the WIP label Feb 4, 2018

@Ezhh

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

Much better now. Thanks for adjusting. 👍

@Ezhh Ezhh added the One approval label Feb 4, 2018

@paramat paramat force-pushed the paramat:floraspread branch from 193b8db to 3831cf4 Feb 4, 2018

Flower spread ABM: Optimise
Match maximum spread density to maximum mapgen density for flowers.
Place 3 flora nodes at once instead of 1.
Change ABM chance value to 300 to match previous spread rate.
ABM becomes 3 times less intensive.

@paramat paramat force-pushed the paramat:floraspread branch from 3831cf4 to 5aa5b5d Feb 4, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

Updated comment.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

@paramat paramat closed this Feb 4, 2018

@paramat paramat deleted the paramat:floraspread branch Feb 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.