-
Notifications
You must be signed in to change notification settings - Fork 509
Add Advancement Renderers #5132
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
Conversation
modmuss50
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start, I havent done an indepth review or looked at the mixins or testmod.
.../fabricmc/fabric/api/client/rendering/v1/advancement/AdvancementBackgroundRenderContext.java
Outdated
Show resolved
Hide resolved
...abricmc/fabric/impl/client/rendering/advancement/AdvancementBackgroundRenderContextImpl.java
Outdated
Show resolved
Hide resolved
...a/net/fabricmc/fabric/impl/client/rendering/advancement/AdvancementRendererRegistryImpl.java
Outdated
Show resolved
Hide resolved
sylv256
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside from modmuss's requests, I made a few nitpicks.
...et/fabricmc/fabric/api/client/rendering/v1/advancement/AbstractAdvancementRenderContext.java
Outdated
Show resolved
Hide resolved
...t/java/net/fabricmc/fabric/api/client/rendering/v1/advancement/AdvancementRenderContext.java
Show resolved
Hide resolved
...client/java/net/fabricmc/fabric/api/client/rendering/v1/advancement/AdvancementRenderer.java
Show resolved
Hide resolved
...lient/java/net/fabricmc/fabric/mixin/client/rendering/advancement/AdvancementToastMixin.java
Show resolved
Hide resolved
...ient/java/net/fabricmc/fabric/mixin/client/rendering/advancement/AdvancementTabAccessor.java
Outdated
Show resolved
Hide resolved
fabric-rendering-v1/src/client/resources/fabric-rendering-v1.accesswidener
Outdated
Show resolved
Hide resolved
modmuss50
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, ill approve this and merge to last call. Just a few minor nit picks to fix before merging.
...ent/java/net/fabricmc/fabric/mixin/client/rendering/advancement/AdvancementsScreenMixin.java
Outdated
Show resolved
Hide resolved
...a/net/fabricmc/fabric/impl/client/rendering/advancement/AdvancementRendererRegistryImpl.java
Outdated
Show resolved
Hide resolved
...a/net/fabricmc/fabric/impl/client/rendering/advancement/AdvancementRendererRegistryImpl.java
Outdated
Show resolved
Hide resolved
...a/net/fabricmc/fabric/impl/client/rendering/advancement/AdvancementRendererRegistryImpl.java
Outdated
Show resolved
Hide resolved
|
Is there a reason why renderers get to pick which advancements they apply to instead of the opposite? Right now if someone else wants to use your renderer, they need to re-register it for their own advancement ids, and afterwards that cannot be undone. Wouldn't it make more sense to add fields to advancements letting them pick renderers from a registry? This way datapacks and modpacks can easily pick from any renderers added by mods. |
I think this would put this PR out of scope for the rendering API module, so much so that it would probably require a new advancement API module, since it would require server-side changes too. |
sylv256
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

This PR adds an API for registering custom renderers for advancement icons, frames, and backgrounds. Modders can use this to have extra fancy visuals for their advancement tabs.
There's some wacky mixins in here haha. I tried my best to make this as compatible as possible with any mod that may reformat the advancements screen.