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

Use sub chunk request system (Fixes light issues, and flickering) #3460

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

valaphee
Copy link
Contributor

@valaphee valaphee commented Dec 16, 2022

Add support for the sub chunk request system, which fixes light issues, and flickering on world loading.
This also shows lighting issues which will be normally seen in Java Edition, and therefore improve parity.

But I also refactored some variable names in JavaLevelChunkWithLightTranslator to make it easier to understand and more general.

Related issue: #3425

@Kas-tle
Copy link
Member

Kas-tle commented Dec 16, 2022

Would this support packet-based light plugins?

@Camotoy
Copy link
Member

Camotoy commented Dec 16, 2022

To an extent - anything that injects into the chunk packet would show up properly. But anything updated outside of that initial chunk packet would not show up.

@valaphee
Copy link
Contributor Author

valaphee commented Dec 16, 2022

Only uses the sky light data given by Java Edition, block light is calculated automatically, don't know what packet-based light plugins do ^^

@CrystalVortex
Copy link

Wait was I suppose to do something? I don't know how pull requests work lol.

@valaphee
Copy link
Contributor Author

Wait was I suppose to do something? I don't know how pull requests work lol.

Nah, just linked your issue, but may take some time until this gets merged, as it is not really a priority.

@Camotoy
Copy link
Member

Camotoy commented Jan 12, 2023

I don't really like the idea of more caching without a config option to turn it off, which is why I haven't moved forward with this PR for a bit. I concede that having this option around for performance is probably a good idea. Maybe we can revisit using NMS on Spigot, at least for the latest version?
Also, some general nitpicks:

  • We don't use _ in variable names as a differentiator (for example, lightData versus lightData_ - can you change the names to be more descriptive of what they are?
  • Your code in BedrockSubChunkTranslator, especially surrounding light data, has no comments and, while I've never looked into the light data part of chunk code, I have no idea what's going on in that class.

@valaphee
Copy link
Contributor Author

valaphee commented Feb 5, 2023

Added some useful comments

@valaphee
Copy link
Contributor Author

valaphee commented Feb 5, 2023

Could also try to improve the code redundancy a little bit, as it borrows a lot of functions from the normal chunk translator

@valaphee valaphee closed this Apr 29, 2023
@valaphee valaphee deleted the sub-chunk-request branch April 29, 2023 12:39
@valaphee valaphee restored the sub-chunk-request branch June 23, 2024 19:48
@valaphee valaphee reopened this Jun 23, 2024
@valaphee
Copy link
Contributor Author

Memory usage could be optimized by deleting the lightData when the sub chunk is translated

Comment on lines +256 to +261
if (type == BlockEntityType.LECTERN && BlockStateValues.getLecternBookStates().get(blockState)) {
// If getLecternBookStates is false, let's just treat it like a normal block entity
bedrockBlockEntities.add(session.getGeyser().getWorldManager().getLecternDataAt(
session, x + chunkBlockX, y, z + chunkBlockZ, true));
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be now removed, as this particular workaround is no longer needed

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.

None yet

6 participants