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

Fix rainbow armor recoloring other leather armor #3936

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix rainbow armor recoloring other leather armor #3936

wants to merge 3 commits into from

Conversation

iTwins
Copy link
Contributor

@iTwins iTwins commented Aug 5, 2023

Description

Due to a desync in SlimefunArmorTask and RainbowArmorTask other leather armor could be recolored.

Proposed changes

Compare against the item the player is currently wearing instead of the cached item.

Related Issues (if applicable)

Resolves #3931

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@iTwins iTwins requested a review from a team as a code owner August 5, 2023 23:32
@github-actions github-actions bot added the ✨ Fix This Pull Request fixes an issue. label Aug 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Your Pull Request was automatically labelled as: "✨ Fix"
Thank you for contributing to this project! ❤️

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 1c35e7ab

https://preview-builds.walshy.dev/download/Slimefun/3936/1c35e7ab

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

J3fftw1
J3fftw1 previously approved these changes Aug 5, 2023
View reviewed changes
JustAHuman-xD
JustAHuman-xD previously approved these changes Aug 6, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM

TheBusyBiscuit
TheBusyBiscuit reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@TheBusyBiscuit TheBusyBiscuit left a comment

Choose a reason for hiding this comment

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

This will be a big performance downgrade.
I reckon a much better approach would be to modify the updateRainbowArmor method as to not update non-rainbow armor.

@iTwins iTwins dismissed stale reviews from JustAHuman-xD and J3fftw1 via 5ef2296 August 9, 2023 18:31
@iTwins
Copy link
Contributor Author

iTwins commented Aug 16, 2023

Even though I changed this last week, I wanted to mention that RadiationTask runs SlimefunItem::getByItem for every item in the player's inventory and runs 3 times as often as RainbowArmorTask on default config. The 4 getByItem calls this would add on top of the 111 of the RadiationTask seem inconsequential to me. However I agree that the previous aproach could be optimized and that it is usually better to go for better performance.

@iTwins iTwins changed the title Fix #3931 Fix rainbow armor recoloring other leather armor Aug 19, 2023
JustAHuman-xD
JustAHuman-xD approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

lgtm i think? testing would be good though lol

@JustAHuman-xD JustAHuman-xD added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Dec 7, 2023
@J3fftw1 J3fftw1 added Build tested Used to indicate the PR preview build has been tested by one of the team and removed 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Dec 31, 2023
J3fftw1
J3fftw1 approved these changes Dec 31, 2023
View reviewed changes
Copy link
Contributor

@J3fftw1 J3fftw1 left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBusyBiscuit TheBusyBiscuit TheBusyBiscuit left review comments

@JustAHuman-xD JustAHuman-xD JustAHuman-xD approved these changes

@J3fftw1 J3fftw1 J3fftw1 approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Build tested Used to indicate the PR preview build has been tested by one of the team ✨ Fix This Pull Request fixes an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slimefun Rainbow armor is able to recolor normal and even custom leather armor
4 participants
@iTwins @TheBusyBiscuit @J3fftw1 @JustAHuman-xD