Comforts and Sleeping Overhaul conflict
PurpleHel opened this issue · 14 comments
With sleeping overhaul installed, sleeping bags from comforts don't work. (I didn't try hammocks.) I dunno which mod is causing the problem. Have a log! https://pastebin.com/WxJM2WD9
It's caused by Sleeping Overhaul. They are not implementing proper checks for the bed position, specifically to see if the bed location is an instance of BlockHorizontal.
That being said, I can't guarantee that these mods will be compatible regardless because of how we both approach the sleeping mechanism. I'm planning on rewriting Comforts at some point, but I don't know when I can get to that nor do I know if that would fix any possible problems.
No worries. I disabled sleeping overhaul to avoid the problem even before I knew it was the source of the problem. Thanks for checking!
I can't figure out why you're using core modding here, but it breaks SleepingOverhaul's "allow sleep during day" optional feature too. I think we're both better off with revisiting this issue once your rewrite happens (which hopefully does not remove core code and makes the core-mod an optional addon, as per official Forge policy). Let me know if you want any help with making the ASM stuff more "friendly".
@cosmicdan
I'd love to make the core-modding optional or ideally remove it entirely, but the code is necessary to change the daytime sleeping checks. You must be aware of this as well since your mod uses this same core-modding, so I'm not sure where the confusion is about its necessity. The difference is that I cannot make this optional because it is part of a core feature of this mod.
At this juncture, I'd like to note a few things in particular about moving forward with a resolution.
-
The core-modding I do is indeed overtly destructive, and I apologize for that as this was all done when I was quite new to modding. Changes will be made in a future update that should hopefully address this and make it "friendlier", so to speak.
-
This issue is actually rather unrelated to the core-modding. I do agree that we're both better off revisiting compatibility between our mods at a later date once I've done some changes, but this particularly is about the fact that you need to check for blocks that are not instances of BlockHorizontal here. This will break whenever something puts a player to sleep without a BlockHorizontal instance or its facing property, which is a very possible situation in modded Minecraft.
-
I'm unaware of any official Forge policy that says that a core-mod must be an optional add-on. This is the latest news I can find about any official core-mod policy. The closest that I can see is to try and separate out coremods from the main mod, which is different from making something optional since dependencies could still require one with the other.
The core-modding I do is indeed overtly destructive, and I apologize for that as this was all done when I was quite new to modding. Changes will be made in a future update that should hopefully address this and make it "friendlier", so to speak.
What I do instead of completely replacing isDaytime is add an additional check afterwards. I have no need to prevent the isDaytime check, I just want to add another condition.
With that said, I still don't understand what your hooks actually do. Both #notTimeToSleep and #advanceTime just seem to be re-implementations of what the vanilla code already does - block sleep during day and skip to the next day, respectively. Both of these forced things will also cause compatibility issues with SleepingOverhaul.
This issue is actually rather unrelated to the core-modding. I do agree that we're both better off revisiting compatibility between our mods at a later date once I've done some changes, but this particularly is about the fact that you need to check for blocks that are not instances of BlockHorizontal here. This will break whenever something puts a player to sleep without a BlockHorizontal instance or its facing property, which is a very possible situation in modded Minecraft.
Indeed, I could of opened a new issue, but since this is about compatibility with SleepingOverhaul I thought I'd also raise it here since they would also need to be improved for compatibility.
You are right in that I should implement those changes for compatibility regardless, though. I'll just temporarily disable my core mod in IDE and sort that out. Thanks.
I'm unaware of any official Forge policy that says that a core-mod must be an optional add-on. This is the latest news I can find about any official core-mod policy. The closest that I can see is to try and separate out coremods from the main mod, which is different from making something optional since dependencies could still require one with the other.
True, I mis-remembered - the policy change was to make core mods separate (but making them optional, if possible, is an obviously beneficial implication is all). It's a little frustrating with Curse files section putting "additional files" all the way down the bottom, though.
Derp: "can only be used during the daytime instead of at night." So your hooks are to let your beds make day to skip to night - got it. Think I need another coffee, the method name starts with a negation so that threw me off the track xD
And regarding your introduction, yeah it would be great if we could get some Forge hooks for this instead - I would of done it myself and submitted a PR but I spent hours just trying to think of how the events would be inserted to be as generic as possible. Maybe it's time to revisit that thought, if both your mod and mine are in mind when designing the event then I'd think that it would be generic enough.
I understand why it may be confusing, I did a lot of weird things in this mod. In addition, the advanceTime hook is actually completely unnecessary since I can accomplish this by subscribing to the WorldTick event instead. It's been removed already in my dev build, I'm just waiting to polish up some of the other parts before pushing out an update.
Although, with this whole thing being brought up, I should probably try to expedite the next update. By the way, I've already changed the ASM in the last few minutes so that it doesn't break your sleeping feature.
I actually have a pending PR to Forge related to the daytime checks here: MinecraftForge/MinecraftForge#5013
You can check to see if that works for your use-case as well.
Indeed, that new event would work. I wrote a comment to show my support of it.
Doesn't matter how much time I spent with ASM, it's always confusing to me 😅 I just missed that last line in the mod description.
Ok so I'll work on the BlockHorizontal thing, and hopefully we can get that PR merged soon!
It's up to me to add/maintain compatibility with other stuff, don't worry about it :) hopefully I can sort it out.
Alright, small update.
The BlockHorizontal check is a good idea of course, implementing that stopped it from crashing and lets your mod take over completely. This was only a problem with the item-use mode of the Sleeping Bag - placed in the world with sneak worked fine already.
I won't be able to do SleepingOverhaul compatibility with the item-use command since I see you handle that yourself in the item class. Something to consider for your re-write, though I don't think it's really a big deal.
@cosmicdan
Finished the update. I've deferred all of the sleeping bag sleeping behavior back to vanilla and changed some of the internal logic, so the sleeping bag in its entirety should work out of the box with your mod.
I didn't change much about the hammock though, and I really can't since I have to change the daytime check from the initial sleep check. But I think you may be able to work around that on your end since you already have your own daytime check in place as well.
If your repo is up-to-date, then it looks like you're using Forge version 14.23.2.2611. I'm building against the latest recommended version, which is 14.23.4.2705. I'm pretty sure the WorldSleepResult stuff is fairly new since it was not available when I first made this mod either.
Heya,
Just working on some other features in my mod and also looking into compatibility with yours at the same time, and I'm looking at how you check whether sleeping is allowed in a dimension. You do this here:
But in my IDE search I have zero occurances of WorldSleepResult class nor the canSleepAt method. How is this possible? Apparently they're vanilla, but they don't exist in 1.12 on my end...?