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

Refactor tree generation #335

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benjamin051000
Copy link
Contributor

I'm learning the code base by refactoring small files at a time. Tree generation was not so intimidating, so I started here. 😄

I intend on reducing repetition in this file, making it easier to comprehend and lessening the chance of making a mistake.

What do you think so far? I want to go even further by reducing each of the match arms, since they all kinda do the same thing. I'll get to it tomorrow.

I would probably recommend reviewing this commit-by-commit, since the overall diff will be large.

@wielandb
Copy link
Contributor

Could you take a look at #327 ? Does it interfere with your refactorings? Should I change anything?

@benjamin051000
Copy link
Contributor Author

benjamin051000 commented Jan 25, 2025

@wielandb looks fine! You didn't edit the tree.rs file. The only thing that will need to change is a slight modification to the create_tree signature (x y z are now a Tuple). Otherwise looks fine and should have no problem merging.

Comment on lines 120 to 132
2 => {
// Spruce tree
editor.fill_blocks(SPRUCE_LOG, x, y, z, x, y + 9, z, None, None);
editor.fill_blocks(BIRCH_LEAVES, x - 1, y + 3, z, x - 1, y + 10, z, None, None);
editor.fill_blocks(BIRCH_LEAVES, x + 1, y + 3, z, x + 1, y + 10, z, None, None);
editor.fill_blocks(BIRCH_LEAVES, x, y + 3, z - 1, x, y + 10, z - 1, None, None);
editor.fill_blocks(BIRCH_LEAVES, x, y + 3, z + 1, x, y + 10, z + 1, None, None);
let birch_leaves_fill = [
((-1, 3, 0), (-1, 10, 0)),
((0, 3, -1), (0, 10, -1)),
((1, 3, 0), (1, 10, 0)),
((0, 3, -1), (0, 10, -1)),
((0, 3, 1), (0, 10, 1)),
];
for ((i1, j1, k1), (i2, j2, k2)) in birch_leaves_fill {
editor.fill_blocks(BIRCH_LEAVES, x + i1, y + j1, z + k1, x + i2, y + j2, z + k2, None, None);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does the spruce tree get Birch leaves? Shouldn't it get spruce leaves? Is this intentional?

@benjamin051000
Copy link
Contributor Author

@wielandb Actuallllyyyyyy I might change it more XD But it still shouldn't be a problem because you only call create_tree() in a few spots in your PR so at most you'll only have to change a few lines. 99% of this PR is in tree.rs which you didn't touch.

@Oleg4260
Copy link
Contributor

Oleg4260 commented Jan 25, 2025

It gives compilation error because of bad formatting, you should execute cargo fmt and do another commit

@benjamin051000
Copy link
Contributor Author

@wielandb the API has changed. See Tree::create() for details. It's actually simpler now, because the RNG is handled internally. Should be very easy to change wherever you need in your branch (assuming this gets merged).

@benjamin051000 benjamin051000 marked this pull request as ready for review January 26, 2025 02:12
round takes a parameter which is a reference to an array of patterns.
The patterns are defined at the top of the file as `[(i32, i32, i32)]`.
This commit is a little messy:
- It reduces repitition of the block fill calls and the snow layer calls
- It also replaces previous usage of `y + i` with `y + j` to keep the
  variable conventions the same.
x, y, z -> (x, y, z)

Also some small variable renames
This decouples the data that each tree has with the block-placing
procedure. Now, all the data about a tree is stored in `struct Tree`,
and the `Tree::create()` function handles all paths accordingly.
@wielandb
Copy link
Contributor

@wielandb the API has changed. See Tree::create() for details. It's actually simpler now, because the RNG is handled internally. Should be very easy to change wherever you need in your branch (assuming this gets merged).

Ah, okay! I will mark my PR as a draft until this is (hopefully) merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants