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

Cache SkinTextures instances & fix toggle menu #121

Open
wants to merge 7 commits into
base: architectury
Choose a base branch
from

Conversation

ItsThosea
Copy link

@ItsThosea ItsThosea commented Nov 22, 2023

In MixinPlayerListEntry, rather than making a new SkinTextures instance each time skin textures are requested, we cache an instance of our modified SkinTextures to supply. This requires us to refresh the list entry when we make a change (e.g. vanilla skin finished loading), which is handled in ListEntryAccessor. Lastly, this also makes the toggle menu and client cape type update instantly in-game, instead of having to leave and rejoin. This also removes the bug fix in CapeFeatureRenderer, as this is now fixed in vanilla.

@h3oCharles
Copy link

h3oCharles commented Nov 22, 2023

yo, i need someone with an animated cape

i have the animated capes purchased but don't have one equipped rn

wants me to equip an animated cape

i do that

🎶We're no strangers to love🎶

image

@ItsThosea
Copy link
Author

...unfortunately, that was me!

@CaelTheColher
Copy link
Owner

CaelTheColher commented Nov 24, 2023

val capeURL = capeType.getURL(profile) ?: return false
this expects the url to be null if the cape type is disabled, so I suggest moving isCapeTypeEnabled into the enum itself and checking if its enabled before getting the url

if (!handler.getHasCape() || !isCapeTypeEnabled(handler.getCapeType())) {

and this sometimes crashes the game if you disable a cape type while in multiplayer due to not checking if cape type is null

@ItsThosea
Copy link
Author

I fixed the possible crash you were talking about.
Wouldn't moving isCapeTypeEnabled to the enum make the toggle menu not take effect instantly?

@CaelTheColher
Copy link
Owner

I fixed the possible crash you were talking about. Wouldn't moving isCapeTypeEnabled to the enum make the toggle menu not take effect instantly?

I don't see why it would. It would be just for convenience so you could call CapeType#isEnabled instead of isCapeTypeEnabled(CapeType), and it would also allow it to be called from anywhere (like on the setCape method) instead of just the one Mixin class

@ItsThosea
Copy link
Author

If we block setCape, doesn't that prevent the cape from ever downloading at all? If that happens, then the toggle menu would have to re-download the capes.

@CaelTheColher
Copy link
Owner

If we block setCape, doesn't that prevent the cape from ever downloading at all? If that happens, then the toggle menu would have to re-download the capes.

it should only prevent downloading capes that are disabled, like a simple return false at the beginning of the setCape method if the cape type its trying to set is not enabled

@ItsThosea
Copy link
Author

If we block the capes from being downloaded, then the toggle menu would have to redownload them when we re-enable the cape type, as opposed to being instant.

@ItsThosea
Copy link
Author

ItsThosea commented Dec 26, 2023

A little late, but now, capes will no longer be downloaded if the cape type is disabled, and isCapeTypeEnabled was moved to CapeType. Sorry.

(also heres 1.20.4 support)

@CaelTheColher
Copy link
Owner

I haven't been really able to work on modding recently due to some personal things, but I do plan on merging this eventually so don't worry about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants