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

[1.21.4] Fix IClientItemExtensions#renderHelmetOverlay never being called #1837

Merged
merged 12 commits into from
Jan 14, 2025

Conversation

IThundxr
Copy link
Contributor

No description provided.

@neoforged-automation neoforged-automation bot added the 1.21.4 Targeted at Minecraft 1.21.4 label Jan 10, 2025
@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@neoforged-compatibility-checks
Copy link

neoforged-compatibility-checks bot commented Jan 11, 2025

@IThundxr, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 6d5e15590d1151b1c978add51b368379dfec0105.

neoforge (:neoforge)

  • net/neoforged/neoforge/client/extensions/common/IClientItemExtensions
    • renderHelmetOverlay(Lnet/minecraft/world/item/ItemStack;Lnet/minecraft/world/entity/player/Player;IIF)V: ❗ API method was removed

@ChampionAsh5357
Copy link
Contributor

So, this method needs a rename and some smarter handling imo. Basically, what Gui#renderCamerOverlays is doing is checking every equipment slot to see whether the equipped item (with sanity checks for is actually equippable and in the proper slot) has a camera overlay. The else statement just says, 'oh attempt to render this every time regardless whether the item is in the correct location'. For example, even holding an item with the method implemented (instead of wearing it on the head) would call the rendering code.

Technically there are a few other asssumptions being made, such as the player must be in first person for the effect to be applied, but we'll assume it's only going to be rendered for first-person cameras that are not scoping. So, for this case, it would make more sense to call the method something like renderFirstPersonOverlay and have it render in conjunction with the texture overlay that can be present.

@IThundxr
Copy link
Contributor Author

IThundxr commented Jan 11, 2025

So, this method needs a rename and some smarter handling imo. Basically, what Gui#renderCamerOverlays is doing is checking every equipment slot to see whether the equipped item (with sanity checks for is actually equippable and in the proper slot) has a camera overlay. The else statement just says, 'oh attempt to render this every time regardless whether the item is in the correct location'. For example, even holding an item with the method implemented (instead of wearing it on the head) would call the rendering code.

Technically there are a few other asssumptions being made, such as the player must be in first person for the effect to be applied, but we'll assume it's only going to be rendered for first-person cameras that are not scoping. So, for this case, it would make more sense to call the method something like renderFirstPersonOverlay and have it render in conjunction with the texture overlay that can be present.

Those would be some good changes, i'll get them implemented and backported to 1.21.1

@IThundxr
Copy link
Contributor Author

@ChampionAsh5357 I've went ahead and made some changes, let me know if you want any other changes

Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

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

Looks good, just giving two quick pedantics.

dhyces
dhyces previously approved these changes Jan 13, 2025
@IThundxr IThundxr dismissed stale reviews from dhyces and ChampionAsh5357 via fc937d4 January 14, 2025 18:54
dhyces
dhyces previously approved these changes Jan 14, 2025
dhyces
dhyces previously approved these changes Jan 14, 2025
Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

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

One minor text change since it can be any equipment slot

…IClientItemExtensions.java

Co-authored-by: ChampionAsh5357 <[email protected]>
@dhyces dhyces merged commit 6bbd94a into neoforged:1.21.x Jan 14, 2025
6 checks passed
@neoforged-releases
Copy link

🚀 This PR has been released as NeoForge version 21.4.58-beta.

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

Successfully merging this pull request may close these issues.

3 participants