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

Fix post-flatterning textures #256

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Conversation

nathanruiz
Copy link
Collaborator

This change updates the textures to use the 1.19.2 release. I've tested this on a number of versions, including 1.8.9, 1.12.2, 1.13.2, and 1.16.5. This covers pre and post flattening.

The largest change is the rework of the define_blocks macro. Rather than including data, offset, and offsets as a method of resolving the block ID, I've changed it to just store information about a block with different states, and then have a separate mapping for the IDs at different versions. The benefit of this is we can generate both the define_blocks block, and the mappings from information in https://github.com/PrismarineJS/minecraft-data. I've included a tool called generate-blocks that will generate those new structures.

For all post-flattening versions, this mapping is a const array of each block state in order. This will add ~1.4MiB to the compiled size, but will allow for very fast lookups from IDs. For the pre-flattening versions, I instead made a match block since it needs to do a lookup on both block ID and blockstate.

The pre-flattening file (blocks/src/versions/legacy.rs) was the only mapping file that can't currently be automatically updated. I generated the initial version from minecraft-data, but then needed to make manual changes to get it to work correctly. I've done lots of testing on 1.8.9 and 1.12.2 to try to catch any issues. While doing this, I found a few bugs in the original code, particularly around update_state that are now fixed.

After looking into the update_state structure, it seems like it is only required for pre-flattening versions, since post-flattening versions just send multiple block updates in those cases. A few cases I tested for this were top half of doors, chests/double chests, stairs, pumpkin stems, and redstone. Pre 1.13, some of these states aren't sent by the server, but are intended to be calculated on the client side. After 1.12, these states are calculated on the server side. Since it's no longer required in 1.13+, I've disabled calls to update_state on those versions.

@nathanruiz nathanruiz force-pushed the textures branch 2 times, most recently from d56a432 to fd05852 Compare January 9, 2023 08:10
Copy link
Member

@terrarier2111 terrarier2111 left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review and many repetitions at the very top.
note that i didn't look at the auto-genned files because that would have been an insane amount of work.

blocks/src/lib.rs Outdated Show resolved Hide resolved
blocks/src/lib.rs Show resolved Hide resolved
blocks/src/lib.rs Show resolved Hide resolved
blocks/src/versions/legacy.rs Outdated Show resolved Hide resolved
blocks/src/versions/legacy.rs Outdated Show resolved Hide resolved
blocks/src/versions/legacy.rs Outdated Show resolved Hide resolved
blocks/src/versions/legacy.rs Outdated Show resolved Hide resolved
src/model/liquid.rs Show resolved Hide resolved
let new = current.update_state(self, bp);
if current != new {
self.set_block_raw(bp, new);
if self.protocol_version < 404 {
Copy link
Member

Choose a reason for hiding this comment

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

why don't we do this in later versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the update_state issue I mentioned in the description. Before the flatterning, the client was expected to make changes to blocks itself. For example, with doors, the server would only send updates for one half when it was opened or closed, and the client was responsible for updating the other half locally.

After the flatterning, the server sends updates for both halves of the door, so we don't need to update the block around it locally.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining this rationale?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated this

// Restore old lighting
self.set_sky_light(bp, sky_light);
self.set_block_light(bp, block_light);
if self.protocol_version < 404 {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated this

@nathanruiz
Copy link
Collaborator Author

@terrarier2111 I'm not sure how to fix the build issue. It seems like it's related to #255.

@terrarier2111
Copy link
Member

@terrarier2111 I'm not sure how to fix the build issue. It seems like it's related to #255.

I'll look into this in the upcoming days.

@terrarier2111
Copy link
Member

So this should now be mergable, right?

@terrarier2111 terrarier2111 merged commit d27a271 into Lea-fish:main Feb 3, 2023
@nathanruiz nathanruiz deleted the textures branch February 3, 2023 23:58
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.

2 participants