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

Add L-system trees as decorations #14355

Merged
merged 18 commits into from Mar 12, 2024
Merged

Add L-system trees as decorations #14355

merged 18 commits into from Mar 12, 2024

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Feb 6, 2024

Goal of the PR

Resolves #7346

How does it work

It uses the same L-system as minetest.spawn_tree, but it's faster since it uses the engine version of make_ltree, which directly writes it into the VoxelManipulator during map generation and does not trigger a light update.

Roadmap

It's listed at https://dev.minetest.net/TODO.

To do

This PR is Ready for Review.

How to test

Add some Decorations, e.g. in MTG.
(aaa is a good seed for v7)

minetest.register_decoration({
	name = "default:testdeco",
	deco_type = "lsystem",
	place_on = {"default:dirt_with_grass", "default:sand"},
	sidelen = 80,
	fill_ratio = 0.005,
	y_max = 31000,
	y_min = 1,
	treedef = {
		axiom="FFFFFA",
		rules_a="[&&&F][&&&++++F][&&&----F]",
		trunk="default:coral_skeleton",
		leaves="default:permafrost_with_stones",
		angle=30,
		iterations=2,
		random_level=0,
		trunk_type="single",
		thin_branches=true,
	},
})

Also test if alias work:

  • replace default:coral_skeleton with alias_test1,
  • replace default:permafrost_with_stones with alias_test2
  • and add:
minetest.register_alias("alias_test1", "default:sand")
minetest.register_alias("alias_test2", "default:snow")

@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! @ Mapgen labels Feb 6, 2024
@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 7, 2024

In the documentation, I would remove the treedef example as its redundant. Referring to the minetest.spawn_tree function is enough.

@cx384
Copy link
Contributor Author

cx384 commented Feb 8, 2024

I removed the redundant example, but I'm unsure what should be there, since for all other fields there are proper examples in the table. However, a reference to the L-system trees section is probably enough.
Moreover, I added a features flag and rebased.

@SmallJoker SmallJoker self-requested a review February 9, 2024 19:18
@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Feb 14, 2024

A question: Since L-system trees might have some randomness involved, how does this randomness get applied in the mapgen?

Does this guarantee that equal world seeds will generate an identical map with identical L-system trees? If not, that should be fixed. Because I think all mapgen stuff (terrain, biomes, decorations, etc.) should be equal if the world seed is the same.

@cx384
Copy link
Contributor Author

cx384 commented Feb 14, 2024

A question: Since L-system trees might have some randomness involved, how does this randomness get applied in the mapgen?

The position is calculated like for any other decoration, depending on the world seed, and the L-system seed gets calculated using the position. (Or the seed will be static if defined in the tree definition.)

seed = p0.X * 2 + p0.Y * 4 + p0.Z; // use the tree position to seed PRNG

Does this guarantee that equal world seeds will generate an identical map with identical L-system trees?

Yes.

The only concern may be, that two L-system decoration, which get placed at the same position in two different worlds, will look exactly the same. However, this does not happen too often and I doubt anyone would notice.

To fix this, the L-system tree seed could generally be calculated differently by taking the world seed into account (and not just the position), but this is out of scope for this PR, I think.

@cx384
Copy link
Contributor Author

cx384 commented Feb 17, 2024

Rebased.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

PR works what it promises. I noticed that some decorations might generate in the same place, which is ugly and needs fixing by someone in a separate PR later on.

The comments aside - looks good so far. Thank you.

doc/lua_api.md Outdated Show resolved Hide resolved
src/mapgen/mg_decoration.h Outdated Show resolved Hide resolved
src/mapgen/mg_decoration.h Outdated Show resolved Hide resolved
src/script/lua_api/l_env.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_env.h Outdated Show resolved Hide resolved
src/mapgen/mg_decoration.h Outdated Show resolved Hide resolved
@sfan5 sfan5 self-assigned this Feb 29, 2024
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 29, 2024
@cx384
Copy link
Contributor Author

cx384 commented Mar 1, 2024

Thanks for the reviews, SmallJoker and sfan5.
I handled every suggestion, and rebased. (since it didn't compile with the Irrlicht version)
Note that

  • read_deco_lsystem still needs a ndef as parameter since it can't access the one from DecoLSystem,
  • read_tree_def moved to c_content.{cpp,h} and not c_converter.{cpp,h},
  • and tree_def is a shared_ptr now.

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 1, 2024
@SmallJoker SmallJoker self-requested a review March 2, 2024 10:35
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works on my machine.

src/mapgen/treegen.h Outdated Show resolved Hide resolved
src/script/lua_api/l_env.h Outdated Show resolved Hide resolved
src/mapgen/mg_decoration.cpp Outdated Show resolved Hide resolved
src/mapgen/treegen.cpp Outdated Show resolved Hide resolved
src/mapgen/mg_decoration.cpp Outdated Show resolved Hide resolved
cx384 and others added 3 commits March 10, 2024 17:54
Co-authored-by: sfan5 <sfan5@live.de>
Co-authored-by: sfan5 <sfan5@live.de>
src/mapgen/treegen.cpp Outdated Show resolved Hide resolved
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

works

@sfan5 sfan5 merged commit 60810c2 into minetest:master Mar 12, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Mapgen >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add L-system trees as decorations
5 participants