-
Notifications
You must be signed in to change notification settings - Fork 473
Added a working smolMe option and made smolpeople work on players only #541
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: dev
Are you sure you want to change the base?
Conversation
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 @benjilmao, 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
This pull request introduces a new "smolMe" secret setting that allows a player to shrink themselves without affecting other players. It also modifies the "smolPeople" setting to only affect actual players, excluding NPCs and other entities that use the PlayerEntity class. The author indicates they implemented this feature after learning some Kotlin and were inspired by requests in Discord.
Highlights
- New Feature: smolMe: Adds a new secret setting,
smolMe, that shrinks only the player enabling the setting. - smolPeople Enhancement: Modifies the
smolPeoplesetting to only affect actual players, excluding NPCs and other entities that inherit fromPlayerEntity. - NPC Exclusion: Introduces a check to identify and exclude NPCs from being affected by the
smolPeoplesetting, based on custom names, display names containing a specific character, or UUID version.
Changelog
- src/main/java/gg/skytils/skytilsmod/utils/SuperSecretSettings.java
- Added
smolMeboolean to the secret settings. - Added
smolmeto the settings loading logic.
- Added
- src/main/kotlin/gg/skytils/skytilsmod/mixins/hooks/entity/EntityLivingBaseHook.kt
- Added
isNPC()function to determine if an entity is an NPC. - Modified
isSmolto includesmolMefor the local player (EntityPlayerSP) and to exclude NPCs.
- Added
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.
A tiny coder's plight,
To shrink themselves from sight,
With Kotlin's grace,
A smaller space,
A secret, hidden light.
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
The pull request introduces a new "smolme" setting that shrinks the player without affecting others, and it also adds a check to exclude non-player entities from being shrunk by the "smolpeople" setting. The author mentions they don't fully understand how they implemented it, which raises some concerns about maintainability. Overall, the changes seem functional, but there are some areas that could be improved for clarity and maintainability.
Summary of Findings
- Code Clarity and Maintainability: The author's comment about not understanding how the code works raises concerns about future maintainability. It's important to ensure the code is clear and well-documented so others (and the author in the future) can easily understand and modify it.
- Magic Number: The number
4is used without explanation in theisNPCfunction. This is a magic number and should be replaced with a named constant or a more descriptive expression.
Merge Readiness
The pull request introduces new functionality, but the author's lack of understanding of the code and the presence of a magic number are concerning. I recommend addressing these issues before merging to ensure the code is maintainable and understandable. I am unable to directly approve this pull request, and other reviewers should review and approve this code before merging. At a minimum, the high severity issues should be addressed before merging.
|
|
||
| return entity.hasCustomName() || | ||
| entity.displayName.unformattedText.contains("♲") || | ||
| entity.uniqueID.version() != 4 |
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.
This checks the version of the UUID. What does version 4 signify, and why does a different version indicate an NPC? Consider adding a constant for 4 and a comment explaining its meaning.
const val PLAYER_UUID_VERSION = 4
return entity.hasCustomName() ||
entity.displayName.unformattedText.contains("♲") ||
entity.uniqueID.version() != PLAYER_UUID_VERSION // Players should have UUID version 4| public static boolean palworld = false; | ||
| public static boolean sheepifyRebellion = false; | ||
| public static boolean smolPeople = false; | ||
| public static boolean smolMe = false; |
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.
| palworld = settings.contains("palworld"); | ||
| sheepifyRebellion = settings.contains("sheepifyRebellion"); | ||
| smolPeople = settings.contains("smolpeople"); | ||
| smolMe = settings.contains("smolme"); |
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.
|
|
||
| val isSmol by lazy { | ||
| Utils.inSkyblock && entity is EntityPlayer && (SuperSecretSettings.smolPeople || isBreefing) | ||
| Utils.inSkyblock && entity is EntityPlayer && (SuperSecretSettings.smolPeople || (entity is EntityPlayerSP && SuperSecretSettings.smolMe) || isBreefing ) && !isNPC() |
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.
| if (entity !is EntityPlayer) return true | ||
|
|
||
| return entity.hasCustomName() || | ||
| entity.displayName.unformattedText.contains("♲") || |
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.
Why is this check needed?
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.
My first thought while reading a doc was to have this check as a safety net to make sure smolpeople excludes npcs and mobs
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.
No I mean that specific line looking for the recycling symbol.
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.
That’s supposed to be “§” because Ive seen somewhere that hypixel uses formatting codes on mob names, idk why is changes that to the recycling icon though
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'm not sure that works how you intend it to, players have colored names as well.
|
|
||
| val isSmol by lazy { | ||
| Utils.inSkyblock && entity is EntityPlayer && (SuperSecretSettings.smolPeople || isBreefing) | ||
| Utils.inSkyblock && entity is EntityPlayer && (SuperSecretSettings.smolPeople || (entity is EntityPlayerSP && SuperSecretSettings.smolMe) || isBreefing ) && !isNPC() |
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.
My first guess is that the npc check is only needed when going down the smolme path so I'd rather have it moved there to slightly optimize the condition check a little.
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.
Looking at it now I could move it to smolpeople since EntityPlayerSP already excludes mobs and for smolme but I’ll have to test that when am home
|
Duplicate of #507 |
I was bored and went to learn some kotlin, figured I'd try doing this since no one wants to based on dc messages.
added the secret settings "smolme" that shrinks the player without affecting other players, also added a check for stuff that use playerEntity to exclude them from being shrunk.
dont ask me to explain it more cuz idk how I even figured that out