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

[NO SQUASH] Allow binding dig, place actions to keys; remove LMB/RMB hardcoding #7924

Open
wants to merge 2 commits into
base: master
from

Conversation

@ClobberXD
Copy link
Contributor

commented Dec 1, 2018

[Adopted from #6537]

  • Remove hard-coded association between dig and place actions, with LMB and RMB respectively.
    before.
    • These actions are still bound to LMB and RMB by default.

Also, mouse buttons can be bound to actions like any other key on the
keyboard.

The old LMB and RMB player control fields still exist alongside dig and place, and this is to ensure mod compatibility.

Tested, works. This PR is ready for review.

TODO

  • Arrive upon a suitable name for the now-configurable LMB and RMB actions
  • Maybe test on Android???

Video demonstration

https://youtu.be/0Ca7nEMiq_U

How to test

  • Re-assign "dig" and/or "place" actions to different keys within the "change keys" dialog, and test the new keybinds.
  • Re-assign other actions to mouse buttons, and test the new keybinds.
  • Tweak repeat_place_interval, and test it by selecting a placeable item (e.g. a node), pointing at a node, and holding down the "place" keybind.
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch from 2d05e09 to da64b4f Dec 1, 2018
@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Rebased and fixed merge conflicts. But I'm not really happy with the choice of words used for both LMB (DIG) and RMB (PLACE) - LMB and RMB is also used for punching and misc. interacting with world respectively. USE and SECONDARY_USE would suit them better, IMHO. Thoughts?


Got some #include errors:

cannot open source file "Keycodes.h"
cannot open source file "IEventReceiver.h"

This PR is going to take some time to be review-ready...

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

Use reminds me of "aux1" and secondary_use reminds me of on_secondary_use node def value. But if that doesn't matter, USE and SECONDARY_USE sound good to me. Could also go with "add" and "subtract"; but this could turn into a lot of thought over a somewhat inconsequential detail.

Yes, but all references to that key now use aux1 only, and I assume it's safe to name LMB USE. I'm not as convinced about SECONDARY_USE for RMB, but it's still much better than PLACE which is misleading.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

@SmallJoker could you please elaborate on this? What exactly needs to be done to fix this compat breakage?

Change

lua_pushboolean(L, control.sneak);
lua_setfield(L, -2, "sneak");
lua_pushboolean(L, control.dig);
lua_setfield(L, -2, "dig");
lua_pushboolean(L, control.place);
lua_setfield(L, -2, "place");

to

	lua_pushboolean(L, control.dig);
	lua_setfield(L, -2, "dig");
	lua_pushboolean(L, control.place);
	lua_setfield(L, -2, "place");
	// Legacy fields to ensure mod compatibility
	lua_pushboolean(L, control.dig);
	lua_setfield(L, -2, "LMB");
	lua_pushboolean(L, control.place);
	lua_setfield(L, -2, "RMB");

Please also adjust the docs accordingly.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

secondary_use sounds better than use_secondary

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

When right clicking air, it's called "activating." So how about activate instead of secondary_use?

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

When right clicking air, it's called "activating." So how about activate instead of secondary_use?

Hmm... Press the activate button to open/close this chest - This doesn't sound as meaningful. The reason why I'm suggesting secondary_use is that it's much more generic and can apply to all the actions equally well. e.g. Press the secondary use button to open the door.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Yep, sounds good to me. TBH I'm just excited and a little anxious so don't mind me.

I can understand. :)

I just wish I can understand why there're include errors popping up in a couple of files.

@lhofhansl

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

I'm very much in favor of the idea (haven't had a chance to look at the changes, though)

@ClobberXD ClobberXD changed the title [WIP] Bind mouse buttons (Adopted from #6537) Bind mouse buttons (Adopted from #6537) Feb 23, 2019
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch 2 times, most recently from 7dc41ca to 05be41c Mar 13, 2019
@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Squashed the commits for ease of manageability.

@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch 2 times, most recently from a85a214 to 5771b4a Mar 13, 2019
@SmallJoker SmallJoker force-pushed the ClobberXD:bind_mouse_buttons branch 3 times, most recently from e7b4aad to a23283e Mar 30, 2019
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch from a23283e to 8dc5bf8 Apr 2, 2019
@rubenwardy rubenwardy force-pushed the minetest:master branch 2 times, most recently from 0e3b135 to 39c54e1 Jun 21, 2019
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch 2 times, most recently from 5f44caf to 5c1d6df Aug 3, 2019
lua_pushboolean(L, c.rightclickable);
lua_setfield(L, -2, "rightclickable");
lua_pushboolean(L, c.can_place_onto);
lua_setfield(L, -2, "can_place_onto");

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Sep 19, 2019

Contributor

If you change this, you have to document it in client_lua_api.txt.

lua_setfield(L, -2, "dig");
lua_pushboolean(L, control.place);
lua_setfield(L, -2, "place");
// Legacy fields to ensure mod compatibility

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Sep 19, 2019

Contributor

Missing documentation.

This comment has been minimized.

Copy link
@ClobberXD

ClobberXD Sep 20, 2019

Author Contributor

Yes, I'm yet to add documentation.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

Thank you @SmallJoker. I am curious, how much support is there? I understand changing a core fundamental such as this can be uncomfortable.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

@SmallJoker what's the point of rebasing if NO CORE DEV supports this?
http://irc.minetest.net/minetest-hub/2019-09-22#i_5595007

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@rubenwardy might be interested in this PR, as he self-requested a review in #6537 before it was closed.

While some extensive testing is recommended, this PR works for the most part. IMO, the main issue is the names, both in the code and the ones exposed to the Lua API. It'd be great if some core devs could pitch in and provide their opinions. For context, well... one would have to begin reading the comments right from the top of the page.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

There's a merge conflict now, and I'm not able to resolve it.

This happened before where I used the PR on my own, and I had to stop because I couldn't figure out how to resolve it. @ClobberXD thank you so much for your contribution, it's really appreciated from here.

It's not really fair to ask so much of contributors.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@rubenwardy https://forum.minetest.net/viewtopic.php?p=315289#p315289 this could have gotten into 5.1. It still could.

Regardless, can you help in supporting this? Just imagine when it drops, and the on the "New Features" list it shows: * Can now bind mouse buttons. I think there will be a fair amount of slow clapping.

Imagine all the people who tried Minetest, tried to bind their mouse buttons, and were surprised by what is rather unusual behavior. Consider DOS games in the early 1990's that allowed binding mouse buttons. Consider how odd this is, for people who might have a preference.

@sofar @Calinou Can I ask for your support? @ClobberXD has worked to resuscitate a neglected PR, and it stands a chance of happening again. Minetest has a LOT of features, and a LOT of them are unused, undiscovered, or unimportant.

This is not one of those features.

@Calinou

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

@jastevenson303 I'm not a core developer, so I'm afraid my approval wouldn't be worth anything special 🙁

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

I would like to see better input support, but I'm feeling too burnt out to review this. Even so, I don't think this should be in 5.1.0 as the deadline is too close now.

I don't see it as an essential feature like you claim. There are tons more features that are way more overdue, such as mobs and a working pathfinder, a main menu that doesn't stink, and particles which actually perform decently

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

@jastevenson303: Thanks for the loads of support you've shown towards this PR. I haven't seen the conflict yet, but I'm sure it's easily fixable. Also, I don't have the time to get this merged before 5.1.0, because remember, the PR itself works fine, but the bunch of naming issues is the elephant in the room, so to say. While I'd have loved to see this in 5.1.0, I agree with rubenwardy that this isn't essential to the release.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@SmallJoker SmallJoker added this to the 5.2.0 milestone Sep 25, 2019
@paramat

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

I'm sure many core devs 'support' the concept of this PR, i do too, but 'support' is different to 'approval'.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch from 77f3cb2 to 3358e44 Sep 26, 2019
@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Rebased. Needs testing to ensure that everything still works. I'm yet to find time to address the line comments.

I've also added another commit that fixes most of the code-style issues in src/gui/guiKeyChangeMenu.cpp

@ClobberXD ClobberXD changed the title Allow binding dig, place actions to keys; remove LMB/RMB hardcoding [NO SQUASH] Allow binding dig, place actions to keys; remove LMB/RMB hardcoding Sep 26, 2019
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch from 3358e44 to bfd851c Sep 26, 2019
ClobberXD and others added 2 commits Oct 16, 2017
Previously, the digging and placing actions were hardcoded to the left
and right mouse buttons respectively. This commit removes this
limitation, allowing these actions to be bound to any key. Dig and place
are still bound to the left and right mouse buttons by default as
before.

Also, mouse buttons can be bound to actions like any other key on the
keyboard.

The old `LMB` and `RMB` player control fields still exist alongside `dig` and `place`, and this is to ensure mod compatibility

Co-authored-by: Sam Caulfield <sam@samcaulfield.com>
@ClobberXD ClobberXD force-pushed the ClobberXD:bind_mouse_buttons branch from bfd851c to 7ea3d51 Sep 26, 2019
@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

Tested and works.

@Ferk

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

About the naming of the actions, to me the right name for the right-button would be "interact".

When holding a The main difference between the "on_use" and the (ill-named) "on_rightclick" is the fact that on_right_click requires a pointed thing. At least the last time I checked (which was quite a while ago).

-checks lua_api -

Uhm.. sorry, I think I'm too late. The name was actually changed from on_rightclick to on_secondary_use already.
Was a change also made to have the action be triggered even when you are pointing at the sky or the nothing?

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

About the naming of the actions, to me the right name for the right-button would be "interact".

All actions done by the player falls under "interact", incl. both LMB and RMB actions :)

And yes, on_secondary_use is triggered when a player right-clicks, while pointing at nothing. on_place is triggered when the player points at an object or a node.

@Ferk

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

All actions done by the player falls under "interact", incl. both LMB and RMB actions :)

Not necessarily.
You can have "on_use" trigger a change in the item itself without interacting with the environment.
Like for example, when you eat an apple, you are not interacting with anything external, you are using the apple itself, not applying it to interact with the thing the apple is pointing at.

And yes, on_secondary_use is triggered when a player right-clicks, while pointing at nothing.

Ok, I'm glad that was changed then. In that case this action is no longer meant to be used for interacting with the world, and this makes it indeed basically the same as on_use, so in that case on_secondary_use makes sense.

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

You can have "on_use" trigger a change in the item itself without interacting with the environment.

Both LMB and RMB actions require the interact priv, and are hence considered interaction, at least by Minetest. That's what I meant.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

When this PR is rebased and all issues are addressed, will you approve it?

Although i support the concept, approving depends on me reviewing and testing and the PR being fit for merge, so i cannot say in advance. I might review but it is not high priority for me.

Had a look through the PR, big and many issues still to consider, not surprising it cannot be ready for 5.1.0.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.