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

chore(rendering): migrate chunk mesh generation Flux #4786

Merged
merged 75 commits into from May 13, 2022

Conversation

pollend
Copy link
Member

@pollend pollend commented Jun 23, 2021

This is a rework of the chunk generation logic with Project Reactor. I do away with ChunkMeshUpdateManager and remove pending mesh from Chunk since all the state information is contained within the Observable. I also added a way to sync the mesh updates back onto the main thread so all the mesh data is generated on a background thread and passed back over to the main thread to upload the data to opengl.

Contributes to #4798

@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 Jun 23, 2021
for (int i = 0; i < vertexBuffers.length; i++) {
int id = vertexBuffers[i];
if (id != 0) {
GL15.glDeleteBuffers(id);
Copy link
Member

Choose a reason for hiding this comment

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

Why use GL15 but change other occurrences to GL30?

Copy link
Member Author

Choose a reason for hiding this comment

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

they end up mapping to the same call.

@skaldarnar
Copy link
Member

What's the difference between this and #4783?

@pollend
Copy link
Member Author

pollend commented Jun 24, 2021

What's the difference between this and #4783?

Just trying something different.

@pollend pollend changed the title Refactor/migrate chunk mesh generation flowable rendering(refactor): migrate chunk mesh generation flowable Jun 26, 2021
@pollend pollend marked this pull request as ready for review June 27, 2021 21:29
@skaldarnar
Copy link
Member

skaldarnar commented Jul 3, 2021

This also works quite well, and I don't see any noticeable stuttering or delays. However, the overall performance seems to be lower than of the previous PR, as I can easily reach the border of generated chunk (with setSpeedMultiplier 5).

I tested this together with #4773 in a Metal Renegades world.

@pollend
Copy link
Member Author

pollend commented Jul 3, 2021

This also works quite well, and I don't see any noticeable stuttering or delays. However, the overall performance seems to be lower than of the previous PR, as I can easily reach the border of generated chunk (with setSpeedMultiplier 5).

I tested this together with #4773 in a Metal Renegades world.

try increasing the parallelism. I have it at 4 but we could go with 5/6

@keturn
Copy link
Member

keturn commented Jan 21, 2022

@keturn
Copy link
Member

keturn commented Jan 21, 2022

@github-actions github-actions bot added the Type: Chore Request for or implementation of maintenance changes label Jan 22, 2022
@keturn
Copy link
Member

keturn commented May 7, 2022

Oh boy: it's just over six weeks until this PR's birthday.

Reviewing the comments above, it looks like the blocker here was that it either didn't have tests or the tests it had weren't passing in CI.

Now I've merged #4987 and we have passing tests!

I'm sure there are follow-up things I would have liked to do (following a "make tests to pin down behavior, then change implementation while supported by tests" pattern), but I don't remember any of those things being necessary for this pass.

I think it's time to re-review this, do a little more hands-on testing to make sure merging #4987 didn't foul things up too badly, and maybe we can finally wrap this thing up.

@pollend
Copy link
Member Author

pollend commented May 11, 2022

This looks good to me. I don't see anything wrong per say.

Copy link
Member

@keturn keturn left a comment

Choose a reason for hiding this comment

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

alright, let's do some merging!

@keturn keturn merged commit 789b392 into develop May 13, 2022
@keturn keturn deleted the refactor/migrate-chunk-mesh-generation-flowable branch May 13, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Concurrency Requests, issues, and changes relating to threading, concurrency, parallel execution, etc. Type: Chore Request for or implementation of maintenance changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants