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

Inventory mouse shortcut improvements #13146

Merged
merged 12 commits into from Jun 5, 2023

Conversation

OgelGames
Copy link
Contributor

@OgelGames OgelGames commented Jan 15, 2023

Re-implementation of #6544 / #7986, with additional features and fixes.

Fixes #5374, fixes #9456, fixes #12596.

Additions

  • Left-drag to distribute items evenly
  • Middle-drag to deposit 10 items in each slot
  • Shift-clicking on the craft preview moves items into the inventory (limited, see note below)
  • Shift-moving items by dragging (works with left, right and middle buttons)
  • Left-drag-moving an item over identical items to pick them up
  • Left-double-click to pickup up all of a selected item
  • Shift-left-click on an item with an identical item to move all of that item

Fixes and changes

  • Scrolling with an item now lets you pick the items back up
  • Moving an item over another item now requires an additional click to swap
  • Mouse button clicks that occur when already holding a button are now ignored

See the short video below for a demonstration of most of the new shortcuts:

2023-01-15.17-29-37.mp4

Fixes:

Also link some closed issues:


A note about the shift-clicking from the craft preview: while right-click and middle-click function as they should (crafting 1x and 10x), left click is limited to 1x. Shift-left-click should craft the maximum possible, but with the slow crafting code (see pandorabox-io/pandorabox.io#543, there doesn't seem to be an issue open here yet) that can cause significant lag spikes when crafting large amounts of items, so it must limited until that code is optimized.

To do

This PR is Ready for Review.

How to test

  1. Get some items.
  2. Use the various combinations of mouse buttons, scroll wheel and the shift key to move items around.

@Zughy Zughy added the Roadmap The change matches an item on the current roadmap. label Jan 15, 2023
@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Jan 16, 2023

This is great! This is a massive improvement and one of the best features in years. It's not perfect yet, a few things still feel a bit wrong:

  • Double-left-clicking an item to pick up all sometimes picks up weird items. Like, if you have a stack of 80 and a lot of single items in the inventory, and then you double-click the 80 stack, the items are sometimes picked up in a weird way. I don't understand the rule for this yet and it seems kinda random and confusing. Maybe care to elaborate the rule to make testing easier?
  • If u hold an item stack and move it around empty slots to distribute them with LMB: The UI looks confusing and it behaves weirdly. Sometimes, the item stack in my cursor disappears while I move my cursor which is VERY irritating.
    • And when I release the mouse button, the item stack I held gets automatically removed from my cursor and lands in an unpredicable slot. I would also like to know the rule for this behavior.
  • Bug: If I start pressing Sneak or a mouse button before I hover a slot, and then move to a slot, the slot does not highlight (but the item moving stuff still works)

@OgelGames
Copy link
Contributor Author

  • Double-left-clicking an item to pick up all sometimes picks up weird items. Maybe care to elaborate the rule to make testing easier?

The items should be picked up in order from left to right, top to bottom, until the stack is full. The code (here) goes through the list in order, moving items to the selected stack until there is no more room, at which point it stops.

  • If u hold an item stack and move it around empty slots to distribute them with LMB: The UI looks confusing and it behaves weirdly. Sometimes, the item stack in my cursor disappears while I move my cursor which is VERY irritating.

The cursor only holds the remainder, so when the items split evenly the cursor won't be holding anything.

  • And when I release the mouse button, the item stack I held gets automatically removed from my cursor and lands in an unpredicable slot. I would also like to know the rule for this behavior.

That doesn't sound right... Whatever items you're holding should stay in the cursor when you release the button, in fact nothing at all should change.

  • Bug: If I start pressing Sneak or a mouse button before I hover a slot, and then move to a slot, the slot does not highlight (but the item moving stuff still works)

Looks like that bug has existed for a while, I checked in 5.4.0 and it happens there too. I'm not sure how to fix that.

@nephele-gh
Copy link

The cursor only holds the remainder, so when the items split evenly the cursor won't be holding anything.

It should still show the item when there is still draggable items available, perhaos with a zero then.

As for the 10 items, could this be configurable for games? that would be quite neat, especially with larger stack sizes where another divisor would make sense.

@Emojigit
Copy link
Contributor

Emojigit commented Feb 18, 2023

A conflict should be solved:

diff --cc src/client/client.h
index 44a0de719,489a76956..000000000
--- a/src/client/client.h
+++ b/src/client/client.h
@@@ -438,10 -437,8 +438,15 @@@ public
        {
                return m_env.getLocalPlayer()->formspec_prepend;
        }
++<<<<<<< HEAD
 +      inline MeshGrid getMeshGrid()
 +      {
 +              return m_mesh_grid;
 +      }
++=======
+ 
+       bool m_inhibit_inventory_revert = false;
++>>>>>>> OgelGames/inventory_mouse_shortcuts
  
  private:
        void loadMods();

I left both getMeshGrid() and m_inhibit_inventory_revert when cherry-picking changes to my local client, and it works. However, which (or actually both) should be left?

How I finally merges:

  1. Checkout 2f9f0c0 (detached HEAD)
  2. Merge this branch (I've already set a remote for this)
  3. solve the conflict by removing the git-generated lines
  4. run git add src/client/client.h && git commit

BTW this PR's markdown description is so good that can be a starting template for all PR creators!

@OgelGames
Copy link
Contributor Author

Both, the only change I made to client.h was adding m_inhibit_inventory_revert, nothing should be removed.

@OgelGames
Copy link
Contributor Author

Anyone know what's holding this PR up? It's been 3 months now...

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code gets more chaotic the more features are added to it. If it is somehow possible to split individual actions into functions, it would be greatly appreciated.

src/client/client.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/client/client.h Outdated Show resolved Hide resolved
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label May 21, 2023
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 21, 2023
@Zughy Zughy removed Action / change needed Code still needs changes (PR) / more information requested (Issues) Rebase needed The PR needs to be rebased by its author. labels May 22, 2023
@SmallJoker SmallJoker self-requested a review May 23, 2023 15:31
@OgelGames
Copy link
Contributor Author

I think that's everything fixed. Ready for review again.

This code gets more chaotic the more features are added to it. If it is somehow possible to split individual actions into functions, it would be greatly appreciated.

Maybe in another PR, I don't want to do such a big refactor in this PR.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few minor code issues. The described item movements worked on my side. I could not find any issues.
Even limited stack movement (e.g. max 10 per movement) did work well. Tested with https://github.com/SmallJoker/minetest_lab/blob/master/init_76_inv_callbacks.lua (attempt to move a stack of 99 with any of the stack movement actions).

src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label May 31, 2023
@srifqi srifqi self-requested a review May 31, 2023 15:02
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it and it works. The concept and its implementation look good, except some code style issues in a few lines. There is a problem with the crafting using Shift when there is no more room, but I think that has other use cases that I can not think of it for now.


My tests:

When using an inventory,

  • Other button click is ignored when already a button is being clicked (being held).

When the targetting inventory is full (using Shift + click),

  • No transfer is happening.
  • No crafting is happening. ⚠
When holding/selecting a stack of items,
  • Left-click + drag to distribute items evenly
  • Middle-click + drag to put 10 items in each slot
  • Right-click + drag to put 1 item in each slot
  • Shift + left-click to move all identical stacks to another inventory
  • Scroll down to pick 1 identical item from an inventory
  • Scroll up to put 1 identical item to an inventory
Over an inventory,
  • Shift + left-click + drag to transfer hovered stacks between two inventories
  • Shift + middle-click + drag to transfer 10 items of each hovered stack between two inventories
  • Shift + right-click + drag to transfer 1 item of each hovered stack between two inventories
  • Left-click + drag to pick/get the hovered identical stack in the inventory
  • Left-double-click to pick the hovered stack and its all stack identical to it
  • Left-click + drag to pick then needing to click again to swap with a non-identical stack
Over the crafting output slot,
  • Left-click to craft once
  • Middle-click to craft 10 times
  • Right-click to craft once
  • Shift + left-click to craft once and transfer the result to the player's inventory
  • Shift + middle-click to craft 10 times and transfer the results to the player's inventory
  • Shift + right-click to craft once and transfer the result to the player's inventory

src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiInventoryList.h Outdated Show resolved Hide resolved
src/inventory.cpp Outdated Show resolved Hide resolved
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
@OgelGames
Copy link
Contributor Author

There is a problem with the crafting using Shift when there is no more room

The intended behavior is if the items can't be shifted, they are picked up by the mouse, as if you were not pressing shift. Is it doing something other than that?

@srifqi
Copy link
Member

srifqi commented Jun 3, 2023

If that is the case, it is fine for me. My first thought was that the crafting with Shift will be cancelled if there are no more rooms in the targetted inventory. (Minecraft cancels the crafting with Shift if the player's inventory is full.)

EDIT: Shift

@sfan5 sfan5 merged commit 252c79d into minetest:master Jun 5, 2023
13 checks passed
arihant2math pushed a commit to arihant2math/minetest that referenced this pull request Jun 11, 2023
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
@FeXoR-o-Illuria
Copy link

I live this, thank you! :)

Concerning that "stacks of 10": The usual stack size is 99. That is divisible by 3, 9 and 11 - but not by 10.
IMO a number that is divisible by the usual stack size should be used instead of 10.

After all being obsessed with 10 is just a strange habit of a single species of mammals from a narrow time window ... and here it doesn't fit well ;)

@nephele-gh
Copy link

Seeing as i've made the default stack size configurable if this is changed it should be configurable also, for my game 10 would be fine but 11 wouldn't be.

@OgelGames
Copy link
Contributor Author

Yeah, better to use a nice even number than something specific to a certain stack size.

Also, middle click has been 10x for a long time, this PR just extended the functionality a bit ;)

@SwissalpS
Copy link

+1 for configurable amount per game.
It has been 10 for a long time and annoying for a long time. 9 makes most sense to me as that is mostly what I need (or a multiple thereof)

(many of the bugs and design 'errors' we fix have been there for a long time, mostly that doesn't stop us from changing them to the better)

Yes OgelGames, out of the scope of your great PR :D

Wuzzy2 pushed a commit to Wuzzy2/minetest that referenced this pull request Jul 31, 2023
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Controls / Input Feature ✨ PRs that add or enhance a feature Formspec Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet