Kir-Antipov / packed-inventory

Inventory management has never been so easy.
MIT License
6 stars 1 forks source link

Improvements #14

Closed sternschnaube closed 10 months ago

sternschnaube commented 10 months ago

Thanks for the update!! πŸ˜„

I was trying the mod now once again and came up with some improvements.

(I use Click Opener to maintain with right-clicks the GUI interactions. That's the reason why I would like to have this behavour splitted from your mod. https://github.com/RealMegaMinds/ClickOpener)

Kir-Antipov commented 10 months ago

Thanks for the update!! πŸ˜„

You are welcome :)

Before we proceed to your questions, let me lay out the fundamental principles and ideas of this mod: 1) The mod strives to ease the burden of inventory management. Thus, out of the box, it should provide the best possible experience. 2) The mod should be as extensible as possible, allowing other developers to easily integrate with it. 3) The mod should be as non-intrusive as possible, minimizing potential incompatibilities with other mods. For example, Packed Inventory has 0 server-related mixins, meaning that compatibility issues may arise only from a logical error made by me or another developer, but not from the core concept of this mod. 4) I never plan to add support for other mods. Maintaining support for a potentially infinite number of mods with ever-changing internals is a nightmare, even if you are getting paid for it - which I am not; I'm a hobbyist mod developer. However, thanks to (2), the situation can be turned around. A potentially infinite number of mod developers could spend a few minutes integrating with my never-changing API (all the steps are laid out in README) and just go on with their lives. This is a much better, fairer, and more stable solution for everyone involved.

Could you add please support for Carpet AMS Addition "largeShulkerBox" rule?

See (4).

Also, Packed Inventory does not hardcode or assume sizes of inventory-providing items. It operates on the actual reported values. Therefore, if an override of the vanilla logic is done correctly and the Shulker Box does report that it has 54 slots instead of the usual 27, Packed Inventory should already be compatible with such a mod. If not, however, I'm not the one to blame here.

Could you please split the GUI opening from (Crafting table, Stone cutters, Shulker boxes, etc) into a sepparate option?

See (1).

This feature is not only a part of the core experience, but it's also the feature this mod was born from. So, no, I'm not going to remove something that the earliest players downloaded my creation for in the first place. Also, your justification for that kinda escapes my ability to comprehend it: "Please, remove this feature from your mod because there's a mod that already does that and only that." Maybe you could just remove the second mod instead, if the only thing it does is already covered by Packed Inventory?

I would like to have the behaviour as follows: Grabbing a Shulker Box, while holding Left Shift, moving the Shulker Box over empty slots puts the items out.

Well, you can remap the key bind to whatever you want. However, I would strongly recommend against remapping any modded key binds that have any action attached to them to something so ingrained into Vanilla, such as Left Control and Left Shift, in general. See (3). Not only would this create conflicts with Vanilla but also with some other mods that decided that conflicting key binds are a cool idea, resulting in a poor overall experience.

For example, I myself remapped the vanilla "Pick the item" key bind to "`", because I cannot justify wasting a mouse button on something I do not use that often, and after that I mapped the middle mouse button to "Interact with item" from Packed Inventory and lived happily ever after.

sternschnaube commented 10 months ago

Thanks for the detailed answer πŸ˜„

I understand your point with mod support >>> in this case for (Carpet AMS Addition "largeShulkerBox" rule). In first place I don't need this myself - but I have always the bigger scale of community behind my thoughts. Maybe I will ask the other mod developer for support.


You do me bad with "remove this feature from my mod" - I was never saying such a thing. And of course I would never ask a developer to remove core features of his mod(s). As I said, I would like to have it optional splitted, maybe into a second keybind, which can be on standart the same keybind as before. No one would get hurted and you could use the Reset button to get the standart configuration of selected keys back. (Since we all know how bad Vanilla is if it comes to key binds).

My issue was just that I opened unintentional the GUI screen.

image

I don't think other mods biting with each other, but the experience feels not as smooth as I remember it from the past πŸ€” I will have to fiddle again with the keybinds - tested it yesterday but did not came up with a great feeling.


"I would strongly recommend against remapping any modded key binds that have any action attached to them to something so ingrained into Vanilla" - I understand your point there, but as long the key has no interaction while you have your inventory open as example, everything is fine.


Whats left now - could you split the "Interact with item / opening GUI" option please into a second keybind? Which could be the same standart key as "K" on default 😊

Kir-Antipov commented 10 months ago

Maybe I will ask the other mod developer for support.

If any problems arise on the other side, I'm always happy to help. The rule of thumb here is that the compatibility layer should not be a part of the code I need to personally maintain after it's written :D

You do me bad with "remove this feature from my mod" - I was never saying such a thing. And of course I would never ask a developer to remove core features of his mod(s).

Oh, ok, I guess I read "I would like to have this behavour splitted from your mod" in a more dramatic way than it was intended to be read.

I don't think other mods biting with each other, but the experience feels not as smooth as I remember it from the past πŸ€”

It should be, though, because I didn't change the way the mod works; I just polished it in some areas. So everything that did work before still works today the same exact way.

Whats left now - could you split the "Interact with item / opening GUI" option please into a second keybind? Which could be the same standart key as "K" on default 😊

To be quite frankly honest with you, I don't wanna xD The current code isn't designed to handle two different key binds that can (and should) target the same key. I don't see why such a scenario should even be possible, as all the outcomes that Interact with item can produce are distinctly different, with no overlaps or conflicts. That's why they are bound to a single key and not to 3-4 of them in the first place.

Let's iterate over all thinkable scenarios.

If Interact with item (without Toggle interaction mode (hold)) is pressed, the following outcomes are possible:

Cursor is Empty Cursor is Not Empty
Slot is Empty N/A Extract from cursor to slot
Slot is Not Empty Open a relevant screen Insert from slot to cursor or vice versa, depending on what you are holding

If Extract from item (or Interact with item + Toggle interaction mode (hold)) is pressed, the following outcomes are possible:

Cursor is Empty Cursor is Not Empty
Slot is Empty N/A Extract from cursor to slot
Slot is Not Empty Extract from slot to cursor Extract from cursor to slot or vice versa, depending on what you are holding

The only way to trigger a screen to pop up is by pressing Interact with item on an inventory-providing item like a Shulker Box or Ender Chest while holding nothing in your cursor. This action is distinct enough, so it cannot be accidentally confused with anything else, and thus, it does not require a separate key bind. And that's the reason it shares the same key with other actions.

Considering all the points mentioned above, let's identify the actual problem you have with the mod, instead of introducing additional overlapping key binds and unnecessary complications to the code, shall we? :) Please describe your problems in a bit more detail - what you are doing, what you expect to happen, what's actually happening, and so on.

By the way, are you testing the mod in Survival or Creative, by any chance?

sternschnaube commented 10 months ago

Thanks again for the rich detailed answer. I should find later time to come on this 😊

And thanks for not taking my words bad - you put a lot of time into your mod(s) and this has all my respect.

sternschnaube commented 10 months ago

I fiddled now again - I'm happy with the result. Put Extract from Item and Interact with Item both on Left Shift and now it feels great πŸ˜„

I just think now about the double size Shulker inventory. I will open an issue to the other mod developer and see what he says about it.

Great that your mod experience is back now!!!

sternschnaube commented 10 months ago

Hello @Kir-Antipov, would you be willed to help with the Shulker Box issue?

I mentioned you on the other mod page, but you must've overseen the notification for that 😊

The mod author says the problems lays on your code, he can't make nothing on his part.

Kir-Antipov commented 9 months ago

Thank you for your patience! I've been quite busy with my other projects. I'm not very good at multitasking, so I typically hyperfocus on a single task until I complete it. Only then can I move on to other things without affecting my productivity :)

Put Extract from Item and Interact with Item both on Left Shift and now it feels great πŸ˜„

Well, this simply means that you are not using Extract from item entirely. Therefore, I strongly recommend unmapping it altogether by pressing the key bind in question in the settings and then pressing Escape. While these binds do not currently interfere with each other (Interact with item simply takes precedence over Extract from item), I cannot guarantee this for future releases, as it's just a mere implementation detail.

Furthermore, I must reiterate that I do not recommend using Left Shift for either Interact with item or Extract from item. It is only suitable as a modifier key, not as the actual action key. Otherwise, you present an interference with both crouch functionality and shift-click in the inventory - as far from ideal as it goes.

The mod author says the problems lays on your code, he can't make nothing on his part.

This is just not true. I have checked how the mods play with each other, and everything functions fine (tooltips, bundle-like features, etc.), except for an incorrect screen popping up when dealing with large shulker boxes from your inventory (it's worth noting that large ender chests work fine too, by the way).

The reason behind this issue is that the mod in question utilizes a custom screen handler, and the only thing it does is basically overwriting the right-click action on a shulker box block. Consequently, every other mod (not limited to just Packed Inventory) aiming to open a shulker box screen without a shulker box block being present (e.g., a shulker box minecart mod, a mod implementing trading stations via shulker boxes, etc.) will display a "wrong" screen from the user's perspective. This is because they have no means of interacting with a custom screen quietly implemented by Carpet-AMS-Addition. So, whose fault is that?

Furthermore, even if there was a necessity to implement a custom screen handler (which is not the case, the folks from Carpet-AMS-Addition could have patched the vanilla screen handler to make it automatically compatible with other shulker box-related mods), they could have registered it via the Packed Inventory API, as I mentioned earlier.

In summary, there is just nothing to this situation where the code on my side could be considered problematic.

sternschnaube commented 9 months ago

Thanks!! ☺️

Kir-Antipov commented 9 months ago

"It's not my problem bro" be like

image