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 dynamic registry element path include namespace #4180

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

Gaming32
Copy link
Contributor

Fixes #4179

Note that since this changes the loading location of modded dynamic registries, this is technically a breaking change to datapacks. However, with 1.21.2 being brand new, and that version already breaking datapack folder structures (with plurals becoming singular), I think now is the perfect time to fix this.

@Gaming32
Copy link
Contributor Author

An additional thing I will note is that fixing this realigns the behavior with the docs of DynamicRegistries. I do believe this used to be the old behavior before RegistryKeys.getPath was added.

The old method of prepending only applied in certain locations, and thus gives different results depending on the circumstance
@modmuss50 modmuss50 added the bug Something isn't working label Oct 23, 2024
@modmuss50 modmuss50 changed the base branch from 1.21.2 to 1.21.3 October 23, 2024 18:09
@modmuss50
Copy link
Member

Change seems fine to me, would it be possible to add some tests to help ensure that this doesnt break again in the future? Could maybe expand one of the existing dynmatic registry tests instead of creating a whole new one.

I get that its a breaking change, but I think the best solution is to just go for it in 1.21.3 otherwise we will be waiting a long time to fix. If anyone disagrees please let your thoughts be known.

@Gaming32
Copy link
Contributor Author

I can add tests, sure.

@Gaming32
Copy link
Contributor Author

I'm not entirely sure how to add tests for this. However, JsonDataLoader is now broken without this.

@PepperCode1 PepperCode1 added the priority:high High priority PRs that need review and work now. Review these first. label Dec 4, 2024
Gaming32 added a commit to Gaming32/bingo that referenced this pull request Dec 4, 2024
@modmuss50 modmuss50 self-assigned this Dec 5, 2024
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Dec 14, 2024
@modmuss50
Copy link
Member

I have added a simple test for this, should stop it from happening again.

@modmuss50 modmuss50 merged commit 3f3c499 into FabricMC:1.21.3 Dec 16, 2024
4 checks passed
modmuss50 pushed a commit that referenced this pull request Dec 16, 2024
… registries (#4180)

* Make dynamic registry element path be namespaced

Fixes #4179

* Remove old method of prepending the namespace

The old method of prepending only applied in certain locations, and thus gives different results depending on the circumstance

* Add simple RegistryKeysTest

---------

Co-authored-by: modmuss <[email protected]>

(cherry picked from commit 3f3c499)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working merge me please Pull requests that are ready to merge priority:high High priority PRs that need review and work now. Review these first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic registry loading location ignores namespace
3 participants