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

"populate noise" step of chunk generation #319

Merged
merged 27 commits into from
Dec 2, 2024

Conversation

kralverde
Copy link
Contributor

@kralverde kralverde commented Nov 20, 2024

This PR implements (most of) the "populate noise" stage of chunk generation

The blender (for smoothing (i think)) and the beardifier (for leaving space for world structures) are not implemented in this PR

This PR changes the default world gen to this noise generation

Checklist

  • Re-implement chunk noise
  • Optimize chunk noise references
  • Look into hot code if chunks take too long to load
  • Add tests for actual chunk noise
  • Code is well-formatted and adheres to project style guidelines: cargo fmt
  • Code does not produce any clippy warnings cargo clippy

@kralverde
Copy link
Contributor Author

The chunk noise generation and population each take like 0.5 seconds, so 1 second per chunk. I'll now start a deep dive into finding optimizations

@kralverde
Copy link
Contributor Author

Chunk gen times are now ~0.1sec which i think is good enough for now

@OfficialKris
Copy link
Contributor

This is a very large PR. I'm just curious, what was the methodology of this implementation?

@kralverde
Copy link
Contributor Author

What do you mean by methodology?

@kralverde
Copy link
Contributor Author

But basically I would look at the source code, write an equivalent in rust, and then write a bunch of unit tests for correctness. It was not fun lol

@kralverde kralverde force-pushed the basic_chunk_generation branch from 5286d88 to 96b79a2 Compare November 23, 2024 00:16
@kralverde kralverde marked this pull request as ready for review November 23, 2024 00:17
@kralverde
Copy link
Contributor Author

kralverde commented Nov 23, 2024

If this PR is merged as-is, the default world generation would become that of the populate noise

@kralverde
Copy link
Contributor Author

kralverde commented Nov 23, 2024

Final bench is ~80msec per chunk on my machine. Can likely be improved but I think its good enough for now

@@ -0,0 +1,158 @@
package com.example.mixin;
Copy link
Owner

Choose a reason for hiding this comment

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

What the hell is this class and why is this in pumpkin-world ?

Copy link
Contributor Author

@kralverde kralverde Nov 24, 2024

Choose a reason for hiding this comment

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

Its code to generate the no_bench_no_blend_0_0.chunk file, a fabric mod

Copy link
Owner

Choose a reason for hiding this comment

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

Why you don't put it into the extractor/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think about that, I'll do that

@kralverde
Copy link
Contributor Author

~60msec/chunk on my machine with the unsafe code

@Q2297045667
Copy link

@kralverde
Copy link
Contributor Author

kralverde commented Nov 25, 2024

Report BUG

"populate noise" step of chunk generation

I don't think this is necessarily relevant to this PR, as this PR does not change any spawning code. It might illustrate an upstream bug though

@kralverde
Copy link
Contributor Author

kralverde commented Nov 25, 2024

There's a regression somewhere in here for terrain gen, don't merge this PR yet.

Isolated the issue to 68949ec likely line 539. I'll make a fix and add a test later today.

@kralverde
Copy link
Contributor Author

kralverde commented Nov 25, 2024

currently benching at ~55msec/chunk now, I think thats adequate for this PR

@kralverde kralverde force-pushed the basic_chunk_generation branch from 19337ba to 2a11487 Compare November 28, 2024 21:48
@kralverde
Copy link
Contributor Author

@Snowiiii should be good to go now I think

@OfficialKris
Copy link
Contributor

Runs great on my computer, very impressive. But why does it have more loc than Minecraft itself?

@kralverde
Copy link
Contributor Author

Runs great on my computer, very impressive. But why does it have more loc than Minecraft itself?

Loc?

@user622628252416
Copy link
Contributor

Runs great on my computer, very impressive. But why does it have more loc than Minecraft itself?

Loc?

lines of code

@OfficialKris
Copy link
Contributor

How are the asset files generated? Are they from minecraft? Is it possible to generate them at compile time? each of them are 3MB and that could be a problem to the git repo if they change often (like version updates).

@kralverde
Copy link
Contributor Author

How are the asset files generated? Are they from minecraft? Is it possible to generate them at compile time? each of them are 3MB and that could be a problem to the git repo if they change often (like version updates).

They're generated from Java, not really able to generate them thru rust... maybe I could write a script to generate and run the tests? But then they couldn't be run with the PR tests.

@Snowiiii
Copy link
Owner

Snowiiii commented Dec 2, 2024

Mostly looks good to me. Only few NIT's but i can improve the code later, i would like to have this merged.
Big Big Thanks @kralverde ❤️

@Snowiiii Snowiiii merged commit 979877f into Snowiiii:master Dec 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants