Skip to content

Conversation

@codemob-dev
Copy link
Contributor

Removes some redundant code and should make it easier to add different update modes/conditions in the future. (eg. mergeOrReplace to prevent branches from fully replacing leaves)

@codemob-dev codemob-dev moved this to Low Priority in PRs to review Dec 2, 2025
@codemob-dev codemob-dev added the refactor shuffle code around label Dec 2, 2025
@SpellDigger
Copy link

if ur adding flags, we should have them as a one optional argument at the end of the function

@codemob-dev
Copy link
Contributor Author

we should have them as a one optional argument at the end of the function

That isn't the standard for zig, this isn't c.

@SpellDigger
Copy link

standard or not, having to specify 2 non-optional enums as the first 2 arguments of a function just makes it annoying
the fact ur putting the zig standard over making ones life easier is just evil, also im 100% sure ive seen a zig function in std that has flags like i described

@Argmaster
Copy link
Collaborator

I agree that this is ugly as hell.
In general parameterizing functions like this is discouraged in clean code guidelines and would mean function should be split into smaller ones, instead of merging into bigger one.

@SpellDigger
Copy link

I agree that this is ugly as hell. In general parameterizing functions like this is discouraged in clean code guidelines and would mean function should be split into smaller ones, instead of merging into bigger one.

this 🙏
having a function that does it all is ok... as long as its organized in a way that wont make it annoying
but if we are getting forced to slap 2 enums in the first 2 arguments having the functions split is just a better option

@Argmaster
Copy link
Collaborator

Aight, before I actually start thinking, I wanted to link this issue #1342 for sake of information propagation.

@codemob-dev
Copy link
Contributor Author

codemob-dev commented Dec 2, 2025

In general parameterizing functions like this is discouraged in clean code guidelines and would mean function should be split into smaller ones

Maybe, but here you would end up with 6 smaller ones (if we added the mergeOrReplace mode) with a lot of shared code, and one of the call sites gets much simpler when doing this.

@IntegratedQuantum
Copy link
Member

I think we should keep generation and regular update functions separate.
I would even suggest to make them entirely separate structs, since generation should use the fast path more often, in the future that will include e.g. precalculating the chunk palette indices for a newly added block, instead of recalculating them for every single block that's added. In general there should be more bulk options.

@IntegratedQuantum
Copy link
Member

Also, at least for the non-generation version, these functions should probably also be refactored to use the BlockPos instead of world-scale relative positions.

@IntegratedQuantum IntegratedQuantum moved this from Low Priority to In review in PRs to review Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor shuffle code around

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants