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

Register Scarpet trait command only if Carpet is loaded and init profession later #129

Merged

Conversation

altrisi
Copy link
Contributor

@altrisi altrisi commented Feb 3, 2023

About the command, it makes it register only if Carpet is loaded, given else it's just wasted memory and a check per player when syncing command. It's not the main purpose of the PR.

About initializing the profession later, there's currently an issue-ish where Carpet is sensitive to the load order with the CarpetEventServer.Event class. If it's initialized too early, when entities from mods that haven't had their initializers run yet will generate a log entry each time they spawn (and will have less functionality in scarpet). Taterzens was causing this with the chain TaterzensFabric.onInitialize > ScarpetProfession::new > ScarpetProfession static > TaterzenScarpetEvent extends > CarpetEventServer.Event (found in gnembon/fabric-carpet#1647).
The fix was moving it to the class initialization of the command, given it runs later, late enough that most mods should've registered their entities. It's not the best of fixes, but it does the job.

I want this to be fixed in Carpet but I haven't done it yet and it likely won't be on 1.19.3, so I'm PRing this fix/workaround so it's not an issue with taterzens on 1.19.3.

This issue is also present in 1.19.2, though idk if you do backports.

@samolego
Copy link
Owner

samolego commented Feb 3, 2023

Thanks for contributing, silly gradle issues still make it fail build.
Seems fine to me otherwise.

Yeah backports are just to much work for me, so this will only be present in 1.19.3

@samolego samolego merged commit e7e7e81 into samolego:master Feb 5, 2023
@altrisi altrisi deleted the conditional-register-and-profession-later branch February 5, 2023 15:07
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