Fix cannot load world after uninstalling dimension mod/datapack without breaking world presets. #2856

qouteall · 2023-01-20T03:51:52Z

In 1.18.x, I had two solutions:

Remove non-vanilla dimensions when deserializing as they will be added back later. Was working in 1.18.x. Not working with 1.19 custom presets. Fix the issue of uninstalling a dimension mod or datapack #2078
Make deserialization fail-soft and remove the invalid entries to avoid having issue in the dangling reference check when freezing registry. Fix the issue of unloading dimensions v2 #2088

The first solution was chosen because of simplicity.

In 1.19.3, as I tested, there is no need to manually remove the dangling references to avoid error in freezing process. So the second solution become simpler. This PR removes the first solution and uses the second solution.

…od/datapack, by making deserialization fail-soft, instead of removing non-vanilla dimensions.

qouteall · 2023-01-20T03:55:13Z

I tested

Loading a world that has bumblezone dimension without bumblezone mod. tested on both client side and server side
Loading a world that has lost world datapack dimension(https://www.planetminecraft.com/data-pack/the-lost-world-a-liminal-dimension/) after removing the datapack. tested on client side
That world preset datapack Fabric API breaks Custom Dimensions #2805 (comment) works after relogging. tested on client side

TelepathicGrunt · 2023-01-31T15:58:32Z

And what about dimension datapacks using vanilla chunk generators/biome sources that users remove and fully expected the dimension to be removed?

Vanilla behavior is to continue to try and create the dimension despite the datapack not on. There won’t be any parsing error since it has no modded entries. Yet the vanilla behavior is a bug and unexpected and against user’s wishes.

The nuking behavior at least let users know the dimension is gone when datapack is removed.

I don’t see any way of properly handling dimensions in level.dat file. It’s a painful and waste of time when the real solution is to make bug reports to Mojang and push Mojang to advance their plans of overhauling how dimension data is stored

Situation:

If a user select a preset and tries to load the world afterwards, then that means the level.dat’s dimension data needs to be trusted and read in order to keep the preset alive.

But if the user adds a dimension mod and then later removes it, the level.dat would blow up due to unknown chunk generator types etc. The user expectation is the dimension just gets removed. Not blow up. So the level.dat data can’t be trusted.

And if the user adds a dimension datapack and later removes it, the game still can parse the dimension data and creates it and keeps it persisted against user wishes. No way to know if the dimension datapack is on or not because level.dat is parsed so early before the datapack’s dimensions are read and no way to know if the dimension is removed because it uses valid existing vanilla chunk generators etc. So level.dat data here cannot be trusted either

Conclusion, to fit all use cases, the level.dat file needs to be simultaneously trusted and not trusted at the same time. A paradox with no solution.

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FailSoftMapCodec.java

modmuss50 · 2023-01-31T16:05:04Z

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FailSoftMapCodec.java

+				if (k.get().left().isPresent() && v.get().left().isPresent()) {
+					builder.put(k.get().left().get(), v.get().left().get());
+				} else {
+					// ignore failure


Its a little unclear to me (with only basic knowlage of dfu) what failuire path is taken when a world is missing. May be a good idea to log out when this happens, or this is already handled by one of the other cases?

The failure path is normally that, when it tries to parse a generator, it cannot recognize the generator type, then the failure propagates to the whole dimension information and the whole deserialization fails. In old versions it will still load the world but have the non-overworld dimensions removed. In newer versions it will show a screen that it fails to load and the user can click "safe mode", althought clicking "safe mode" doesn't do anything.
With this PR, when the generator fails to deserialize, it will be ignored.

qouteall · 2023-02-01T01:44:37Z

As @TelepathicGrunt said, with this PR, removing a dimension datapack that only uses vanilla things will not have that dimension removed. This is a rare case (most dimension datapacks use its own things).

It's possible to fix both issues by adding extra data to tell that some dimensions comes from preset and should not be removed before reading level.dat. This data can be stored in an extra file. It's more complex than the current solution (I may make a PR when I have time). Another edge case is that the preset dimension depends on some things that a datapack provides and that datapack gets removed.

qouteall · 2023-02-01T01:51:14Z

I think that Minecraft's world preset design is not good. The world preset can add dimensions but there was already the functionality of adding dimensions. So there is a duplication of functionality. The world preset should only be a list of dimension ids and the datapack should add the relevant dimensions in the old way, then in level.dat it should reference the world preset id.

This reverts commit 0c0642a.

Technici4n · 2023-02-05T12:15:21Z

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FailSoftMapCodec.java

+	 * In this implementation, if one deserialization fails, it will log and ignore.
+	 */
+	@Override
+	public <T> DataResult<Map<K, V>> decode(final DynamicOps<T> ops, final MapLike<T> input) {


How and why is this different from Mojang/DataFixerUpper#55?

I didn't see that PR. Yes this PR probably does similar thing. (The relevant datafixerupper code contains a lot functional abstractions so I am not sure whether that PR works the same as this PR)

I see the difference. In that PR, when one deserialization fails, the result will be in "partial result". In this PR, the result will always be normal result.
The DataResult can be in 3 states:

successful with one result

failed with one partial result

failed with no partial result
I am not sure how vanilla handles the partial result (DFU is very convoluted). As I tested, always yielding successful result works.

Update: In Minecraft 1.19.3, it rejects partial result of dimension data. (LevelStorage.createLevelDataParser uses getOrThrow with allowPartial argument false.) So that PR won't directly solve the issue without extra changes.

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/mixin/dimension/RegistryCodecsMixin.java

Technici4n

Looks great now, good call on adding the instanceof check.

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FailSoftMapCodec.java

…ut breaking world presets. (#2856) * Fix the issue that cannot load world after uninstalling a dimension mod/datapack, by making deserialization fail-soft, instead of removing non-vanilla dimensions. * Fix style. * Fix license. * Small changes to FailSoftMapCodec. * Make FailSoftMapCodec final. * Revert "Make FailSoftMapCodec final." This reverts commit 0c0642a. * Use ModifyVariable and change comments. * Remove unnecessary `equals` (cherry picked from commit 00a2eb1)

Fix the issue that cannot load world after uninstalling a dimension m…

a0d367d

…od/datapack, by making deserialization fail-soft, instead of removing non-vanilla dimensions.

qouteall mentioned this pull request Jan 20, 2023

Fix #2805: Fabric API deleting custom dimensions #2835

Closed

qouteall added 2 commits January 20, 2023 12:21

Fix style.

5f12a7b

Fix license.

75fecf7

Technici4n added bug Something isn't working priority:high High priority PRs that need review and work now. Review these first. labels Jan 20, 2023

apple502j approved these changes Jan 29, 2023

View reviewed changes

LambdAurora added a commit to LambdAurora/quilt-standard-libraries that referenced this pull request Jan 30, 2023

Port FabricMC/fabric#2856 and attempt to add a confirmation screen.

b821808

modmuss50 self-requested a review January 31, 2023 15:55

modmuss50 reviewed Jan 31, 2023

View reviewed changes

qouteall added 3 commits February 1, 2023 20:21

Small changes to FailSoftMapCodec.

c970836

Make FailSoftMapCodec final.

0c0642a

Revert "Make FailSoftMapCodec final."

bbea6c5

This reverts commit 0c0642a.

qouteall requested a review from modmuss50 February 1, 2023 12:53

modmuss50 approved these changes Feb 4, 2023

View reviewed changes

Technici4n reviewed Feb 5, 2023

View reviewed changes

Technici4n suggested changes Feb 5, 2023

View reviewed changes

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/mixin/dimension/RegistryCodecsMixin.java Outdated Show resolved Hide resolved

Use ModifyVariable and change comments.

99617b0

qouteall requested a review from Technici4n February 5, 2023 14:45

Technici4n approved these changes Feb 5, 2023

View reviewed changes

modmuss50 added the last call If you care, make yourself heard right away! label Feb 5, 2023

Juuxel reviewed Feb 5, 2023

View reviewed changes

fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FailSoftMapCodec.java Outdated Show resolved Hide resolved

Remove unnecessary equals

f87e462

qouteall requested a review from Juuxel February 6, 2023 12:15

modmuss50 added the merge me please Pull requests that are ready to merge label Feb 11, 2023

modmuss50 merged commit 00a2eb1 into FabricMC:1.19.3 Feb 13, 2023

Technici4n mentioned this pull request Feb 15, 2023

Fabric API breaks Custom Dimensions #2805

Closed

qouteall mentioned this pull request Jul 22, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cannot load world after uninstalling dimension mod/datapack without breaking world presets. #2856

Fix cannot load world after uninstalling dimension mod/datapack without breaking world presets. #2856

qouteall commented Jan 20, 2023 •

edited

Loading

qouteall commented Jan 20, 2023 •

edited

Loading

TelepathicGrunt commented Jan 31, 2023

modmuss50 Jan 31, 2023

qouteall Feb 1, 2023

qouteall commented Feb 1, 2023 •

edited

Loading

qouteall commented Feb 1, 2023

Technici4n Feb 5, 2023

qouteall Feb 5, 2023

qouteall Feb 5, 2023

qouteall Feb 5, 2023

Technici4n left a comment

Fix cannot load world after uninstalling dimension mod/datapack without breaking world presets. #2856

Fix cannot load world after uninstalling dimension mod/datapack without breaking world presets. #2856

Conversation

qouteall commented Jan 20, 2023 • edited Loading

qouteall commented Jan 20, 2023 • edited Loading

TelepathicGrunt commented Jan 31, 2023

modmuss50 Jan 31, 2023

Choose a reason for hiding this comment

qouteall Feb 1, 2023

Choose a reason for hiding this comment

qouteall commented Feb 1, 2023 • edited Loading

qouteall commented Feb 1, 2023

Technici4n Feb 5, 2023

Choose a reason for hiding this comment

qouteall Feb 5, 2023

Choose a reason for hiding this comment

qouteall Feb 5, 2023

Choose a reason for hiding this comment

qouteall Feb 5, 2023

Choose a reason for hiding this comment

Technici4n left a comment

Choose a reason for hiding this comment

qouteall commented Jan 20, 2023 •

edited

Loading

qouteall commented Jan 20, 2023 •

edited

Loading

qouteall commented Feb 1, 2023 •

edited

Loading