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

Add slot text to Rabbit levels in the Chocolate Factory #827

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

aiden-powers
Copy link
Contributor

@aiden-powers aiden-powers commented Jul 9, 2024

Added slot text to the chocolate factory for rabbits, unemployed is represented as 0. This also adds a public list of rabbits inside the edited files + the rabbit record now includes the level of the rabbit
image

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jul 9, 2024
viciscat
viciscat previously approved these changes Jul 9, 2024
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 9, 2024
if (slot.id == rabbit.slot()) {
// Assuming the level is to be displayed directly on the slot.
// Adjust x and y offsets as needed.
return List.of(new SlotText(Text.literal(String.valueOf(rabbit.level())), TextPosition.TOP_LEFT));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use the static SlotText#topLeft method instead of the constructor for a cleaner look, and perhaps some colors as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For colors, would that be to match the employee level of the rabbit? "Manager" being purple etc

Copy link
Collaborator

Choose a reason for hiding this comment

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

That could work, yes

Emirlol
Emirlol previously requested changes Jul 9, 2024
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.

The lore regex method works fine, but since a lot of things in the chocolate factory have levels it could be a bit more generalized to include other things in the CF, such as the time tower, rabbit shrine, etc. Also, this should stop working on max level rabbits since they don't have the next level text in their lore, but I don't have any at max level so I don't know for sure.

I'd suggest parsing the display names instead as well, it would be easier to handle since all items include their current levels in their display names and there's only 2 different formats. With this, you could use a switch for the different slots with the different formats and it should be much easier that way. This is up to you, though.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed merge me please Pull requests that are ready to merge labels Jul 9, 2024
@Emirlol
Copy link
Collaborator

Emirlol commented Jul 9, 2024

One other suggestion is to not commit directly to your fork's master branch, and to create new branches and commit to them instead so you can work on multiple things at once while keeping a clean branch to create new ones off of. If you don't know how to change it at this moment I'd suggest just keeping it this way for now and only doing that for future PRs if you ever wish to do more.

@kevinthegreat1 kevinthegreat1 added the good first issue Welcome new contributors :) label Jul 9, 2024
@kevinthegreat1
Copy link
Collaborator

Hi, thanks for your interest and contribution. Please address the comments from other maintainers and we’ll try to get this merged.

@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jul 9, 2024
@kevinthegreat1 kevinthegreat1 added the bleeding edge This PR has been accepted into bleeding edge label Jul 11, 2024
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jul 13, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jul 30, 2024
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.

Note

Not tested. Also squash please.

@kevinthegreat1 kevinthegreat1 added the tester needed This is used for a Discord webhook to create a thread and notify the tester. label Jul 30, 2024
@kevinthegreat1 kevinthegreat1 added this to the 1.22 milestone Jul 30, 2024
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 30, 2024
@kevinthegreat1 kevinthegreat1 removed the tester needed This is used for a Discord webhook to create a thread and notify the tester. label Jul 30, 2024
@kevinthegreat1 kevinthegreat1 merged commit c31cdee into SkyblockerMod:master Jul 31, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bleeding edge This PR has been accepted into bleeding edge good first issue Welcome new contributors :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants