-
Notifications
You must be signed in to change notification settings - Fork 473
fix: smolpeople breefingdog player sizing #571
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: 2.x
Are you sure you want to change the base?
fix: smolpeople breefingdog player sizing #571
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.
Summary of Changes
Hello @P3nguinMinecraft, 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!
This pull request primarily addresses an issue with player sizing for 'smolpeople' and 'breefingdog' features. The previous method of spoofing isBaby()
no longer works, so I've implemented a new approach that directly scales player models using a PlayerEntityRenderer
mixin. Additionally, the logic for determining 'smol' or 'breefing' status has been refactored to ensure correct per-player evaluation, moving away from shared by lazy
hook instances. An unrelated change to the World Age HUD display has also been included.
Highlights
- Player Sizing Fix: Resolved the broken player sizing for 'smolpeople' and 'breefingdog' by moving away from the ineffective
isBaby()
spoofing method. - New Player Scaling Mechanism: Implemented direct player model scaling (0.5x) by introducing a new
MixinPlayerEntityRenderer
that injects into thePlayerEntityRenderer
'sscale
method and manipulates theMatrixStack
. - Refactored Player Status Checks: Updated the
smol
andbreefing
status determination logic fromby lazy
properties on a sharedEntityLivingBaseHook
instance to per-call evaluations within the newPlayerEntityRendererHook
, preventing incorrect state sharing. - Mixin and Hook System Integration: Introduced
MixinPlayerEntityRenderer
andPlayerEntityRendererHook
to manage the new scaling logic, along withExtensionPlayerEntityRenderer
for proper integration into the existing extension/hook system. - World Age HUD Update (Unrelated): Moved the
dayState
calculation for the World Age HUD from theWorldAgeHud
class to theMiscFeatures
object, updating it on every tick.
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.
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 refactors the implementation for resizing certain players by moving from isBaby()
spoofing to direct MatrixStack
scaling in the renderer. This is a good architectural improvement. I found a critical issue in the new Mixin implementation where a static method is injected into an instance method, which will cause a crash. I've also included some suggestions for improving the new hook class for better maintainability and to clarify the intended behavior of a randomized effect.
.../main/java/gg/skytils/skytilsmod/mixins/transformers/renderer/MixinPlayerEntityRenderer.java
Outdated
Show resolved
Hide resolved
fun isBreefing(state: PlayerEntityRenderState): Boolean | ||
= state.name == "Breefing" && (SuperSecretSettings.breefingDog || Random.nextInt( | ||
100 | ||
) < 3) |
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.
Consider making isBreefing
private since it's only used within this class to improve encapsulation. Also, using Random.nextInt()
in a function called on every render frame will cause the "Breefing" player's size to flicker. If this is not the intended visual effect, consider caching the random result per player entity. Finally, the values 100
and 3
are magic numbers. Define them as named constants for clarity.
fun isBreefing(state: PlayerEntityRenderState): Boolean | |
= state.name == "Breefing" && (SuperSecretSettings.breefingDog || Random.nextInt( | |
100 | |
) < 3) | |
private fun isBreefing(state: PlayerEntityRenderState): Boolean | |
= state.name == "Breefing" && (SuperSecretSettings.breefingDog || Random.nextInt( | |
100 | |
) < 3) |
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.
TODO: flickering for breefing
mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/misc/MiscFeatures.kt
Outdated
Show resolved
Hide resolved
im actually so bad at branches bruh accidentally added null world age fix in here somehow |
Wouldn't these changes now break smolpeople on 1.8. Due to the presenece of preprocessing in the repo id assume there are still 1.8 releases planned so instead of removing the old code you would prob have to preprocess it out instead |
I can fix the commits in a bit. |
4a5a17e
to
1e708b8
Compare
mod/src/main/kotlin/gg/skytils/skytilsmod/mixins/extensions/ExtensionPlayerEntityRenderer.kt
Outdated
Show resolved
Hide resolved
mod/src/main/kotlin/gg/skytils/skytilsmod/mixins/hooks/renderer/PlayerEntityRendererHook.kt
Outdated
Show resolved
Hide resolved
.../main/java/gg/skytils/skytilsmod/mixins/transformers/renderer/MixinPlayerEntityRenderer.java
Show resolved
Hide resolved
mod/src/main/kotlin/gg/skytils/skytilsmod/mixins/hooks/renderer/PlayerEntityRendererHook.kt
Outdated
Show resolved
Hide resolved
Is there a reason why the logic is now performed in the mixin itself? It would make more sense imo to simply retain the existing logic (lazy caches the value once computed). |
caching the value was an issue because it kept computing before you loaded in or smth and you would have to change lobbies to apply settings changes |
Can you change the behavior so 1.8.9 works the same way then? |
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.
The logic for checking isSmol
and isBreefing
should be consolidated in the hook and reused across versions. looks like the only version specific thing is accessing the name which can just be supplied in a function invokation.
//#if MC>12000 | ||
//#else |
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 MC>12000 | |
//#else | |
//#if MC==10809 |
by lazy
was not going to work because all players would share the same hook instance, so I just made the smol / breefing check per callI am not quite sure if I matched the extension / hook system well