Research unlock with vault economy provider #4002
Conversation
|
Your Pull Request was automatically labelled as: "🎈 Feature" |
|
Duplicate of #3942 |
not the same thing, this implementation is much simpler, and it only adds two new optional settings, the other one creates an unlock provider and what not, i guess it depends on the way you want to add this feature, this is quick and simple, the other one is more scalable probably |
| enable-vault-economy: false | ||
| economy-price-multiplier: 1000.0 |
There was a problem hiding this comment.
I think this should be grouped and simplified
| enable-vault-economy: false | |
| economy-price-multiplier: 1000.0 | |
| vault-integration | |
| enabled: false | |
| cost-multiplier: 1000.0 |
There was a problem hiding this comment.
do you mean you want vault-integration to be inside researches? or you want it at the top level?
| boolean creativeResearch = p.getGameMode() == GameMode.CREATIVE && Slimefun.getRegistry().isFreeCreativeResearchingEnabled(); | ||
| return creativeResearch || p.getLevel() >= cost; | ||
| boolean economyResearch = Slimefun.getIntegrations().isVaultInstalled() && Slimefun.getIntegrations().getVaultIntegration().hasBalanceForResearch(p, cost); | ||
| boolean expResearch = !Slimefun.getRegistry().isVaultEconomyEnabled() && p.getLevel() >= cost; | ||
| return creativeResearch || economyResearch || expResearch; |
There was a problem hiding this comment.
This is a method with high traffic, so think about if we really need to calculate all of this everytime...
If you split this into smaller methods then the || operator would ensure that the sub-methods are not called unnecessarily.
Agreed. Also @ybw0014 we don't really mark PRs as duplicates unless absolutely obvious. The same feature can be implement in numerous ways and we may choose one implementation over the other for reasons. |
Slimefun preview buildA Slimefun preview build is available for testing! https://preview-builds.walshy.dev/download/Slimefun/4002/ff5f2648
|
TheBusyBot
added
the
⚡ Merge Conflicts
TheBusyBot
removed
the
⚡ Merge Conflicts
Description
Adds the option to use money instead of experience to unlock research, without major changes in the codebase or config files.
Proposed changes
I added two new keys to config.yml inside researches.
enable-vault-economyandeconomy-price-multiplier. Ifenable-vault-economyis true, money will be used instead of experience, and to avoid having to define the prices again for each investigation, the price is calculated by doing xp levels cost timeseconomy-price-multiplier, that way the ratio between prices is the same as the previous exp cost system.I created a VaultIntegration class with a few methods, in case anyone wants to mess with the economy in the future.
The lore that shows the cost of unlocking a research shows the price formatted correctly as a currency.
Checklist
NonnullandNullableannotations to my methods to indicate their behaviour for null values