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

Mixin Changes + Compatibility With my Mod #155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shadowhunt22
Copy link

@shadowhunt22 shadowhunt22 commented May 9, 2024

Reason for Change

As expressed in a Discord thread, this mod, for some reason, would cause rendering issues for one of my mods which adds a feature renderer. The solution to the problem, as documented here and tested on the 1.20 branch, shows that injecting into the PlayerRenderer constructor with the annotation of

@Inject(method = "<init>*", at = @At("RETURN"))

would, for some reason, cause my mod to have rendering issues. Changing it to

@Inject(method = "<init>", at = @At("RETURN"))

fixes the rendering issues.

Changes

Because I initially tested this on the 1.20 branch, I was not aware that the main branch had a different method for adding the CustomLayerFeatureRenderer. Upon looking at the PlayerRendererMixin, the CustomLayerFeatureRenderer was adding by injecting into Minecraft's PlayerRenderer#setModelProperties. The change log is as follows:

  • Changed how the CustomLayerFeatureRenderer was added to the PlayerRenderer. Formally, this was done by injecting into Minecraft's PlayerRenderer#setModelProperties, but is now injecting into the constructor.

  • There was a concern that "Somehow [injecting into the constructor to add the CustomLayerFeatureRenderer] in 1.16.5 is a bit unpredictable, [and that] only late adding layer works well," but my testing has shown there are no prevailing issues.

…enderer. Formally, this was done by injecting into Minecraft's PlayerRenderer#setModelProperties.

There was a concern that "Somehow [injecting into the constructor to add the CustomLayerFeatureRenderer] in 1.16.5 is a bit unpredictable, [and that] only late adding layer works well," but my testing has shown there are no prevailing issues.
Copy link

what-the-diff bot commented May 9, 2024

PR Summary

  • Added a Custom Layer Renderer to the Player Renderer
    The creation method of the Player Renderer class now includes a new code line that adds a Custom Layer Renderer. This might enhance our game's visual presentation.

  • Simplified Player Model Property Setting
    Removed the 'loaded' variable along with its linked condition from the 'setModelProperties()' method. This simplification would potentially improve the efficiency of how our game sets the properties of player models.

  • Cleaned up constructor in Player Renderer class
    Removed commented out code that was initially intended for adding the Custom Layer Feature Renderer but was not in use. This cleanup improves the readability of the code.

@shadowhunt22
Copy link
Author

Oh, and a version bump because this would be considered a bug fix? 1.6.4 -> 1.6.5? Although I think I will leave that up to you because you have a better understanding of the gradle tools you use for your projects 😅

@tr7zw
Copy link
Owner

tr7zw commented May 14, 2024

Hm seeing this pr, I'm really not sure. This might break the mod with other mods(the initial reason on why the late init was done in the first place, but don't ask me which mods exactly were the reason).

@shadowhunt22
Copy link
Author

shadowhunt22 commented May 15, 2024

Can you give me some of the mods where these problems take place?

If I am being completely honest, it would be on these other mods to make them compatible with this mod IF you were adding this render feature in the constructor, not you making your mod compatible with other mods by adding the CustomLayerFeatureRenderer when the world loads.

I made my mod compatible with this mod by changing the priority of my mixin to be loaded later than what is being done in this PR.

@tr7zw
Copy link
Owner

tr7zw commented May 15, 2024

All this PR changes is the location the renderlayer ends up in the render layer list, that's it. So "technically" it shouldn't change anything(if mc wasn't buggy). The incompatibility with your mod is an issue with minecrafts vertex sorting messing up. Doing this order change here in 3d Skin layers may fix the rendering for your mod, but in turn, suddenly render the 3d skin layers above armor, held items, cosmetics from mods like essential etc. That's why I'm hesitant to do this change, as it might screw up a lot of other things(I sadly don't remember why I moved it to be added to the very end of the list by adding it later at runtime, maybe the commit back from 1.16? times has an issue related to it, idk). Thats why I don't want to merge something that would change the render order from 3d skin layers to everything else and potentially open Pandora's box of bugs. Sodium with CaffeineMC/sodium-fabric#2016 should render in the correct order I assume, no matter the order(since your mod isn't public from what I can tell, I can't test anything).

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.

None yet

2 participants