-
-
Notifications
You must be signed in to change notification settings - Fork 91
Feature + Improvement: Crate Prio Pearl Lineups + Lineup Accuracy #184
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: main
Are you sure you want to change the base?
Conversation
…(whilst hideFarWaypoints enabled)
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.
Hello @Jordyrat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! gemini-code-assist here, providing a summary of this pull request. This PR introduces improvements to the Kuudra pearl waypoints, focusing on increasing their accuracy by updating coordinates. Additionally, it adds a new feature that displays pearl waypoints specifically tailored to guide the player towards their priority crate, as determined by the 'NoPre' module. This priority waypoint feature is dependent on the 'Hide Far Waypoints' setting to manage visual clutter.
Highlights
- Pearl Waypoint Accuracy: Updated coordinates for several existing pearl lineups in the Kuudra nether phase to improve their accuracy.
- Crate Priority Waypoints: Implemented a new feature that calculates and displays a specific pearl waypoint to reach the player's priority crate, based on the 'NoPre' module's determination.
- New Configuration Option: Added a new boolean setting 'Show Priority Waypoints' in the Pearl Waypoints module, which is dependent on 'Hide Far Waypoints'.
- New Supply Pick Up Spots: Added several new
SupplyPickUpSpotenum values to better represent specific locations relevant to pearl lineups.
Changelog
Click here to see the changelog
- src/main/kotlin/me/odinmain/features/impl/nether/NoPre.kt
- Added a new
priovariable to store the priority supply pickup spot. - Modified the
getSupplyPickUpSpotfunction to set thepriovariable based on the missing item and pre-spot.
- Added a new
- src/main/kotlin/me/odinmain/features/impl/nether/PearlWaypoints.kt
- Added a new boolean setting
showPriorityWaypointsdependent onhideFarWaypoints. - Updated the
pearlLineupsmap with newSupplyPickUpSpotkeys and revisedstartPosandlineupscoordinates for improved accuracy. - Added new
SupplyPickUpSpottypes (ShopCorner,XSafe,xCannonStair,SquareLow) to thepearlLineupsmap. - Updated the
blockNameMapwith revised coordinates. - Added
prioLineupandprioColorvariables. - Added a
cratePriorityPearlfunction to determine the priority pearl lineup based on the closest pickup spot and theNoPre.prio. - Modified the
onRenderfunction to track the closest supply pickup spot and render the priority lineup if the setting is enabled. - Updated the
Lineupdata class to include asupplyfield.
- Added a new boolean setting
- src/main/kotlin/me/odinmain/utils/skyblock/KuudraUtils.kt
- Added new enum values (
XSafe,ShopCorner,xCannonStair,SquareLow) to theSupplyPickUpSpotenum with their respective locations.
- Added new enum values (
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Pearls fly through the air,
To crates that need repair,
New lines guide the way,
For a better Kuudra day,
Accuracy beyond compare.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request aims to improve pearl lineup accuracy and add a new feature for displaying pearl waypoints towards the priority crate. The changes in NoPre.kt correctly set the priority, and the new enum values in KuudraUtils.kt along with updated coordinates in PearlWaypoints.kt should help with accuracy.
However, there are a few key areas that need attention:
- A critical issue in
PearlWaypoints.ktaffects both thehideFarWaypointsfunctionality and the determination of theclosestSupplyPickUpSpotfor priority waypoints. - An unused import should be removed.
- A potential placeholder coordinate
BlockPos(0,0,0)needs verification. - A TODO comment for a specific waypoint can be addressed.
Overall, the new features are valuable additions. Addressing the identified issues will ensure the stability and correctness of these features.
Summary of Findings
- Critical Logic Error in Waypoint Rendering: The
onRenderloop inPearlWaypoints.ktincorrectly handles the 'closest' state, breakinghideFarWaypointsand passing the wrong supply spot for priority pearl calculations. - Potential Incorrect Waypoint Coordinate: A pearl lineup destination is
BlockPos(0,0,0), which is likely a placeholder. - Unused Import: An unused and deprecated import (
jdk.nashorn.internal.ir.Block) exists inPearlWaypoints.kt. - TODO Item: A
TODOfor adding a pearl waypoint from 'Equals to Square' remains in the code. - Alpha Value Convention: An alpha value of
3fis used, which is unconventional if a 0-1 range is expected. - Code Repetitiveness (Not Commented): The
cratePriorityfunction inNoPre.ktandcratePriorityPearlinPearlWaypoints.ktare quite long and have repetitive structures. This was not commented on due to review settings (severity likely low/medium for maintainability). - Stylistic Nitpicks (Not Commented): Minor stylistic issues like
emptyList<BlockPos>()(can beemptyList()) and missing newlines at EOF were found but not commented on due to review settings (severity low).
Merge Readiness
Due to the critical issue identified in PearlWaypoints.kt that affects core functionality of both existing and new features, I recommend that these changes not be merged until this and other high/medium severity issues are addressed. Once these are resolved, the PR will be in much better shape. I am not authorized to approve pull requests, so please ensure further review and approval by authorized maintainers after the necessary changes are made.
| lineups = setOf(BlockPos(-97, 157, -114)) | ||
| supply = SupplyPickUpSpot.ShopCorner, | ||
| startPos = setOf(BlockPos(-71, 79, -135)), | ||
| lineups = setOf(BlockPos(0, 0, 0)) |
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.
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.
Correct based on the co-ords I was given but I have been told this one is a bit off. Will test further
src/main/kotlin/me/odinmain/features/impl/nether/PearlWaypoints.kt
Outdated
Show resolved
Hide resolved
odtheking
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.
Just some comments for the meanwhile
src/main/kotlin/me/odinmain/features/impl/nether/PearlWaypoints.kt
Outdated
Show resolved
Hide resolved
|
Have tested tri, the shop corner waypoint doesn't show (might be a wrong waypoint) and prio waypoints aren't showing for me. |
|
Staying as a draft for the foreseeable, I will update the shop pearl but idk why the prio waypoints aren’t showing and idk how the dynamic works to try and fix it to be further away. Will remove dynamic and update shop in next commit, but can’t really test for the foreseeable as Kuudra is a pain to do rn. Will see how it looks after foraging update |
|
Either way I implemented dynamic waypoints in the OdinFabric repository so if you come back to working on it you can take a look at that |


Improvement:
Feature: