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: do light merging later, remove ChunkProcessingPipeline #4879

Draft
wants to merge 2 commits into
base: feat/chunk-ordering-reactor
Choose a base branch
from

Conversation

naalit
Copy link
Contributor

@naalit naalit commented Aug 31, 2021

Contains

Most of the complexity around ChunkProcessingPipeline is because of the need to support chunk tasks which require neighboring chunks. The only task which requires that is light merging, so this PR experiments with moving light merging to after the main chunk processing phase. This makes it so that chunks appear in the world immediately after their generated, but will have their lighting updated when neighboring chunks are generated, instead of waiting for the neighbors to generate to appear at all. It also means ChunkProcessingPipeline can be removed entirely, and replaced with fairly simple usage of the Reactor APIs.

How to test

Try generating a world, loading a world, playing in multiplayer. My multiplayer doesn't really seem to work right now anyway so I haven't tested whether this PR breaks it.

Outstanding before merging

Is there any reason besides light merging to support chunk tasks with multiple requirements? If so, the complexity might be worth it.

@github-actions github-actions bot added the Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity label Aug 31, 2021
@DarkWeird
Copy link
Contributor

DarkWeird commented Aug 31, 2021

It is incorrect. (Parallel processing incorrect too. But i did it)

Main pros:

  1. Light is logical value, which can be used by game logic. Like spawn mobs in the dark.

I try to do this already :(

@keturn
Copy link
Member

keturn commented Sep 8, 2021

@tolziplohu I know you had some questions about this. What should we do with this PR? Put it back in Draft status?

@naalit
Copy link
Contributor Author

naalit commented Sep 11, 2021

@tolziplohu I know you had some questions about this. What should we do with this PR? Put it back in Draft status?

It sounds like if chunks aren't marked ready until light merging happens, that would probably solve the problems @DarkWeird identified. I don't much time currently though, so I'll mark it as a draft for now.

@naalit naalit marked this pull request as draft September 11, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants