Skip to content

Conversation

Oleg4260
Copy link
Contributor

@Oleg4260 Oleg4260 commented Jan 25, 2025

In my previous pull I only added landuse for quarries and landfills, so there would be stone in quarries, but I did not add ores as said in #330, now I added ores as well

Fixes #330

@Oleg4260
Copy link
Contributor Author

Oleg4260 commented Feb 3, 2025

I just added another feature that can be enabled in the settings. It fills the underground space with stone, I think it can be useful for many users, but it also should be disabled by default because it increases generation time significantly. Only applies when terrain is enabled. Also now there's bedrock at level -64 for both modes.
image
image

Copy link
Contributor

@benjamin051000 benjamin051000 left a comment

Choose a reason for hiding this comment

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

I didn't review the JS but it looks fine. The Rust is fine as well but consider using fill_blocks() to make it a little simpler

Copy link
Contributor

@benjamin051000 benjamin051000 left a comment

Choose a reason for hiding this comment

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

Please squash the last commit into the previous one. You can do this with an interactive rebase (git rebase -i HEAD~2). Do either a squash or a fixup on the last one so that the previous 2 commits get combined

@Oleg4260
Copy link
Contributor Author

Oleg4260 commented Feb 4, 2025

I am not sure if I did it right, let's just hope I didn't break anything...

@Oleg4260 Oleg4260 changed the title Added ores generation in quarries Added ores generation in quarries + other changes Mar 8, 2025
@Oleg4260
Copy link
Contributor Author

Oleg4260 commented Mar 8, 2025

Looks like the checking proccess broke for some reason and I can see it in other pull requests as well. Should I do something with it?

@benjamin051000
Copy link
Contributor

See #378

@Oleg4260
Copy link
Contributor Author

Oleg4260 commented Mar 9, 2025

I'm sorry if this commit wasn't necessary, it's my first time dealing with merge conflicts, should I do something or can the maintainer fix it?

@Oleg4260
Copy link
Contributor Author

Oleg4260 commented Mar 9, 2025

Ok, i think i figured it out. But i feel awkward making a lot of commits

@benjamin051000
Copy link
Contributor

You can squash the extra commits together using git rebase --interactive if you want to.

But there are loads of commits in main that are typos, merges, not actual changes. So it's probably fine, don't worry about it unless @louis-e says otherwise lol 👍

Copy link
Contributor

@benjamin051000 benjamin051000 left a comment

Choose a reason for hiding this comment

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

Unsure about that argument. See #374 where I had to change that, and added unit tests. If #374 gets merged you should add a unit test too to make sure the argument gets parsed correctly

pub terrain: bool,

/// Enable filling ground (optional)
#[arg(long, default_value_t = false, action = clap::ArgAction::SetFalse)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is setfalse what we want here? I don't think this will ever be true.

@louis-e
Copy link
Owner

louis-e commented Mar 23, 2025

Hi there, I have to apologize for the long delay - but looks good to me, thanks a lot for your contribution! :)

@louis-e louis-e merged commit 931f03a into louis-e:main Mar 23, 2025
3 checks passed
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.

[FEATURE] Implement quarries with stone and ore

3 participants