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.

Sign up for GitHub

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

Jump to bottom

Improve mining waypoint #791

Merged
merged 30 commits into from
Jul 25, 2024
Merged

Improve mining waypoint #791

merged 30 commits into from
Jul 25, 2024

Conversation

olim88
Copy link
Contributor

@olim88 olim88 commented Jun 20, 2024

updated how waypoints in the crystal hollows are done

  • uses same labels as commissions to be able to read from far away (fixes Improve Crystal Hollows waypoints #760)
    image
  • improved the extraction for chat waypoints
  • added wishing compass solver
  • add remove command /skyblocker crystalWaypoints remove "place"
  • use chat messages to pin point waypoint location (now creates waypoint when talking to king ect)

There are tests written for the wishing compass solver and i have played with this a bit and not found any other bugs.

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jun 20, 2024
@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label Jun 20, 2024
kevinthegreat1
kevinthegreat1 reviewed Jun 24, 2024
View reviewed changes
src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java

}

private static ActionResult onBlockInteract(PlayerEntity playerEntity, World world, Hand hand, BlockHitResult blockHitResult) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't know this was needed, I thought onItemInteract would be enough.

@kevinthegreat1
Copy link
Collaborator

Casually drops wishing compass solver lol. Very impressive pr.

@kevinthegreat1 kevinthegreat1 added the bleeding edge This PR has been accepted into bleeding edge label Jun 24, 2024
viciscat
viciscat reviewed Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@viciscat viciscat left a comment

Choose a reason for hiding this comment

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

I didn't test but code looks good, good job olim

src/main/java/de/hysky/skyblocker/mixins/PlayerListHudMixin.java Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java Outdated Show resolved Hide resolved
@viciscat viciscat mentioned this pull request Jun 26, 2024
Closed
@BigloBot BigloBot mentioned this pull request Jun 26, 2024
Merged
@BigloBot
Copy link
Contributor

image
The nittiest of nitpicks that idrc about but if The Wishing Compass can't seem to locate anything! is in chat Skyblocker probs shouldn't error.

From my little usage seems great though!

@AzureAaron AzureAaron added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jul 10, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jul 10, 2024
Emirlol
Emirlol previously requested changes Jul 13, 2024
View reviewed changes
Copy link
Collaborator

@Emirlol Emirlol left a comment

Choose a reason for hiding this comment

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

Other than a few small things here and there, looks pretty good from my few testing attempts. Honestly, the wishing compass solver is goated.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jul 13, 2024
@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. and removed changes requested This PR need changes labels Jul 15, 2024
@olim88
replace crystal waypoints with mining lebels
ee300d3 
replace the use of crystal waypoints with mining labels so they can be read from the hole of the hollows
@olim88
improve cword parssing
4250376 
take more location formats and partial matches for names
@olim88
use named way-point for metal detector
cfb861d
@olim88
add scanning for location chat
7b2efe7 
scan for messages to improve locations and auto add King. also fix bugs just introduced
@olim88
add wishing compass solver
8fa21f0
@olim88
clean up some unwanted cwords being passed
b3aa880
@olim88
add remove command and consistence
e30367c 
add command to remove way-point and consistence for the add and share
@olim88
add options to config and translations
c22843b 
add option for wishing compass to config and translations for text in it. add separate scale for crystal waypoints and commissions
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed merge conflicts This PR has merge conflicts that need solving. labels Jul 15, 2024
kevinthegreat1
kevinthegreat1 requested changes Jul 22, 2024
View reviewed changes
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

I've made some comments. I addressed as many as I can, but I still have some questions which I left unresolved. You can look at the latest commit for the changes I made.

src/main/java/de/hysky/skyblocker/config/categories/MiningCategory.java Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/MiningLocationLabel.java Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java Show resolved Hide resolved
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jul 22, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jul 22, 2024
@kevinthegreat1
Copy link
Collaborator

The translations test is much more... annoying than imagined. I'll try to get this fixed tho. https://discord.com/channels/507304429255393322/842691768175951942/1264978519784689827

kevinthegreat1
kevinthegreat1 approved these changes Jul 23, 2024
View reviewed changes
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Well, that was the deepest mojank in a while.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 23, 2024
kevinthegreat1
kevinthegreat1 requested changes Jul 23, 2024
View reviewed changes
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Wait no, the comments above need to be responded to.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed merge me please Pull requests that are ready to merge labels Jul 23, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jul 24, 2024
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 25, 2024
@kevinthegreat1 kevinthegreat1 merged commit 8f475f0 into SkyblockerMod:master Jul 25, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viciscat viciscat viciscat left review comments

@Emirlol Emirlol Emirlol left review comments

@kevinthegreat1 kevinthegreat1 kevinthegreat1 approved these changes

Assignees
No one assigned
Labels
bleeding edge This PR has been accepted into bleeding edge new feature This issue or PR is a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Crystal Hollows waypoints
7 participants
@olim88 @kevinthegreat1 @BigloBot @viciscat @Emirlol @LifeIsAParadox @AzureAaron