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.

Sign up for GitHub

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

Jump to bottom

[1.20.2] Add an event to register custom sprite source types #77

Merged
merged 10 commits into from
Nov 29, 2023
Merged

[1.20.2] Add an event to register custom sprite source types #77

merged 10 commits into from
Nov 29, 2023

Conversation

Minecraftschurli
Copy link
Contributor

@Minecraftschurli Minecraftschurli commented Aug 3, 2023 •
edited
Loading

This PR makes it possible to register custom SpriteSource types and load custom SpriteContents from them.

@CLAassistant
Copy link

CLAassistant commented Aug 3, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Minecraftschurli Minecraftschurli changed the title Add an event to register custom sprite source types [1.20.1] Add an event to register custom sprite source types Aug 3, 2023
@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.20 Targeted at Minecraft 1.20 labels Aug 3, 2023
Technici4n
Technici4n requested changes Aug 3, 2023
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

This should also have some sort of test to make checking that this works now and in the future possible.

@Minecraftschurli
Copy link
Contributor Author

The event being on the forge bus is intentional, I asked on discord and got the answer from @TheCurle that the forge bus is the correct one and most of the others are on the wrong bus (see here)

@Minecraftschurli Minecraftschurli force-pushed the feature/custom-sprite-source-types branch from c4b1b11 to 116af92 Compare August 16, 2023 08:05
XFactHD
XFactHD previously approved these changes Aug 31, 2023
View reviewed changes
@Minecraftschurli Minecraftschurli mentioned this pull request Oct 8, 2023
Closed
@Minecraftschurli Minecraftschurli self-assigned this Oct 31, 2023
@Minecraftschurli Minecraftschurli force-pushed the feature/custom-sprite-source-types branch from 6cdb716 to 11db25c Compare November 1, 2023 16:52
@Minecraftschurli Minecraftschurli changed the title [1.20.1] Add an event to register custom sprite source types [1.20.2] Add an event to register custom sprite source types Nov 1, 2023
@Minecraftschurli Minecraftschurli force-pushed the feature/custom-sprite-source-types branch from 11db25c to 3bc2c25 Compare November 1, 2023 17:37
Technici4n
Technici4n previously requested changes Nov 2, 2023
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

Can you also update the CustomTASTest testmod with this PR?

ChampionAsh5357
ChampionAsh5357 requested changes Nov 5, 2023
View reviewed 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.

Just some internal markings and it looks good to go. Figured they should be added since we shouldn't be firing this event outside of the NeoForged startup.

src/main/java/net/neoforged/neoforge/client/ClientHooks.java Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/client/event/RegisterSpriteSourceTypesEvent.java Show resolved Hide resolved
XFactHD
XFactHD requested changes Nov 28, 2023
View reviewed changes
Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

Two rather nitpicky things, but other than that it works great in my testing :shipit:

src/main/resources/META-INF/accesstransformer.cfg Outdated Show resolved Hide resolved
tests/src/main/java/net/neoforged/neoforge/debug/client/CustomSpriteSourceTest.java Outdated Show resolved Hide resolved
@Minecraftschurli Minecraftschurli enabled auto-merge (squash) November 28, 2023 16:30
XFactHD
XFactHD previously approved these changes Nov 28, 2023
View reviewed changes
marchermans
marchermans requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@marchermans marchermans left a comment

Choose a reason for hiding this comment

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

See comments, plus: This PR needs documentation in the PR message, what can you do with these changes, how is it supposed to be used etc.

src/main/java/net/neoforged/neoforge/client/ClientHooks.java Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/client/event/RegisterSpriteSourceTypesEvent.java Show resolved Hide resolved
@Technici4n Technici4n removed their request for review November 29, 2023 12:54
ChampionAsh5357
ChampionAsh5357 requested changes Nov 29, 2023
View reviewed 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.

Mainly documentation issues.

patches/net/minecraft/client/renderer/texture/atlas/SpriteResourceLoader.java.patch Outdated Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/client/event/RegisterSpriteSourceTypesEvent.java Outdated Show resolved Hide resolved
tests/src/main/resources/assets/minecraft/atlases/blocks.json Outdated Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/client/textures/SpriteContentConstructor.java Outdated Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/client/textures/SpriteContentConstructor.java Outdated Show resolved Hide resolved
tests/src/main/java/net/neoforged/neoforge/debug/client/CustomSpriteSourceTest.java Show resolved Hide resolved
ChampionAsh5357
ChampionAsh5357 previously approved these changes Nov 29, 2023
View reviewed 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.

Sure, this is good enough for now.

@Minecraftschurli Minecraftschurli merged commit 2f87365 into neoforged:1.20.x Nov 29, 2023
2 checks passed
@Minecraftschurli Minecraftschurli deleted the feature/custom-sprite-source-types branch November 30, 2023 08:00
Xing-C added a commit to Xing-C/NeoForge that referenced this pull request Dec 4, 2023
@Xing-C
Revert "[1.20.2] Add an event to register custom sprite source types (n…
1f9d55c 
…eoforged#77)"

This reverts commit 2f87365.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XFactHD XFactHD XFactHD left review comments

@marchermans marchermans marchermans approved these changes

@ChampionAsh5357 ChampionAsh5357 ChampionAsh5357 left review comments

@Technici4n Technici4n Technici4n left review comments

Assignees

@Minecraftschurli Minecraftschurli

Labels
1.20 Targeted at Minecraft 1.20 enhancement New (or improvement to existing) feature or request
Projects
None yet
Milestone
20.2 Stable Release
Development

Successfully merging this pull request may close these issues.
7 participants
@Minecraftschurli @CLAassistant @marchermans @XFactHD @Technici4n @ChampionAsh5357 @sciwhiz12