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 gamepad stash control, fix #7019 #7551

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sophie452
Copy link

Should improve gamepad control in stash, see #7019
First time contribution, so be gentle :)

Copy link
Contributor

@ephphatha ephphatha left a 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.

Source/controls/plrctrls.cpp Outdated Show resolved Hide resolved
Source/controls/plrctrls.cpp Outdated Show resolved Hide resolved
Source/controls/plrctrls.cpp Outdated Show resolved Hide resolved
@AJenbo
Copy link
Member

AJenbo commented Nov 21, 2024

@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.

@Sophie452 Sophie452 force-pushed the master branch 2 times, most recently from 0fee26d to 1a49194 Compare November 26, 2024 06:59
@Sophie452
Copy link
Author

Sophie452 commented Nov 26, 2024

Thanks for your thorough review. I tried to correct all points.

Copy link
Contributor

@ephphatha ephphatha left a comment

Choose a reason for hiding this comment

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

Just a couple of bugs to squash then it's good to go :)

Source/controls/plrctrls.cpp Outdated Show resolved Hide resolved
Source/controls/plrctrls.cpp Outdated Show resolved Hide resolved
@AJenbo
Copy link
Member

AJenbo commented Nov 26, 2024

Also please run clag-format to make sure the changes conform to the style guide

@Sophie452
Copy link
Author

Sorry that you had to wait. The improvements have been completed and added the clang-format. Thanks for your help :)

Copy link
Contributor

@ephphatha ephphatha left a comment

Choose a reason for hiding this comment

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

Nice work, moving around the stash with a gamepad is much nicer now.

Testing this change has revealed an issue with the way we handle Stash to Inventory movement specifically, it ends up requiring moving right twice if the cursor moves onto a 2-wide item in the last two stash columns. This appears to be because we don't handle moving onto the inventory as aggressively as moving out of it. It's a more complicated fix than I'd want to include in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants