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

Issues with the flower spread ABM #2022

Closed
paramat opened this issue Jan 17, 2018 · 20 comments

Comments

Projects
None yet
7 participants
@paramat
Copy link
Member

commented Jan 17, 2018

EDIT: Only issue left is should a plant only spread to a surface node identical to the one it is on? I think probably yes, to stop flora spreading into biomes it is not suitable for.
///////////////////

Flowers mod was added to MTG for mgv6 and the ABM was intended to spread flowers only so was lightweight. Long grasses were then added to the 'flora' group to make the ABM spread those, but still in mgv6 which has a very low density of grasses.

Now we have new mapgens with a new biome system and grasses can be much higher density, and very high densities may be used in some biomes, such as my plans for coastal grassy dune biomes.
The ABM has been shown by the profiler to be surprisingly intensive. For every flora node it is applied to it does 'find nodes in area' for a 9x9x9 node volume.

I suggest the ABM is now being applied to too much, grass, dry grass, junglegrass and ferns are present in high numbers and are not high priority for spreading. Some produce crop seeds but these seeds can then be obtained from the crop itself. I can't see much need for grasses to spread.

Another good reason: 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.

This wouldn't be a problem if we farmed flora using seeds as i have suggested, we could remove the ABM completely.
If some way of spreading grasses and ferns is needed we can come up with another method for those.

@paramat paramat changed the title Remove grasses from flower spread ABM (remove from flora group) Remove grasses, ferns from flower spread ABM (remove from flora group) Jan 17, 2018

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

I don't like this idea of removing grasses, ferns from flower spread, it is one of those things that makes world alive.

@twoelk

This comment has been minimized.

Copy link

commented Jan 17, 2018

It has always been difficult to farm flowers or even grass. In games that have starting from an empty world as theme such as the justtest servers, the skytest games, the waterworld versions of unternull or ice and snow worlds players need a lot of space to generate enough flowers for dyes. There should be farming possabilities for any plant that offers a resource. Besides having a nice garden full of flowers should be possible without raiding the generated countryside of it's beauty.

Indeed I think plains of high grass and lush blooming meadows are two sights somewhat missing in mtg.

So I propose to give grass and alikes a spreading function of it's own, hopefully without adding more load ;-P Also I would suggest to give the shovel more uses. Similar to some bush mods it could be used to move a plant as it currently is, growth stages included, while punching actually destroys the plant and generates the drops which should in my opinion also include flower seeds to be able to multiply them in farming or gardening.

@4w

This comment has been minimized.

Copy link

commented Jan 17, 2018

Instead of removing such an unique feature why not simply blacklist the “intensive biomes”?

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

Eww no, messy. Flora needs to spread in all biomes, a biome may have a hgh density of grass but have a few flowers that you want to spread. Also, biomes have varying densities within them instead of a single density.
I don't think it's unique, the current spread method is crude and problematic, seeds would be lightweight and more interesting for the player.

This is a big issue:

Another good reason: 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.

We could possibly use groups such that a flower node only counts other nearby flowers, grass only counts other grass. we have groups for grass and dry grass already and can add one for ferns, the 'flora' group can be for flowers only as originally.
I can do this as a start together with spreading only to identical surface nodes.

We have to keep the ABM for mods that use it using the 'flora' group for their nodes, but MTG doesn't have to use it.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

Did some profiling in mgv7.
In terms of maximum times, the flower spread ABM is the most intensive ABM by a factor of 2.6 over the next. Max time taken in a server step was 9ms (server step is 90ms).
In terms of average times, the flower spread ABM is the most intensive ABM by a factor of 3.6 over the next. Average time per server step is around 3.5ms.

This ABM was originally present in the flowers mod for flowers only, the spread is tuned to stop at a density of 4 plants per 9x9 area which was the density of mapgen flowers in mgv6. it was not meant to apply to other plants or other biome systems.
This maximum density is now applied to all flora nodes whatever they are and wherever they are regardless of local density variation by noise.

The spread often halts if it reaches a steady-state distribution where there are >3 plants within 9x9 for all plants, sometimes this happens quite quickly.

@sofar any comments?

@MarkuBu

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

It doesn't make sense to count grass as plant. Flowers can grow, regardless of the amount of grass. They can replace grass.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 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.
MTG with grasses removed from flora group: Max 1.7ms Av 0.27ms.
13 times less by averages. Will try to find a dense area of flowers to test.

I could do this:

We could possibly use groups such that a flower node only counts other nearby flowers, grass only counts other grass. we have groups for grass and dry grass already and can add one for ferns, the 'flora' group can be for flowers only as originally.

But i'm hesitating because i feel we should move to using seeds instead.

@paramat paramat self-assigned this Jan 20, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

Compromise proposal:
Keep the flower spread ABM (no seeds for now) but apply it to flowers only, and optimise it in my PR #2016
Remove grasses and ferns from the flora group.

Grasses and ferns don't need to spread because they are not a resource. Kelp and dry shrub don't spread. Green grass and junglegrass are a resource for your first wheat and cotton seeds, after that seeds come from the crop, because of this those grasses are not likely to be harvested much, and are abundant in the world.

Dry grass and ferns are no resource so should certainly be removed from the flora group.

For now though i'll just optimise the ABM with no behaviour change.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2018

Grasses and ferns don't need to spread because they are not a resource.

Meh, that's sad, then add a way to generate them, aka ~~~bone meal~~~ fertilizer >_>

@4w

This comment has been minimized.

Copy link

commented Jan 21, 2018

Meh, that's sad,

I am pretty sure that a mod can simply add grass back to the flora group.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2018

It may not happen, these are only my own opinions. The optimisation PR helps.

@Ezhh

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

I've been slow to reply on this one due to thinking it over, but even though I sort of like the idea of seeds, I don't really like the idea of flowers not spreading by themselves. I've always enjoyed that both flowers and grass spread and grow without my direct intervention, and the world would feel quite a bit more static and dull if this was taken away.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2018

Also, avg 3.5 ms actually seems very fine (tiny) to me, other mods like biomelib go into hundreds easily.
What I'm missing?

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2018

biomelib is a bad example, it's extremely intensive.
1ms is a long time in code, compare 3.5ms to a server step of 90ms. The maximum time profiled was 9ms, so occasionally 10% of server step time was spent on one ABM from the base game.

I don't expect to convice other devs so for now i'll optimise and improve the ABM.

@paramat paramat removed their assignment Jan 24, 2018

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

If grass doesn't spread, it will become a non-renewable and finite resource which is quite a serious nerf.

Maybe just put grass and grass-like nodes into their own group and give them their own ABM, but with a lower frequency and chance to reduce server load.

@paramat paramat changed the title Remove grasses, ferns from flower spread ABM (remove from flora group) Issues with the flower spread ABM Jan 27, 2018

@paramat paramat added Performance and removed Controversial labels Jan 27, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2018

Changed thread title as i expect removing grasses and ferns from the flora group will not be accepted.

@paramat paramat self-assigned this Feb 1, 2018

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

#2016 merged.

Only issue left is should a plant only spread to a surface node identical to the one it is on? I think probably yes, to stop flora spreading into biomes it is not suitable for.

@paramat paramat removed the Performance label Feb 4, 2018

@twoelk

This comment has been minimized.

Copy link

commented Feb 10, 2018

define identical node, you may have different types of dirt with grass for example. some might be added by a mod or some future feature.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2018

Good point, but usually a different surface node is a different biome and decorations are usually intended to be limited to a certain biome. If a decoration is in multiple biomes or on multiple surface nodes then it will already be on both surface types already, so no issue.
I think i will code this.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2018

#2046 merged.

@paramat paramat closed this Feb 17, 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.