-
Notifications
You must be signed in to change notification settings - Fork 795
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 gamepad stash control, fix #7019 #7551
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see some progress on that issue :D (especially since I've been too lazy to do anything like I said I would ages back...). The behaviour is a definite improvement compared to what we currently have.
As part of #6536 I added some code in PerformPrimaryAction (see https://github.com/Sophie452/devilutionX/blob/c641b49d73a547fcf953ed305936c9414d525f45/Source/controls/plrctrls.cpp#L1897-L1904) that shifts the cursor to the bottom right cell after pasting an item. That's no longer necessary with this PR, we want the cursor to stay in the centre of the item.
const StashStruct::StashCell itemIdAtActiveStashSlot = Stash.GetItemIdAtPosition(ActiveStashSlot); | ||
do { | ||
ActiveStashSlot.x--; | ||
} while (ActiveStashSlot.x > 0 && itemIdAtActiveStashSlot == Stash.GetItemIdAtPosition(ActiveStashSlot) && itemIdAtActiveStashSlot != StashStruct::EmptyCell && holdItem.isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit since this works, but I'm not a massive fan of the way it's structured. The loop only comes into play if holdItem.isEmpty()
is true so the conditions should really have that test first. Also checking if the itemIdAtActiveStashSlot is valid (i.e. not the empty cell) should really go before looking up the item at the new active slot position.
// Stash coordinates are all the top left of the cell, so we need to shift the mouse to the center of the held item | ||
// or the center of the cell if we have a hand cursor (itemSize will be 1x1 here so we can use the same calculation) | ||
if (holdItem.isEmpty()) { | ||
const StashStruct::StashCell itemIdAtActiveStashSlot = Stash.GetItemIdAtPosition(ActiveStashSlot); | ||
if (itemIdAtActiveStashSlot != StashStruct::EmptyCell) { | ||
const Item stashItem = Stash.stashList[itemIdAtActiveStashSlot]; | ||
const Point firstSlotOnItem = FindFirstStashSlotOnItem(itemIdAtActiveStashSlot); | ||
itemSize = GetInventorySize(stashItem); | ||
mousePos = GetStashSlotCoord(firstSlotOnItem); | ||
ActiveStashSlot = FindClosestStashSlot(mousePos); | ||
} | ||
} | ||
|
||
mousePos += Displacement { itemSize.width * INV_SLOT_HALF_SIZE_PX, itemSize.height * INV_SLOT_HALF_SIZE_PX }; | ||
SetCursorPos(mousePos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is no longer accurate, it'd be best if you can update it :)
Copying the comments from
devilutionX/Source/controls/plrctrls.cpp
Lines 996 to 1017 in 8615d31
// If we're in the inventory we may need to move the cursor to an area that doesn't line up with the center of a cell | |
if (Slot >= SLOTXY_INV_FIRST && Slot <= SLOTXY_INV_LAST) { | |
if (!isHoldingItem) { | |
// If we're not holding an item | |
int8_t itemInvId = GetItemIdOnSlot(Slot); | |
if (itemInvId != 0) { | |
// but the cursor moved over an item | |
int itemSlot = FindFirstSlotOnItem(itemInvId); | |
if (itemSlot < 0) | |
itemSlot = Slot; | |
// then we need to offset the cursor so it shows over the center of the item | |
mousePos = GetSlotCoord(itemSlot); | |
itemSize = GetItemSizeOnSlot(itemSlot); | |
} | |
} | |
// At this point itemSize is either the size of the cell/item the hand cursor is over, or the size of the item we're currently holding. | |
// mousePos is the center of the top left cell of the item under the hand cursor, or the top left cell of the region that could fit the item we're holding. | |
// either way we need to offset the mouse position to account for items (we're holding or hovering over) with a dimension larger than a single cell. | |
mousePos.x += ((itemSize.width - 1) * InventorySlotSizeInPixels.width) / 2; | |
mousePos.y += ((itemSize.height - 1) * InventorySlotSizeInPixels.height) / 2; | |
} |
const Point firstSlotOnItem = FindFirstStashSlotOnItem(itemIdAtActiveStashSlot); | ||
itemSize = GetInventorySize(stashItem); | ||
mousePos = GetStashSlotCoord(firstSlotOnItem); | ||
ActiveStashSlot = FindClosestStashSlot(mousePos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is actually unnecessary, it causes a behaviour difference compared to moving through the inventory. If you comment it out the gamepad navigation "remembers" the last column/row that was active when moving along staggered items. As-is you end up laddering to the top left cell of the stash.
2024-11-20T172157_devilutionx.mp4
2024-11-20T174602_devilutionx.mp4
@Sophie452 nice work, if you have time to address the comments from @ephphatha I think this will be good to merge. You can ignore the failing PS4 build, it's a known issue with a required package. |
Should improve gamepad control in stash, see #7019
First time contribution, so be gentle :)