-
Notifications
You must be signed in to change notification settings - Fork 546
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
Fixes #4123 - Coal Generator will no longer be locked after researching #4124
Conversation
Due to a logic bug in the Legacy storage backend if there was a duplicate ID it would mark it as researched for the first, then see it researched already and remove it on the second. This was happening for the Coal Generator and Bio Reactor here. Both shared he same research ID 173. We're just doing this fix for now until we can move away from the legacy backend (work in progress).
Your Pull Request was automatically labelled as: "✨ Fix" |
Quality Gate passedIssues Measures |
Slimefun preview buildA Slimefun preview build is available for testing! https://preview-builds.walshy.dev/download/Slimefun/4124/d818fa1f
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of doing this imo we should just change the id. We can still have this patch for future errors but this patch shouldn’t be needed
It's a hard no on changing the ID. This has been this way for years, I just want to get rid of these IDs. They'll be gone soon, let's deal with it for now. |
Description
Due to a logic bug in the Legacy storage backend if there was a duplicate ID it would mark it as researched for the first, then see it researched already and remove it on the second. This was happening for the Coal Generator and Bio Reactor here. Both shared he same research ID 173.
We're just doing this fix for now until we can move away from the legacy backend (work in progress).
Proposed changes
If we already see the value as researched in the file just double-check that the player does not have it researched.
This change also needed us to pin Paper due to a Paper PR that caused MockBukkit to fail. Not a fan of this but it should only be temporary until MockBukkit updates.
I hate that we always use a "1.x" version which versions are pushed to... Everyone really should always pin to just specific versions to avoid these kinda problems. But that's another topic for another day.
Related Issues (if applicable)
Fixes #4123
Checklist
Nonnull
andNullable
annotations to my methods to indicate their behaviour for null values