Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Publicized MinecraftClient#textureManager#190

Merged
TheGlitch76 merged 4 commits intoPatchworkMC:masterfrom
YTG1234:feature/public-MinecraftClient#textureManager
Sep 26, 2020
Merged

Publicized MinecraftClient#textureManager#190
TheGlitch76 merged 4 commits intoPatchworkMC:masterfrom
YTG1234:feature/public-MinecraftClient#textureManager

Conversation

@YTG1234
Copy link
Contributor

@YTG1234 YTG1234 commented Sep 15, 2020

Just saw this on #125 so I decided to do it.

@YTG1234 YTG1234 changed the title Added the patchwork-access-modifications module and publicized `Min… Added the "patchwork-access-modifications" module and publicized Min… Sep 15, 2020
@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

Maybe we could also move all Forge's access modifications that are already in Patchwork to this new module, idk how it's going to be possible to "load" the access widener when developing other modules. Any thoughts?

@TheGlitch76
Copy link
Member

Moving access wideners to their own module isn't the right solution yet because nobody has made a jar processor to make access wideners transitive.
I think instead of its own module, for now this should either be in an appropriate module that already exists (Check the Forge source code and see if it's used anywhere), or failing that dropped in patchwork-fml.

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

I think it may be used in the forge CloudRenderer: mc.textureManager.bindTexture(texture);. I'm not sure

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

Yep. It's only used in CloudRenderer

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

I will put it in the FML module

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

Moved it to the FML module for now. I didn't bump the version - as the commit msg says.

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 16, 2020

Turns out I put a /. Fixed it to a ., it should run now.

@YTG1234 YTG1234 changed the title Added the "patchwork-access-modifications" module and publicized Min… Publicized MinecraftClient#textureManager Sep 17, 2020
@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 17, 2020

Updated the title so it's less confusing

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 17, 2020

@famous1622 I changed things up a little, can you review again?

@rikka0w0
Copy link
Contributor

Moving access wideners to their own module isn't the right solution yet because nobody has made a jar processor to make access wideners transitive.
I think instead of its own module, for now this should either be in an appropriate module that already exists (Check the Forge source code and see if it's used anywhere), or failing that dropped in patchwork-fml.

I think in the future the rendering stuff should have their own package. But for now I would agree that this aw should be in patchwork-fml. Sometimes mc.textureManager is used by mods in their tileentity render and entity render, I don't there is a direct reference to that field in Forge.

@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 17, 2020

It's interesting that in most places Forge uses getTextureManager(), so I don't understand why the field has been made public

@cittyinthecloud
Copy link
Contributor

Was probably public forever ago and they don't want to break mods

Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

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

LGTM!

@TheGlitch76 TheGlitch76 merged commit 94d0092 into PatchworkMC:master Sep 26, 2020
@YTG1234
Copy link
Contributor Author

YTG1234 commented Sep 26, 2020

Was probably public forever ago and they don't want to break mods

I thought Forge was the opposite of backwards-compatibility

@TheGlitch76
Copy link
Member

Forge is more than just Lex and all of them do try to maintain back-compat for some things.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants