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

Middle drag + Left drag #6544

Closed
wants to merge 3 commits into from
Closed

Middle drag + Left drag #6544

wants to merge 3 commits into from

Conversation

asl97
Copy link
Contributor

@asl97 asl97 commented Oct 19, 2017

Implement #2963.
A follow up on #6416.

The middle click is pretty simple, the left click, Honestly, I just throw in some code and got it to work base on the idea in #2963.

Dragging to non empty slot isn't support since it's much more complex, probably out of scope of this pr.

READ: left click drag is mostly just a Proof of Concept, is still mostly a Work in Process and probably Horribly Implemented.

Here is a break down of what I did for the left click

  • keep a list of the ItemSpec since the left click was press down
  • create an action list and update it every time a new ItemSpec get added
  • run all the action in the list when the left click was let go off.
  • action list is generated and ran on letting go off the left click as needed

The left click drag display update is only local (afaik).
Only after the left click is let go off then it sends the list of item move action.

There are a number of problem with the left click still, such as

(TODO):

  • The click not being detected properly sometimes.
  • The first slot where you start the click drag is wonky
  • After a while, the inventory is reset to before dragging until the button is let go off.
    (0.14 in video, seem like some kind of lag detection thingy which happen when the server didn't respond to the changes)
    Middle drag + Left drag #6544 (comment)
  • Figure out why m_selected_item is gone when the left click is let go off, it causes segmentation fault when the starting drag slot isn't the same as the slot the item came from.
    seem to be due to BLACK MAGIC cause by verifySelectedItem, the source of calling is still unknown.
    • Possible fix: create our own copy of m_selected_item just for left drag
      • Figure out blank image when dragging when using the copy
  • Remove all the debugging actionstream
  • right click select half (or other method), drag preview has missing unselected amount in the source slot
  • leftover preview is wrong when there more slot is selected than item.

Old video: https://www.youtube.com/watch?v=15lDdFRYdkI

@paramat paramat added @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature labels Oct 19, 2017
@asl97
Copy link
Contributor Author

asl97 commented Oct 21, 2017

Fixed the client revert by adding an inhibiting variable check.

@paramat paramat added the WIP The PR is still being worked on by its author and not ready yet. label Oct 21, 2017
@asl97
Copy link
Contributor Author

asl97 commented Oct 27, 2017

It should be done, Just need a lot of testing as well as commit squashing.

@asl97 asl97 changed the title Middle drag + (PoC, WIP) Left drag Middle drag + Left drag Oct 27, 2017
@paramat paramat removed the WIP The PR is still being worked on by its author and not ready yet. label Oct 27, 2017
@asl97 asl97 force-pushed the middle-drag branch 3 times, most recently from 627aa44 to ea8403d Compare October 27, 2017 12:29
@asl97
Copy link
Contributor Author

asl97 commented Oct 27, 2017

Removed unnecessary whitespace changes, re-added accidentally removed stuff, rebased and squashed into the three main commit.

@asl97
Copy link
Contributor Author

asl97 commented Oct 27, 2017

Just when I thought I am close to done, there are more edge cases in the preview/prediction.

@basicer
Copy link
Contributor

basicer commented Oct 27, 2017

clang gives this warning for me, which seems like a reasonable point.

src/guiFormSpecMenu.cpp:3805:37: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                                                        if (m_selected_amount - each >= 0){
                                                            ~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~

@asl97
Copy link
Contributor Author

asl97 commented Oct 27, 2017

@basicer The line number doesn't seem to match, is your copy outdated?
It should be safe to remove though, since it was a left over from when I just add 'slot to drag to' into the list without any condition.


I changed the TODO list from strikethough to a task list, there are two more known problem which need to be fix before merging.

One of them is simple, the other gonna need some time.

@paramat
Copy link
Contributor

paramat commented Dec 18, 2017

Middle mouse button needs to be rebindable to a key first, as some 'mouse equivalent devices' have no middle button and poor workarounds for middle-click.

@Fixer-007
Copy link
Contributor

Fixer-007 commented Dec 19, 2017

Why not merge this PR and theeen work on rebindable mouse keys? :) So it wont be held hostage for next 1000 years.

@paramat paramat added the Rebase needed The PR needs to be rebased by its author. label Feb 5, 2018
@paramat
Copy link
Contributor

paramat commented Aug 1, 2018

@asl97

I changed the TODO list from strikethough to a task list, there are two more known problem which need to be fix before merging.
One of them is simple, the other gonna need some time.

All boxes are ticked, to be clear is this ready now?

@asl97
Copy link
Contributor Author

asl97 commented Aug 2, 2018

It was but stuff happen, something about a refactor and now it's probably broken.

Honestly, this whole PR was mostly just me reacting to you closing #2963 (comment) with "Quite complex", "Old issue and no support, low priority" and not to mention, a ":-1:".

It's a feature that would improve the user experience of the crafting system.
Minetest crafting was (and probably still is) annoying with all the clicking required to place item in their slots.
As such, rejection base on those kind of reason just didn't sit well with me, and thus a PoC click drag was born.

I am gonna just close this since I don't really intent to work on it anymore, mostly because I don't really follow minetest development since... well, long ago.

Feel free to take over or reimplement it.

@asl97 asl97 closed this Aug 2, 2018
@paramat paramat added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Aug 4, 2018
@paramat
Copy link
Contributor

paramat commented Aug 4, 2018

I've removed my -1 from the issue, i don't disapprove anymore, that was a mistake.
The issue was closed only because there was no core dev support for 2+ years, if there is core dev support the issue will be reopened.
I didn't close it due to my own opinion or disapproval, it takes more than 1 disapproval to close an issue or PR.
I hope this will get some core dev attention other than mine.

@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants