Skip to content
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

Improvement: Enchant Parsing SBA Convert Roman Numerals and NEU Inventories compat #1717

Merged
merged 8 commits into from
Aug 26, 2024

Conversation

VixidDev
Copy link
Contributor

@VixidDev VixidDev commented May 6, 2024

What

Makes enchant parsing work when SBA's Convert Roman Numerals feature is enabled and in NEU's inventories, such as /pv and storage overlay.

This will most likely need testing before approval since there is probably a certain case where my regex or code doesn't like a certain enchant format on some item / enchant / lore description. I've tested pretty much every item I have and other peoples items through /pv and they all work, but more testing from others would be appreciated.

Changelog Improvements

  • Improved Enchant Parsing compatibility with other mods. - Vixid
    • Now works with SBA's Convert Roman Numerals feature.
    • Now works with NEU's inventories, such as /pv and storage overlay.

@hannibal002
Copy link
Owner

ItemHoverEvent already does the same thing. just make its list modifyable, ig

@VixidDev
Copy link
Contributor Author

VixidDev commented May 6, 2024

ItemHoverEvent already does the same thing. just make its list modifyable, ig

Oh I see, idk how I missed that event. Yeah I'll probably just use that and make it modifiable.

@Mikecraft1224
Copy link
Contributor

Mikecraft1224 commented May 8, 2024

Doesn't work with NEU enchant colors
The regex in line 37 doesn't understand the enchants when there are other color codes applied

Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mikecraft1224's comment

@hannibal002 hannibal002 added this to the Version 0.25 milestone May 8, 2024
@VixidDev VixidDev marked this pull request as draft May 9, 2024 21:52
Copy link

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label May 16, 2024
@hannibal002 hannibal002 removed this from the Version 0.26 milestone May 18, 2024
VixidDev added 3 commits May 31, 2024 01:46
# Conflicts:
#	src/main/java/at/hannibal2/skyhanni/features/misc/items/enchants/EnchantParser.kt
Copy link

github-actions bot commented Jun 5, 2024

Conflicts have been resolved! 🎉

@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Jun 5, 2024
@VixidDev VixidDev marked this pull request as ready for review June 5, 2024 23:58
@VixidDev
Copy link
Contributor Author

VixidDev commented Jun 6, 2024

Moving the ItemHoverEvent mixin inject ensures it is never affected by NEU enchant colors so it doesn't break and die. If anyone tests this make sure your repo is using the updated regex otherwise the tooltip will be missing random enchants. Works with roman numerals to numbers from both sba and sh, but the roman to numbers from sh doesn't work in NEU pv, but I think thats for @Mikecraft1224 to amend.

@VixidDev VixidDev requested a review from hannibal002 June 6, 2024 00:07
@hannibal002 hannibal002 added this to the Version 0.26 milestone Jun 7, 2024
@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Jun 8, 2024
Copy link

github-actions bot commented Jun 8, 2024

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

Copy link

github-actions bot commented Jun 8, 2024

Conflicts have been resolved! 🎉

@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Jun 8, 2024
@hannibal002 hannibal002 modified the milestones: Version 0.26, Version 0.27 Jun 8, 2024
@VixidDev VixidDev requested a review from CalMWolfs June 17, 2024 13:21
Copy link

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Aug 10, 2024
Copy link

Conflicts have been resolved! 🎉

@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Aug 10, 2024
@jani270 jani270 modified the milestones: Version 0.28, Version 0.27 Aug 23, 2024
@jani270 jani270 added the Soon This Pull Request will be merged within the next couple of betas label Aug 23, 2024
@hannibal002 hannibal002 merged commit 57224a6 into hannibal002:beta Aug 26, 2024
3 checks passed
@github-actions github-actions bot removed the Soon This Pull Request will be merged within the next couple of betas label Aug 26, 2024
MTOnline69 pushed a commit to MTOnline69/SkyHanni that referenced this pull request Sep 11, 2024
…tories compat (hannibal002#1717)

Co-authored-by: Cal <cwolfson58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants