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

Change the registry to use world UUIDs #3672

Closed
wants to merge 0 commits into from

Conversation

gecko10000
Copy link

@gecko10000 gecko10000 commented Sep 12, 2022

Description

I have recently run into an issue where a world, when created quickly enough after deletion of a world with the same name, will not store data. I have tested this extensively and narrowed the issue down to this ticker code and this check for the world's existence. Since the ticker code only runs every half second, it does not remove the world from the registry until up to a Slimefun tick later and therefore causes issues in the constructor.

An easy way to recreate this issue is to delete a world and immediately create a new one with the same name.

Proposed changes

This PR changes the registry to use UUIDs instead of the world names themselves. This does not affect the storage location in files and therefore these are not breaking changes.

This may not be the ideal way to do it; it may be better to remove the world from the registry here instead.

Related Issues (if applicable)

No issue opened

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.14.* - 1.19.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@gecko10000 gecko10000 requested a review from a team as a code owner September 12, 2022 01:22
svr333
svr333 previously approved these changes Sep 15, 2022
Copy link
Member

@svr333 svr333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look indeed like it isnt a breaking change and would still work with all worlds.
So in that case it looks good to me!

@svr333 svr333 self-assigned this Sep 15, 2022
@TheBusyBiscuit TheBusyBiscuit added 🔧 Technical Thread This Issue is a technical Thread where developers can discuss technical changes to Slimefun. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Sep 29, 2022
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Jun 20, 2023
@J3fftw1 J3fftw1 closed this Jul 9, 2023
@J3fftw1
Copy link
Contributor

J3fftw1 commented Jul 9, 2023

im not sure how i closed this throught git.
I was updating his branch.

@variananora variananora removed the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! 🔧 Technical Thread This Issue is a technical Thread where developers can discuss technical changes to Slimefun.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants