-
-
Notifications
You must be signed in to change notification settings - Fork 163
Add MP to PV #1970
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
base: master
Are you sure you want to change the base?
Add MP to PV #1970
Conversation
Closes SkyblockerMod#1924 Added magical power calculation and display to profile viewer UI. **Changes:** - added 'getMagicalPower(JsonObject playerProfile)' function to calculate MP from accessories - display MP in '/pv' ('SkillsPage') UI - profile viewer UI increased height by 8 pixels **Testing:** Tested it with random Skyblock profiles from early game to maxed. For comparison I used the Skycrypt MP calculation / data. **Problems:** The algorithm can be off for a few points at certain profiles because 'ItemStack.getSkyblockRarity()' returns 'SPECIAL' for 'VERY_SPECIAL' Hatcessories. So it can happen, that the player has 1817 but Skyblocker shows 1815 MP because it sees 'VERY_SPECIAL' as 'SPECIAL'. Max difference because of this is 4 MP, because you can only have 2 active 'VERY_SPECIAL' Hatcessories.
Fixed 'ProfileViewerTextWidget.java:214: trailing whitespace [RegexpSingleline]'
src/main/java/de/hysky/skyblocker/skyblock/profileviewer/ProfileViewerTextWidget.java
Outdated
Show resolved
Hide resolved
…rough local functions
src/main/java/de/hysky/skyblocker/skyblock/profileviewer/ProfileViewerTextWidget.java
Outdated
Show resolved
Hide resolved
|
fair point, fixed it |
src/main/java/de/hysky/skyblocker/skyblock/profileviewer/ProfileViewerTextWidget.java
Outdated
Show resolved
Hide resolved
viciscat
left a comment
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.
looks good but i will let ya all duke it out for the hatcessories shenanigans
|
Ill make it so that only one Hatccessorie counts. Im pretty confident its a bug that the two I mentioned dont cancel eachother out - because the same Hatccessories just in different colors do. |
|
wdym exactly? I only changed alignment values. |
Alex33856
left a comment
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.
LGTM
AzureAaron
left a comment
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.
I think this should be rewritten to use our own accessories data since it is consistent with the other accessory features in the mod.
|
What is the big value you think this change would bring? I dont really see it rn. The code is clean and understandble, so I dont think there can be big misunderstandings. I get the point of "making everything use API x" but that is not the point here. The NW calculation for /pv also uses the data I use here. If there are good arguments for refactoring everything, I would ofc do it, but the thing was pretty much done for me (because, well it had the "merge me please" tag already). |
It is not great maintenance wise to have two (soon three with vic's helper) things doing relatively similar things but in totally different ways (in this case the data source). It is much preferable in the long term to keep things consistent where possible (e.g. why have two ways to render the similar filled boxes for different features when you can just have one). I don't think it is too difficult to change the usage, it is the same information just in a different format. |
|
Ok, make sense. Wouldve been nice if you had said it earlier but whatever. Ill get to it in a few days (like 1-2), because I got a minijob and no time atm. Is it the "https://azureaaron-api..."? And do we have a global-loader for that data too? |
It's in the TooltipInfoType.ACCESSORIES constant, the data is similar pretty much just laid out differently. Note for hatcessories they are all part of the same family and have the same tier of 1. |
|
Looked into it and I dont see the work to value tradeoff. The way the constants give the data is perfect: When using your data I would need to check manually if item X is a parent or upgrade of the current item I check this data for. As I said already, the constants are used elsewhere too. So if the constants are off, broken or wrong in any other way they have to be fixed no matter if my algorithm uses them or not. The refactoring would take me quite a while because im not very experienced with this type of data (and also I would need to test everything again...) and for me its just not really worth the time atm. If im wrong and the solution is actually very easy, please tell me in the end, im also just here to learn. If no one is bothered by it dont close the PR, maybe when I get some time I will refactor it. |



Closes #1924
Added magical power calculation and display to profile viewer UI.
Changes:
Testing:
Tested it with random Skyblock profiles from early game to maxed. For comparison I used the Skycrypt MP calculation / data.
Problems:
The algorithm can be off for a few points at certain profiles because 'ItemStack.getSkyblockRarity()' returns 'SPECIAL' for 'VERY_SPECIAL' Hatcessories. So it can happen, that the player has 1817 but Skyblocker shows 1815 MP because it sees 'VERY_SPECIAL' as 'SPECIAL'. Max difference because of this is 4 MP, because you can only have 2 active 'VERY_SPECIAL' Hatcessories.
If you need any changes, lemme know.
Merry Christmas, Cheers.