Add system for Dynamic Registry Flags #330
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Currently, only one flag, `OPTIONAL`, exists. - Modified DynamicRegistrySyncMixin in order to implement these flags. Due to the Redirect mixin header used to implement the flag filtering being taken directly from FAPI, the header of DynamicRegistrySyncMixin was changed to mention FabricMC copyright, in accordance with QSL contribution rules in CONTRIBUTING.md
- Modify DynamicRegistryFlagManager to use Identifiers over RegistryKeys internally, in order to avoid generic-related jank. - Fix mixin ported from FAPI by correcting method target string to use Quilt Mappings. - Implement very basic tests.
ix0rai
added
enhancement
|
This should probably be considered a blocking addition for FAPI 0.86.0 & 0.86.1 compat in QFAPI alongside #329. |
Pyrofab
reviewed
Jul 26, 2023
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
|
Going to go through and implement some of the changes suggested by @Pyrofab in a moment. Probably going to have both varargs for the flags and the current system, in order to have similarities to both QSL's static registry flags and FAPI's dynamic registry flags. Disabling flags will need to check if the dynamic registries are frozen, and if they are, refuse to change the state of the flag on that registry. This should stop desyncs, hopefully. |
- Added protected helper method DynamicMetaRegistryImpl#isFrozen for use in DynamicRegistryFlagManager - Added DynamicMetaRegistryImpl#LOGGER with the name "quilt_dynamic_registries" as a logger for catching exceptions related to the setting of flags - Removed the ability to disable dynamic registry flags, changed the signature of the related flag-enabling methods from enableFlag -> setFlag (on both DynamicRegistryFlag and DynamicRegistryFlagManager) - DynamicRegistryFlagManager#setFlag now throws an IllegalStateException if a mod attempts to enable a flag when dynamic registries are frozen. This is caught and printed to the logger mentioned prior in DynamicRegistryFlag calls - Added varargs for setting DynamicRegistryFlag values on dynamic registry creation - Improved DynamicRegistryFlag#OPTIONAL javadocs slightly
EnnuiL
added
t: refactor
EnnuiL
approved these changes
Jul 29, 2023
There was a problem hiding this comment.
While I do have style cleanup suggestions in mind, I believe it would get way too nitpicky to have it block a PR (I'd rather have a post-merge commit than that);
The important thing is: the logic looks sound to me; Ennui's Stamp of Approval (Can Skip FCP Edition)
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
...re/registry/src/main/java/org/quiltmc/qsl/registry/impl/dynamic/DynamicMetaRegistryImpl.java
Show resolved
Hide resolved
...registry/src/main/java/org/quiltmc/qsl/registry/impl/dynamic/DynamicRegistryFlagManager.java
Outdated
Show resolved
Hide resolved
...registry/src/main/java/org/quiltmc/qsl/registry/impl/dynamic/DynamicRegistryFlagManager.java
Outdated
Show resolved
Hide resolved
...ary/core/registry/src/main/java/org/quiltmc/qsl/registry/mixin/DynamicRegistrySyncMixin.java
Show resolved
Hide resolved
...ary/core/registry/src/main/java/org/quiltmc/qsl/registry/mixin/DynamicRegistrySyncMixin.java
Outdated
Show resolved
Hide resolved
One of them needs a more specific wording change, so it's going to be done in another commit since that's easier for us Co-authored-by: Ennui Langeweile <[email protected]>
accidentally dropped a word guh
|
I LOVE FORGETTING A SINGLE WORD AND HAVING TO MAKE ANOTHER COMMIT!!!!! |
mood |
EnnuiL
approved these changes
Jul 29, 2023
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicRegistryFlag.java
Outdated
Show resolved
Hide resolved
Hopefully the final style change moment Co-authored-by: Ennui Langeweile <[email protected]>
LambdAurora
reviewed
Jul 30, 2023
...registry/src/main/java/org/quiltmc/qsl/registry/impl/dynamic/DynamicRegistryFlagManager.java
Show resolved
Hide resolved
...registry/src/main/java/org/quiltmc/qsl/registry/impl/dynamic/DynamicRegistryFlagManager.java
Show resolved
Hide resolved
Co-authored-by: LambdAurora <[email protected]>
Pyrofab
reviewed
Jul 30, 2023
...ry/core/registry/src/main/java/org/quiltmc/qsl/registry/api/dynamic/DynamicMetaRegistry.java
Show resolved
Hide resolved
- Added back methods to DynamicMetaRegistry with the same signature as the old ones to avoid binary incompatibility. These are marked as deprecated and should be removed in a breaking/major QSL release.
…into feature/dynamic-registry-flags
cocona20xx
added a commit
to cocona20xx/quilt-standard-libraries
that referenced
this pull request
Aug 2, 2023
* Implement Dynamic Registry Flags. - Currently, only one flag, `OPTIONAL`, exists. - Modified DynamicRegistrySyncMixin in order to implement these flags. Due to the Redirect mixin header used to implement the flag filtering being taken directly from FAPI, the header of DynamicRegistrySyncMixin was changed to mention FabricMC copyright, in accordance with QSL contribution rules in CONTRIBUTING.md * Implement Dynamic Registry Flags p2 - Modify DynamicRegistryFlagManager to use Identifiers over RegistryKeys internally, in order to avoid generic-related jank. - Fix mixin ported from FAPI by correcting method target string to use Quilt Mappings. - Implement very basic tests. * Expanded upon OPTIONAL flag javadocs to elaborate use-case * Expanded further on OPTIONAL javadocs * Implemented some changes suggested by Pyrofab - Added protected helper method DynamicMetaRegistryImpl#isFrozen for use in DynamicRegistryFlagManager - Added DynamicMetaRegistryImpl#LOGGER with the name "quilt_dynamic_registries" as a logger for catching exceptions related to the setting of flags - Removed the ability to disable dynamic registry flags, changed the signature of the related flag-enabling methods from enableFlag -> setFlag (on both DynamicRegistryFlag and DynamicRegistryFlagManager) - DynamicRegistryFlagManager#setFlag now throws an IllegalStateException if a mod attempts to enable a flag when dynamic registries are frozen. This is caught and printed to the logger mentioned prior in DynamicRegistryFlag calls - Added varargs for setting DynamicRegistryFlag values on dynamic registry creation - Improved DynamicRegistryFlag#OPTIONAL javadocs slightly * Commit most style changes One of them needs a more specific wording change, so it's going to be done in another commit since that's easier for us Co-authored-by: Ennui Langeweile <[email protected]> * Make documentation for DynamicRegistryFlag more accurate * Improve the javadocs on DynamicRegistryFlag AGAIN accidentally dropped a word guh * BUT WAIT, THERE'S MORE! Hopefully the final style change moment Co-authored-by: Ennui Langeweile <[email protected]> * Implement style change in DynamicRegistryFlagManager Co-authored-by: LambdAurora <[email protected]> * Fix binary incompatibility hopefully? - Added back methods to DynamicMetaRegistry with the same signature as the old ones to avoid binary incompatibility. These are marked as deprecated and should be removed in a breaking/major QSL release. * Remove deprecation annotations on compat. method overloads in DynamicMetaRegistry --------- Co-authored-by: Ennui Langeweile <[email protected]> Co-authored-by: LambdAurora <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the beginnings of a system similar to (but far simpler than) the Static Registry Sync API for Dynamic Registries, allowing flags to be set which modify the behavior of Dynamic Registries.
Currently, only one flag,
DynamicRegistryFlag.OPTIONAL, exists—this flag is a clone of the similarSyncOption#skipWhenEmpty, and thus only effects synchronized dynamic registries, and simply has no effect on non-synced dyn. registries.Both the
OPTIONALflag (and it's FAPI counterpart) set whether a dynamic registry should attempt to be synced with a connecting client which does not have said dynamic registry. This allows for clients without certain dynamic registries to still be able to connect, and thus is useful when creating entirely server-sided mods intended to be compatible with vanilla.More flags can easily be added in the future, however the infrastructure for flags that apply to all dynamic registries, and not just synced ones, has not yet been set up—a future PR to add such a flag would need to add a mixin for that purpose somewhere other than
DynamicRegistrySync.Note that the test mod for the registry API is ironically too complicated to serve as a proper test for this API addition—I have created a test mod locally (using the
mavenLocal()repo) and tested having a vanilla client connect to a pure QSL + testmod server with an optional dynamic registry defined in the testmod, and it thankfully did work. If required, I can upload this testmod's repo to github with instructions on how to run it.