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

Improve player inventory handling #209

Merged
merged 23 commits into from
Jul 3, 2024
Merged

Improve player inventory handling #209

merged 23 commits into from
Jul 3, 2024

Conversation

Jikoo
Copy link
Owner

@Jikoo Jikoo commented Jun 26, 2024

Improve compatibility/reduce risk of breakages by creating an entirely new inventory implementation wrapping internals.

If anyone is interested in testing this out, I believe it is currently very stable. Builds are available from Actions runs.
Note that earlier builds of 1.21 that have an abstract class InventoryView are probably unsupported.

TODO:

  • Fix desync on relog
  • Add safeguards to prevent mean inventory sorting plugins from adding our swanky new placeholder items to inventories
  • Reorganize crafting contents view to make a nice little square
  • Disallow offline/inactive edits to crafting slots to prevent item loss
  • Improve shift+click/drag+click logic of internals
  • Add placeholders to nonstandard slots if empty for clarity (armor, off hand, cursor, crafting)
  • Add translations for new features? Piggybacking Minecraft translations now.
  • Backport to older supported versions Older versions are on life support. Backporting is a lot of work (mostly on the verifying behavior side, Mojang mappings do make most version changes a copy+paste).

Subsequent PRs will cover the ender chest and feature configuration/management. Feature configuration will in turn enable advanced resource pack usage for increased inventory legibility.

Closes #200
Closes lishid#172
Closes lishid#92

Jikoo added 3 commits June 25, 2024 21:49
This removes the need for a lot of messy magic trying to prevent item deletion or duplication.
@Jikoo
Copy link
Owner Author

Jikoo commented Jun 27, 2024

Pretty happy with how things are coming along:
image
image
I'm hoping to do pretty placeholders for functional slots so it's clearer what's what, and then I will likely shift the new slots around a little more. I like the armor in the bottom left and the off hand adjacent to the chest, and the new buffers make it a little clearer where the normal inventory ends, so that much is set at least. I was debating trying to set up custom model data but I think it makes more sense to leave that to translations - it would require a custom resource pack either way.

Per checklist, still buggy though. Logging in while someone has your inventory open results in the inventory desyncing and becoming un-editable. Only the main inventory slots, not the armor, which is odd because they're handled exactly the same way. It's probably one tiny mistake somewhere, but I've been thinking way too much about serious important things like how to make everything pretty. Once the inventories look beautiful we can worry about silly little things like not actually being able to edit them.

Jikoo added 11 commits June 28, 2024 07:33
Default shift-clicking (at least the version the decompiler spits out) is a bit of a mess. It checks emptiness against raw slot content rather than reported slot content, so we can't use placeholders and allow shift-clicking into slots. Since armor placeholders would be really nice, that's a bit of a nuisance. Additionally, if we do not have placeholders in some slots (i.e. future state where we allow removal based on settings), nonstandard slots should never be quick-moved into.
There goes my secret plan to dupe diamonds by putting a diamond block in the grid, removing it with another account viewing my inventory, and  crafting the result once the grid is clear.
This idea was a bit dumb, but I was mostly done before I realized it would be much safer to create fake items by lying to the client during the sync process. Since it took a while, I'm committing it in case I need it in the future.
This reverts commit 7805e93.
It was a neat idea and kinda worked, but if Minecraft ever modifies click handling this will have a high probability of breaking.
This is what I get for not working in a linear fashion
Prevents other people grabbing them entirely
I was right, it was a single little mistake! Honestly no idea why armor was working, it shouldn't have.
Good ol' order of operations
Drop now-useless methods from internals
@Jikoo
Copy link
Owner Author

Jikoo commented Jun 29, 2024

I'm really excited about placeholders! All of the fake items in the inventory are now fake clientside items that the server knows about and keeps in sync. Even if an inventory sorting plugin is improperly sorting our container (which very carefully reports its slots as what they are, check InventoryView#getSlotType!), it can't add copies of our placeholders to other inventory slots.

I've squashed all of the bugs I could locate, so ideally the only things left to do are cosmetics and backporting. I'm marking the PR ready for review to indicate that I believe the current build is safe to use. If anyone does have feedback please let me know what you think. If you have layout suggestions, now is the time - once I settle on something and it goes live, I am unlikely to reorganize.

@Jikoo Jikoo marked this pull request as ready for review June 29, 2024 18:08
@Jikoo
Copy link
Owner Author

Jikoo commented Jul 1, 2024

Tried a lot of various options for placeholders for armor. Iron is too common as real armor. Leather looks a bit weird, but making it #C8C8C8 does make the majority of it #8B8B8B like the inventory background. A friend recommended chain, but the new textures look jarringly bright to me. Maybe I'm just a Minecraft boomer.

After that I got with someone who I used to administrate with and we talked about layout. I had already been tossing around the idea of setting up a resource pack that would use a custom font with placeholders disabled, so we talked through a bunch of ideas and settled on this as a layout:
full
The crafting result will be added for clarity, but will be read-only, hence the red output.

The non-resource-pack placeholder-based one will be a little messier, but ideally still comprehensible (old layout, but you get the idea):
image

@Jikoo
Copy link
Owner Author

Jikoo commented Jul 1, 2024

All righty, new layout is done and about as pretty as I think I can make it without a resource pack.
image
Opening self only allows access to equipment and drop slots - the only real reason to open yourself is to put non-equippable stuff on your armor slots.

I don't think I want to bother backporting this - testing on older versions is gross and boring, and I'm having fun. Honestly, I might just drop 1.20.4/1.20.6 because otherwise they both stay until 1.22, 1.20.6 was a pretty large and short-lived update.

Jikoo added 2 commits July 2, 2024 10:24
I'm not thrilled about adding keeping the unported versions around, but 1.21 is only 18.7% market share right now. 1.20.4 is 24.6%, and 1.20.6 is 11%. Realistically I should drop 1.20.6 first, but eh.
@Jikoo
Copy link
Owner Author

Jikoo commented Jul 3, 2024

Got the resource pack working with custom model data. It would look cleaner with fonts as well, but I believe that results in missing character textures for clients without the pack installed. It might be possible with a custom font and a translation that defaults to blank, but right now I'm happy with how pretty it currently is. Honestly, anything is way better than "just know what everything is," and since it took me an hour to figure out how to fix the result slot recolor rendering very dark, perhaps I will leave it at that.
image

Resource pack capability will come with a future PR I think - it will have to be customizable so that people can edit it to their liking.

@Jikoo Jikoo merged commit 09b3f83 into master Jul 3, 2024
3 checks passed
@Jikoo Jikoo deleted the dev/improve-player-inv branch July 3, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant