B1n-ry / Youre-in-grave-danger

A minecraft mod which adds one single block. The grave, which will generate when you die, and store your items
https://www.curseforge.com/minecraft/mc-mods/youre-in-grave-danger
MIT License
59 stars 16 forks source link

[COMPAT] Beans Backpacks #152

Closed BeansGalaxy closed 2 months ago

BeansGalaxy commented 4 months ago

I see you added compat for my mod and I just broke it in my latest update :) I was looking through your code and I wanted to clear some stuff up so our mods actually work together instead of having headaches and issues.

I was thinking separating the method drop() so that one part you can specify the direction x y z and it returns any ItemStacks it wants to drop onto the ground so you can collect them in your grave. Heres the new method to call the new drop() I put it away from the acutal class so incase of any future refactors since this time compat broke due to a refactor.

LMK if you have any more ideas or need anything else before I post this version.

B1n-ry commented 4 months ago

Thanks for clearing up the set vs update thing. I managed to figure out what worked, wasn't exactly sure why before, but roughly. Must have just missed the clear, as that didn't give an as notable output. Now I understand more though, so thank you for that.

As for the backData.drop(), I want everything stored inside of the grave block itself, with references to where they were stored and so on. That way I can back up the inventory fully, and implement other on death inventory management functionality with the backpack items. I'm not sure though if I understand how the backData.drop() method is used. As far as my understanding goes, drop() will still spawn the backpack entity. Or have I misunderstood how it works? Because if that's so, won't the items duplicate when I try to put them inside the grave? I don't know if you're referring to this comment in my code with dropping the backpack, and thought I wanted to always drop it on death? That's at least sort of how I've interpreted things at the moment.

That comment is related to specific "drop on ground" functionality if graves were to be disabled or something. I will still store the backpack in the same format to be able to back it up and such, so I can't really drop it in it's entity form, since I don't have a player reference. I can't store the player reference within the object either, since I have to be able to store and load from NBT. Is there a way to make the backpack entity spawn just with UUID or GameProfile?

BeansGalaxy commented 4 months ago

I was unsure if you were forced to collect the backpack inventory because you didn't know how to properly place the backpack. I get now you want to store the backpack inside your grave no matter how beautiful my entities look.

The backData.drop() method clears the player's back data and tries to put what is can inside a backpack. If a player is wearing an elytra then it throws it onto the ground and if they're wearing a Decorated Pot it drops the inventory + the pot onto the ground as items.

If those methods could help I'd want to write a special compat method since I'm constally moving my classes since this mod's my intro to modding and I keep finding stuff that could be re-written. To check if a player is wearing a backpack use backData.getStack(). That automatically checks Curios/Trinkets.

I also forgot to mention but I have 3 classes BackData BackSlot & InSlot. BackData hold all the info for everything in the mod while BackSlot is only the slot that appears in the inventory. With Curios/Trinkets installed the BackSlot never gets used and it's replaced with the InSlot which is tied to the last item inserted to the backpack. I saw you were using BackSlot instead of BackData in some cases and that could be another cause for C/T breaking

B1n-ry commented 4 months ago

Yeah I almost felt bad for putting the backpack in the grave, instead of placing it. It does look very good! But from what I gather I should use backData more and backSlot less. I think I got the "equip" and "get" backpack functionality correctly, as it seems to work. It's implemented here (get) and here (equip). Being able to place backpacks without a player reference would in my case make it possible to drop the backpack in its entity form when the grave module is either disabled, or instructed to drop the backpack in the world. But if that doesn't make sense within your mod you shouldn't have to do that. It technically works just dropping all the items on the ground normally. If you did change it though, you wouldn't necessarily have to replace something. You could just overload the function with new variables and call one of them from the other. Oh also I encountered log spam when hovering over the ender backpack in my inventory. It happened both this and last version, however I have only confirmed it happening outside my dev env last version (or at least some previous version, I haven't kept track of how many updates you've released since my initial compat). You might already know of it just thought I'd mention it since I did see it on 2 different versions. But yes anyway I really appreciate you taking the time to try to improve the compatibility! Let me know if there's anything else you want me to do!

BeansGalaxy commented 4 months ago

I can understand that you'd want to store the backpack since they could be destroyed in lava. I'll likely get an update out tomorrow with these changes and I'll let you know here when it's live.

As for the log spam it's likely that the the ender backpack didn't have it's key registered. A lot of it's code is placed onto of the rest of the backpack code when I should've just rewrote most of it so that it worked together. Instead I just have a lot of checks to see if it's a Ender Backpack. I'll check it out again.

BeansGalaxy commented 4 months ago

20.1-0.17-v2 is live. I put all the helper methods inside of CompatHelper, hopefully this ends the generational war between inventory expansion mods and grave mods :)

B1n-ry commented 4 months ago

Awesome! Thank you for going the extra mile with the compat class! I'm just waiting on more info regarding an issue with my graves, and then I'll update as well. In my opinion there's not really a war between inventory expansion mods and grave mods. I mean in my case, I like inventory expansion mods. I just don't know about all of them, and people only report (sometimes) when a crash happens. But do let me know if you end up moving stuff around or changing something! Didn't find a "add to backpack" method, so I used the same as I did before, but other than that I'm now only using the CompatHelper static functions to handle the data. I'm open to changing stuff if you think it would make more sense in your mod!

BeansGalaxy commented 4 months ago

I just meant a war because I see a lot of issues with other backpack mods and I get a lot of issues with grave mods so I assume we're not the only ones.

As for an add to backpack, do you need a method to add items? Using the createBackpackEntity() you can specify its inventory using the NonNullList<ItemStacks> stacks param (I think it would be a DefaultedList<ItemStack> in fabric) but I can add a method if you need to add items later than its creation.

Thanks for helping out though, like I said I've been getting reports for bugs with grave mods and I figured if its a reoccurring thing it must be something with my mod so I appreciate your help 😎 👍 👍 I'm pretty new to modding so if any of my implementation is unconventional I'd understand any criticism and I'm open to other ideas

B1n-ry commented 4 months ago

I mean adding items to the backpack in the player inventory. And you don't have to implement that if you don't want to. I just noticed that you had a getter and setter for the backpack itself, but only a getter from the backpack contents. When a mod is incompatible with a grave mod, the grave mod usually has to implement compatibility with the other mod, but if it has a strong enough API (like most popular ones do), the inventory expanding mod can implement compat too. I'm not aware of any inventory expansion mod that does this though. I'm usually the one implementing all compatibility between mods for YiGD (at least when it comes to inventory management).

BeansGalaxy commented 4 months ago

The getBackpackInventory() getter is the actual item list so doing .clear() if they don't have a backpack and .addAll(itemStacks) would work. I was assuming you'd drop the backpack onto the ground if they already had one equipped but I can add a method to merge items into the backpack if thats what you're looking for. You'll have to do something with the items if the backpack fills up though.

FYI my item list isn't a fixed length like most containers in minecraft so make sure you don't try to get or place something out of it's index

B1n-ry commented 4 months ago

My bad, I thought getBackpackInventory() created a copy for some reason lol. Moving to setting the contents of the backpack that way I can no longer get the overflow back like I could with insertSilent(), but I'll insert into the same backpack the items was in originally, so it shouldn't ever overflow anyway. Also the backpack is only dropped if the grave should drop all of its contents when claimed, or if the backpack should specifically drop on death (for example if graves are disabled). Thanks for clearing that up though!

B1n-ry commented 4 months ago

Ok so I know I mentioned like 2 weeks ago I would update soon, but I had exams that required my focus. Now they are done and I should be able to get an update out within a few days though (probably tomorrow if all goes to plan). I did notice however when updating to the newest version, that the helper methods are no longer in the CompatHelper class? To me it seems like it went away between Merge remote-tracking branch 'origin/20.1' into 20.1 and Cauldrons are now on the creative tab, where the latter commit must have changed parent commit, as looking at the commit history in the file currently doesn't show any evidence of these helper methods ever existing. I would assume this isn't intentional?

B1n-ry commented 3 months ago

Not sure if you've noticed my question yet, so commenting on this again. In my latest update I just used the classes and stuff by accessing player BackData and such, which I used before this issue was created. I'm absolutely fine with keeping it this way, I'm just not sure again if this change where these methods was removed was intentional or not

BeansGalaxy commented 3 months ago

Oops, sorry for accidently ghosting you D: It looks like with those methods were deleted in a bad merge I made. I'll get move those methods back to CompatHelper in 20.1-0.23-v2 (for real this time)

BeansGalaxy commented 3 months ago

Actually, I've been getting a lot of issue with over grave mods clearing my inventory on death. I'm going to have to find the exact like of code that starts processing a player's death and inject there but I know this'll mess up anything you'd want to do since I'm trying to get ahead of grave mod's injects.

I've just learnt about Functions and Runnables and they're super super powerful so I'll now have a Function run that executes my drop code. I'll leave a way for you to override that function you instead of you canceling my code you'll be replacing it entirely. I also changed how pots work since adding cauldrons. They no longer drop all their items and instead drop the pot item loaded with the inventory.

This is my first mod so I'm always learning new ways to handle my code and I think this is better option again and I hope exams went well! ;)

BeansGalaxy commented 3 months ago

Actually Actually... I realized I just described an Event, so I made a cancelable event fired when the Back Slot should drop. You're given access to the BackpackInventory & BackSlot or a way to cancel my method entirely and write you're own implementation. Unless you have any suggestions, this is my last idea lol.

Here are the Events for both Events for both Fabric & Forge

B1n-ry commented 2 months ago

Updated the mod yesterday with the correct code. I'm sorry for taking so long, I've just done different things lately. Compat between our mods should work now anyway. Thank you for the help in improving the compat, and make sure to let me know if you would notice the compat breaking in the future!