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

Make DFU error-tolerant to mod custom generator types #3213

Merged
merged 4 commits into from
Aug 13, 2023

Conversation

qouteall
Copy link
Contributor

@qouteall qouteall commented Jul 22, 2023

The PR fixes an issue of DFU that, when upgrading a world, it fails to deserialize mod custom generator type when upgrading, then the vanilla dimensions may get lost.

My previous PR about dimension deserialization is different. That PR fixes the issue after uninstalling dimension mod/dimension datapack by making the deserialization fail-soft. That deserialization is for reading data in level.dat. This PR fixes deserialization in DFU, triggered when upgrading world.

In MC, it firstly upgrade the data then deserialize the data. It also does deserialization and serialization during upgrading. The deserialization here means the deserialization during upgrading.

The fix is to change Schema2823 and make its TaggedChoice to ignore unknown chunk generator types.

When testing it, I found that DFU is sensitive to dimension id's hashcode, that's why I added another dimension fabric_dimension:void_earilier_in_hash_map into the test mod. In NbtCompound it uses HashMap so the order in the map is correlated with hashcode. In CompoundListCodec.decode, it deserializes according to the order in the map. And when one entry's deserialization fails, the thing in result will be a failed result. In apply2stable, if one fails, the failure will propagate the the remaining elements (it's how applicative works). If the result is in failed state, the lambda in apply2stable will not execute, so the remaining things will not go to the result even if deserialziation succeededs. The dimension fabric_dimensions:void is after all vanilla dimensions in hash map so it will not trigger this issue. So I added the dimension fabric_dimension:void_earilier_in_hash_map to make this issue manifest.

DFU is so "GREAT".

This only happens with custom chunk generator types (not happening with dimension datapacks). To test this, I added the dimension fabric_dimension:void_earilier_in_hash_map in 1.19.4 branch of fabric api and created a world that contains that dimension in 1.19.4.

TestDimUpgrade.zip

Without this PR, opening that world then it will log Unsupported key: fabric_dimension:void; Unsupported key: fabric_dimension:void and the nether and end dimensions will vanish.

This makes the DFU to not fail when encountering custom generator type. (The PR also make it not fail when encountering custom biome source type in vanilla chunk generator types). It will possibly be compatible with mod custom datafixers for mod custom chunk generator in level.dat (I haven't seen such mod).

About the usage of @Redirect: MixinExtra allows WrapOperation to a call that has been redirected, so there is not much need to replace redirect to other injections (I think other injections are more error-prone than redirect in this case).

@Juuxel Juuxel added the bug Something isn't working label Jul 22, 2023
@Juuxel Juuxel requested a review from a team July 22, 2023 10:04
@qouteall
Copy link
Contributor Author

qouteall commented Aug 9, 2023

The core thing is in TaggedChoiceTypeMixin. TaggedChoiceType contains a map from string to codec. It simply dispatches the codec based on the type field.

All other mixins are for only enabling fail-soft in the specified places. If making all TaggedChoiceType fail-soft is appropriate, this PR can be reduced to only one Mixin.

Copy link
Contributor

@liach liach left a comment

Choose a reason for hiding this comment

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

I think minimizing behavioral impact is more important, so having more mixins is ok.

@liach liach requested review from a team August 9, 2023 14:48
@modmuss50 modmuss50 self-requested a review August 9, 2023 19:25
@modmuss50 modmuss50 added last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge labels Aug 9, 2023
@modmuss50 modmuss50 merged commit 8536527 into FabricMC:1.20.1 Aug 13, 2023
5 checks passed
modmuss50 pushed a commit that referenced this pull request Aug 13, 2023
* Make DFU error-tolerant to mod custom generator types.

* Fix license. Rename mixin.

* Fix license.

* Disable remap in Schema2832Mixin
modmuss50 pushed a commit that referenced this pull request Aug 13, 2023
* Make DFU error-tolerant to mod custom generator types.

* Fix license. Rename mixin.

* Fix license.

* Disable remap in Schema2832Mixin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants