-
Notifications
You must be signed in to change notification settings - Fork 421
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
Skip binding references on SimpleRegistry#freeze #4123
base: 1.21.1
Are you sure you want to change the base?
Conversation
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) |
For reference, this is impacting a mod of mine in 2 ways:
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. |
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. |
@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 |
I bet I can produce a test that would have previously failed using |
@modmuss50 Added 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. |
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 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. 🤷 |
I'm seeing compiler errors on |
I think its becuase you didnt have the latest fixes and forked from a broken state, I have just merged, should fix that.
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. |
@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. |
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
atvalueToEntry.forEach
whenever a new value has been assigned to an existing key. The issue is thatSimpleRegistry#valueToEntry
is an instance ofIdentityHashMap
, which does not guarantee that itsforEach
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 aLinkedHashMap
, 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
atregisterBlocks
, which confirms that this bind operation was unnecessary.