Fix disabled items preventing research completion #3827
Conversation
src/main/java/io/github/thebusybiscuit/slimefun4/api/researches/Research.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java
Outdated
Show resolved
Hide resolved
|
This is not the method you should use to fix the bug, by changing the is enabled function you can affect A TON of things that you don't know of. Instead change how the stats are calculated |
|
For an example of this fix see: JustAHuman-xD@1807315 |
1 similar comment
|
For an example of this fix see: JustAHuman-xD@1807315 |
|
I didn't make the PR for some reason wether it was time or an issue I don't remember |
But if you bind 2x researches to the same item, it unbinds the item from one of the researches (the first), thus leaving a research with NO items. This research is then NOT unlockable and again causes the same problem where you can't 100% research. Researches with no items should be treated as not enabled because they are incomplete and cannot work correctly. This was conciously changed globally to prevent anywhere research is used from taking into account any research that is not set up correctly. |
|
Idk it still feels like an iffy change but others can give their opinion on it. I think that way because people could make dummy/marker researches with no items in it intentionally, or disabled items intentionally tho a reason for that I can't think of off the top of my head. And the change you made to getResearches is definitely an iffy change. I would suggest instead adding a method called getEnabledResearches. As getResearches implies all researches not just enabled ones. |
|
TLDR changing the behavior of old and crucial API is always scary |
src/main/java/io/github/thebusybiscuit/slimefun4/core/SlimefunRegistry.java
Outdated
Show resolved
Hide resolved
|
NB adds new Placeholder |
| @@ -649,6 +649,11 @@ private void checkForDeprecations(@Nullable Class<?> c) { | |||
| * The new {@link Research} for this {@link SlimefunItem}, or null | |||
| */ | |||
| public void setResearch(@Nullable Research research) { | |||
| if (this.isDisabled()) { | |||
There was a problem hiding this comment.
If this check is not here, we'd have to check every affected item when getting enabled researches.
I don't think this is a good solution anymore though tbh
TheBusyBot
added
the
⚡ Merge Conflicts
|
Superceded by #3917 |
Description
When a SlimefunItem bound to a Research is disabled, the Research is not disabled. This means that the Research becomes locked permanently and the user can never achieve 100% Research unlock completion. If a SlimefunItem is globally disabled, it should not be bindable to Research since it's never usable. If a Research has no bound SlimefunItems (because they're all globall disabled), the research should act as not enabled also. Disabled researches should be filtered out too from the Progress stat.
Proposed changes
Prevent disabled items being added to Research, filter out Researches with no Items bound when requested. This could also be triggered some other place that's more efficient than every time research is requested. Please suggest another place to perform this action if a better place exists.
Related Issues (if applicable)
Resolves #3782
Checklist
NonnullandNullableannotations to my methods to indicate their behaviour for null values