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

Update some things to 1.21.2 #52

Draft
wants to merge 6 commits into
base: 1.21.2
Choose a base branch
from

Conversation

TheDeathlyCow
Copy link

Hi, thought I'd make a pull request with my progress updating to 1.21.2.

One thing I've noticed is that the EntitiesPreRenderCallback, EntitiesPostRenderCallback, and PostWorldRenderCallbackV1-3 all have basically direct equivalents in Fabric API's WorldRenderEvents class. Is it a good idea to continue to keep these events in Satin? They seem a bit unnecessary and more prone to breakage with custom renderers. Perhaps if you really want to avoid any breaking changes, they could just be deprecated and called from an equivalent Fabric API event listener. Though I don't think it would be unwarranted to have a breaking change for this scale of an update...

@TheDeathlyCow TheDeathlyCow changed the base branch from 1.21 to 1.21.2 December 31, 2024 10:29
@DietrichPaul
Copy link
Contributor

please merge!!!

@Pyrofab
Copy link
Member

Pyrofab commented Jan 2, 2025

I'd rather keep the satin events for backward compatibility, but they can delegate to the Fabric events where relevant

to DietrichPaul: I appreciate your interest, but note that it's only a draft

@TheDeathlyCow
Copy link
Author

TheDeathlyCow commented Jan 2, 2025

I've set it up to simply delegate those events to Fabric API now. I've done my best to make sure that they are actually equivalent, and I believe they are. The Pre/Post entities callbacks both injected into basically the same point. The post world callback seems a bit trickier, since so much changed here since 1.21.1, but I think the LAST event is the closest to what the intended use of the post world event is.

Edit: Also, what was the point of the call to freezeDepthMap in the original event? Is this still needed? nvm its part of Satin I'll re-add it

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.

3 participants