Skip binding references on SimpleRegistry#freeze

[PersonTheCat]

Registry Order Fix

Description

These changes fix a rare bug where overwritten registry entries get bound to the wrong value.

Explanation

This problem occurs on SimpleRegistry#freeze at valueToEntry.forEach whenever a new value has been assigned to an existing key. The issue is that SimpleRegistry#valueToEntry is an instance of IdentityHashMap, which does not guarantee that its forEach method will run in order of insertion. Thus, occasionally, registry entries can get bound in an unpredictable order. The actual order is based on object identity; however, this order is also not guaranteed.

Design Considerations

One downside to this solution is that any existing mixins wrapping this operation may not get invoked. Thus, there is some potential for breakage with existing mods. However, I am not aware of any such mods and believe this change to be largely safe.

Alternate Strategies

In fixing this issue, I had also considered switching the implementation of valueToEntry to be a LinkedHashMap, which would guarantee order by insertion; however, such a change would alter fundamental systems involving registry operations. In my opinion, the scope of this change is greater than simply skipping that binding in the first place.

Automated Testing

This change is already covered by an existing test in RegistrySyncTest at registerBlocks, which confirms that this bind operation was unnecessary.

[apple502j]

Registry overwriting is a cursed practice that is technically not supported, but is useful anyway... (For reference: vanilla does not re-bind, it occurs at the mixin, L161)

[PersonTheCat]

For reference, this is impacting a mod of mine in 2 ways:

1. Dynamic registry generation based on data pack configurations. Conflicts are expected to be handled by data pack order.
2. World gen changes, such as dynamically modifying noise router settings. IMO, it's probably safer to just re-register the data than attempt to modify it.

Not all of this code is public yet, but it's similar to what Lithostitched does with its Modifier API.

Maybe there are better ways to achieve that, but I have seen mods overwrite registry entries in the past.

[modmuss50]

Would it be possible to expand the testmod to include a case that reproduces the issue this solves? We really do not in anyway support registry replacement, especially for static registries. Dynamic ones might be a bit different though.

[PersonTheCat]

@modmuss50 Yes. This does affect direct modification of dynamic registries. I can reproduce it by creating a simple registry and appending a few values to the same key. The result is inconsistent after calling freeze. Will update the code later.

[PersonTheCat]

I bet I can produce a test that would have previously failed using DynamicRegistrySetupCallback#EVENT. If a value is registered using this event and a second value is bound to the same key from the data packs, the output is inconsistent. This was actually the original error I had encountered. I'll explore that approach instead.

[PersonTheCat]

@modmuss50 Added RegistryOrderTest, which confirms that data pack values always replace values registered by DynamicRegistrySetupCallback#EVENT. Imagine these values are generated and the user has the option to explicitly define them.

The second scenario this was intended to fix was a registry overwrite issue. I was attempting to replace noise router settings instead of modifying them directly (because IMO the data should stay immutable), but it sounds like this scenario is not officially supported. Let me know your thoughts.

[modmuss50]

I really dont like registry overriding like this, what is stopping you from having the "default" values in a datapack? You mention world gen, but is this not all datapack driven anyway?

[PersonTheCat]

I really dont like registry overriding like this, what is stopping you from having the "default" values in a datapack? You mention world gen, but is this not all datapack driven anyway?

What's happening is we have an Injector API which generates multiple entries at a time. For example, we have an OreInjector which generates multiple configured and placed features, as well as updating the relevant biome configs (although we use Fabric Biome Modifications for this part). This is designed to provide a similar experience to one of my previous mods (for older MC versions), while providing better compatibility with new data pack systems.

Since we're modifying registry entries, my opinion is that the safest option is to re-register the data instead of modifying all of it with Mixin accessors.

I understand the reservations. however. Part of my issue is that I'm targeting 3 platforms for this mod and Fabric is the only one requiring a workaround. 🤷

[PersonTheCat]

I'm seeing compiler errors on :fabric-transfer-api-v1:compileJava related to Mixin, but I haven't touched this project. Not sure how to proceed. I am able to build successfully on my machine, but maybe I'm using a different task. Will continue to investigate, but I believe these errors are unrelated.

[modmuss50]

I'm seeing compiler errors on :fabric-transfer-api-v1:compileJava

I think its becuase you didnt have the latest fixes and forked from a broken state, I have just merged, should fix that.

targeting 3 platforms for this mod and Fabric is the only one requiring a workaround

I get that other platforms allow it, but my understanding is that they also support registry replacement and have various reasons to do so. E.g they discoruage Mixins, and have their own wrapper ontop of the registry. While Fabric tries to keep as close to vanilla as possible and we promote using Mixins to alter the registry entry as needed instead of replacing it.

It might be good to get input from other people to see what they think, at the moment I am quite wary about this change and worry about unknown side affects. Worse case you can just have this mixin in your mod and be done with it, it should be fairly compatible if others want to do the same.

[PersonTheCat]

It might be good to get input from other people to see what they think, at the moment I am quite wary about this change and worry about unknown side affects. Worse case you can just have this mixin in your mod and be done with it, it should be fairly compatible if others want to do the same.

@modmuss50 Fair point. If you would consider overrides for data packs only, I suggest adding an explicit test for overrides to static registries. I agree that it's best not to replace blocks and items, but in this case, it's just a data update. If not, Mixin would be an acceptable approach. (It would just take a lot more code)

Edit for anyone else considering this change: IMO adding an explicit test / error whenever a mod tries to do a registry overwrite is a good idea anyway, since it clearly calls out what is and is not supported by the API.