feat: Parse charms as gears, show charm identification percentages #2271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… correctly Tome base stats are a special kind of identifications. Essentially they behave just like normal identifications, except that they always have a pre-identified, static value, and they are displayed in a non-regular manner. This kind of info is not used at the moment, but they will be used in the tome guide, and when sharing tomes in chat, with the new gear encoding. Charms have this kind of stats as well, except that they display it like normal stats, and are variable. See https://i.imgur.com/AD8O0y2.png for an example.
kristofbolyai
force-pushed
the
charm-parsing
branch
from
b444699 to
85eb4a5
Compare
kristofbolyai
commented
Dec 9, 2023
common/src/main/java/com/wynntils/models/wynnitem/parsing/WynnItemParser.java
Outdated
Show resolved
Hide resolved
…lculated for items with fixed stats
magicus
reviewed
Dec 10, 2023
common/src/main/java/com/wynntils/models/stats/builders/MiscStatKind.java
Show resolved
Hide resolved
|
I'm honestly not sure I understood how you are handling the level-variable stats. I will need to read through the code again when I'm more sharp. Apart from that, it looks fine I guess, it was just more of a "standard" conversion. |
kristofbolyai
commented
Dec 10, 2023
|
Okay, so you're using the levelRange as a requirement on the entire charm, and then you inject this when displaying a certain stat for that charm? This seems to be mandated by the API design, I guess. So if Wynn ever introduces more stats that have a level range, that level range will have to be the same for all stats on that particular charm. (Or they will have to redo the API.) I guess this works. |
magicus
approved these changes
Dec 11, 2023
kristofbolyai
changed the title
…ustom stat type ranges, rework the inverted stat system, other fixes to internal rolls (#2282) * refactor: Allow custom base ranges for items, rework inverted calculation, add option to display stat as inverted * fix: Fix SpellStatType internal names for percentage values * fix: Fix elemental defense stats Turns out the API was wrong, and the removed type does not exist, and is only a typo. This is now confirmed to work in-game. * fix: Correctly calculate internal role in every case I've done throughout testing, but I think just running with the assertions enabled without any issues proves that this should work. * feat: Reworked perfect/increase/decrease percentages * fix: fix legacy order not displaying skill stats (#2283) * chore: [auto-generated] Update urls.json [ci skip] (#2284) Co-authored-by: magicus <[email protected]> * feat: Pet menu search (#2285) * feat: Pet menu search * ci: spotless formatting --------- Co-authored-by: DonkeyBlaster <[email protected]> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: magicus <[email protected]> Co-authored-by: Ryan <[email protected]> Co-authored-by: DonkeyBlaster <[email protected]>
# Conflicts: # common/src/main/java/com/wynntils/features/tooltips/ItemStatInfoFeature.java # common/src/main/java/com/wynntils/handlers/tooltip/gear/IdentifiableGearItemInfo.java # common/src/main/java/com/wynntils/handlers/tooltip/tome/IdentifiableTomeItemInfo.java # common/src/main/java/com/wynntils/models/rewards/TomeInfoRegistry.java # common/src/main/java/com/wynntils/models/rewards/type/TomeInfo.java # common/src/main/java/com/wynntils/models/stats/builders/DefenceStatBuilder.java # common/src/main/java/com/wynntils/models/wynnitem/AbstractItemInfoDeserializer.java
kristofbolyai
changed the title
|
@magicus Do you have time to review this, or should I self-review and merge? |
# Conflicts: # common/src/main/java/com/wynntils/models/stats/StatCalculator.java # common/src/main/java/com/wynntils/models/stats/builders/DefenceStatBuilder.java # common/src/main/java/com/wynntils/models/stats/type/StatType.java
We should not modify the internal roll, as it causes issues in the future, for gear encoding. Also handle star calculations.
|
I've self-reviewed this, should be good. I will merge this before the announcement on discord. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Review by commit. I tried to make it integrate as neatly as I could, but I am aware that some parts are a bit more shoehorned than we would like it to be ideally, but I do believe this is the nicest way to integrate this overall, with minor modifications.
Some screenshots:
Note that charm ids are inverted differently than spell stats. This means they also have 30-130 as the internal roll range, not taking the base value sign into account. They also have the best effects on the lowest internal rolls.
Other items:
