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

Fix for #158 and #121 #159

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

Conversation

EndangeredNayla
Copy link

@EndangeredNayla EndangeredNayla commented Sep 30, 2024

Fixes #158 and should fix #121
This reverts commit b00938e.

If this somehow doesn't get merged i have no choice but to fork the mod, which i believe is allowed under the license. Mind confirming that?

@EndangeredNayla EndangeredNayla changed the title Fix for #158 Fix for #158 and #121 Sep 30, 2024
@Su5eD
Copy link
Member

Su5eD commented Sep 30, 2024

Have you checked that this change does not re-introduce the issue(s) that were originally fixed by the commit in question?

@EndangeredNayla
Copy link
Author

Ill check that now, still a similar fix has to be done in 1.21 aswell.
image

@EndangeredNayla
Copy link
Author

So if there is a real fix for 1.21. i would just like to see if backported. Dont want to put too much on your plate tho, hopefully identifying the commit helps

@EndangeredNayla
Copy link
Author

Fabric seemenly fixed the bug in FabricMC/fabric#3517.

So maybe reverting our fix won't do much. still testing

@EndangeredNayla
Copy link
Author

EndangeredNayla commented Sep 30, 2024

The original jukebox bug gets unpatched, i can confirm that, but is this tiny bug really worth losing support for all transport pipes?

including piping out of jukeboxes

Not to mention this bug exists in 1.21

@EndangeredNayla
Copy link
Author

The solution here is to merge this commit which fixes the bug in 1.20 and 1.21 and implement Fabric upstream's fix for the jukebox mixin in the transfer api.

I don't mind doing it if your not up for the task.

If so do you want two serprate PRs or the same PR

@EndangeredNayla
Copy link
Author

Just bumping this

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.

2 participants