Skip to content

[1.20.4] Improve NeoForge's chunk generator command #364

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

Merged
merged 20 commits into from
Dec 16, 2023

Conversation

TelepathicGrunt
Copy link
Contributor

@TelepathicGrunt TelepathicGrunt commented Dec 10, 2023

Special thanks to Jasmine and Gegy for allowing us to use their Fabric Chunk Pregenerator as a basis for this PR. https://github.com/jaskarth/fabric-chunkpregenerator/tree/master

We will of course need some testing for this.

New commands and changes are:

  • /neoforge generate start <x> <y> <z> <chunkRadius> [progressBar] - Generates a square centered on the given position that is chunkRadius * 2 on each side.

  • /neoforge generate stop - Stops the current generation and displays progress that it had completed.

  • /neoforge generate status - Displays the progress completed for the currently running generation.

  • /neoforge generate help - Displays all the generate commands and the general tip.

  • General tips: If running from a server console, you can run generate in different dimensions by using /execute in neoforge generate...

Fixes: #331

Special thanks to Jasmine and Gegy for allowing us to use their Fabric Chunk Pregenerator as a basis for this PR.
https://github.com/jaskarth/fabric-chunkpregenerator/tree/master

We will of course need some testing for this.

New commands and changes are:

- /neoforge generate <x> <y> <z> <chunkRadius> [progressBar] - Generates a square centered on the given position that is chunkRadius * 2 on each side.

- /neoforge generate stop - Stops the current generation and displays progress that it had completed.

- /neoforge generate status - Displays the progress completed for the currently running generation.

- /neoforge generate help - Displays this message.

- General tips: If running from a server console, you can run generate in different dimensions by using /execute in <dimension> neoforge generate...
@TelepathicGrunt TelepathicGrunt marked this pull request as draft December 10, 2023 19:21
@TelepathicGrunt
Copy link
Contributor Author

Draft as there's some calculations we need to check out with the % as it goes to 101% before completion. Also, should we put a maximum limit on the chunk radius param? Maybe 1000000?

@TelepathicGrunt TelepathicGrunt changed the title Improve NeoForge's chunk generator command [1.20.4] Improve NeoForge's chunk generator command Dec 10, 2023
@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.20 Targeted at Minecraft 1.20 labels Dec 10, 2023
@TelepathicGrunt TelepathicGrunt marked this pull request as ready for review December 11, 2023 00:10
@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Dec 11, 2023

Command used:
/neoforge generate start 0 90 0 32 true

Before generate command is ran:
image

After generate command is ran (scale slightly zoomed in. Squares still covers same area):
image

TPS before running:
image

TPS while running:
image

With the old command code, I could see bees stuttering a bit as they fly around. With this new command code, the bee flying is much smoother.

Copy link
Contributor

@Speiger Speiger left a comment

Choose a reason for hiding this comment

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

I said I would provide a review on a implementation provided.

Overall the implementation is decent.
There is things that could be improved to give it a decent speedup/control/usability.
But this patch isn't really focused on it.
The focus is to fix the previous implementation that was just bad.

There is a few things that should be addressed because I get these reports frequently enough that forge will be basically swamped with reports.

  • First: Chunk Radius is great and it should be kept, but make sure to limit it to something reasonable so if someone types in 20000 and thinks 20000 blocks they are simply not generating for a Literal year.
    After talking to the neoforge team, we came up that the best suitable max radius is 1024 chunks.
  • Second: Don't use public tickets. The likelihood of the pregenerator breaking a mods functionality or a mod breaking the pregenerator is just simply to likely to happen.
    A Dedicated ticket should be provided.
  • Third: Why multiple help messages when minecrafts chat supports new line characters in chat?

Once the first two are addressed i think the commit is worthy of getting into neoforge.
Maybe not the best/fastest implementation, but good enough for the masses.

Edit:
If someone wants to follow the discussion about the review of this pullrequest.
It can be found here

@TelepathicGrunt
Copy link
Contributor Author

Hold off on merge while I look into the queue a bit more. And also look into the memory issue some more

@TelepathicGrunt TelepathicGrunt marked this pull request as draft December 11, 2023 12:58
@TelepathicGrunt TelepathicGrunt marked this pull request as ready for review December 12, 2023 00:58
@TelepathicGrunt
Copy link
Contributor Author

Ok iterator issues should be folly resolved. There's a memory leak with vanilla POI system but that'll be best handled in a separate PR/work that will benefit this generate command as a big side bonus. For now, this generate command does what it says on the tin as its sole focus as external optimizations is out of scope of this PR.

Copy link
Contributor

@Speiger Speiger left a comment

Choose a reason for hiding this comment

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

Yeah, I don't see any problem with this implementation anymore.

All problems I have found are resolved with it.

Only thing that might should be considered with this patch should be maybe #356
Because this pregenerator might benefit from that too.

But that is not that important.
Overall I approve!

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Dec 12, 2023

Added code to now skip already fully generated chunks. Special thanks to Gegy for suggesting this idea!

New speed when in a large already-generated area:

2023-12-12.07-27-45.mp4

Old speed when in a large already-generated area:

2023-12-12.07-30-08.1.mp4

@Speiger
Copy link
Contributor

Speiger commented Dec 12, 2023

Huh 2x improvement, didn't expect that.
Though I wonder how much faster it is for generation if it isn't present...
These solutions don't come for free.
(Which is why i didn't suggest it, since 99% of the people won't use it on expansion)

Speiger

This comment was marked as resolved.

@Matyrobbrt Matyrobbrt merged commit 823e543 into neoforged:1.20.x Dec 16, 2023
@TelepathicGrunt TelepathicGrunt deleted the pregenerator-improvement branch June 12, 2024 22:16
@phit
Copy link
Contributor

phit commented Aug 24, 2024

as a server owner now first seeing this in action on 1.21.1, this is very very nice and such a quality of life improvement, thank you all involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Targeted at Minecraft 1.20 enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pregenerator needs some tweaks
5 participants