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

modifier actions will shadow simple actions if flag=false (fix #18793) #32457

Closed

Conversation

AlexHolly
Copy link
Contributor

@AlexHolly AlexHolly commented Oct 1, 2019

It will keep the current input behavior, if needed is_action_pressed_on_modifier=false can be set.
(also improved as_text() for modifiers)

Test script:

extends Control

const is_action_pressed_on_modifier = 0

func _ready():
	add_key("Save", get_combo([KEY_CONTROL], KEY_S))
	add_key("Select S", get_keyboard_event(KEY_S), is_action_pressed_on_modifier)
	
	add_key("Run", get_keyboard_event(KEY_SHIFT))
	add_key("Jump", get_keyboard_event(KEY_SPACE), is_action_pressed_on_modifier)
	add_key("Super Jump", get_combo([KEY_SHIFT], KEY_SPACE))

func get_combo(modifiers, key):
	var new_event = InputEventKey.new()
	new_event.scancode = key
	
	for m in modifiers:
		if typeof(m)==TYPE_INT:
			if m==KEY_ALT:
				new_event.set_alt(true)
			if m==KEY_SHIFT:
				new_event.set_shift(true)
			if m==KEY_CONTROL:
				new_event.set_control(true)
			if m==KEY_META:
				new_event.set_meta(true)
			if m==KEY_MASK_CMD:
				new_event.set_command(true)
	
	return new_event

func get_keyboard_event(key):
	var new_event = InputEventKey.new()
	new_event.scancode = key
	return new_event

func add_key(action, key_event, _is_action_pressed_on_modifier=true):
	key_event.set_action_pressed_on_modifier(_is_action_pressed_on_modifier)
	
	if !InputMap.has_action(action):
		InputMap.add_action(action)
		
	InputMap.action_add_event(action, key_event)

func _input(event):
	if(Input.is_action_just_pressed("Save")):
		print("Save")
	if(Input.is_action_just_pressed("Select S")):
		print("Select S")
	
	if(Input.is_action_just_pressed("Run")):
		print("Run")
	if(Input.is_action_just_pressed("Jump")):
		print("Jump")
	if(Input.is_action_just_pressed("Super Jump")):
		print("Super Jump")

Bugsquad edit: Fixes #18793.

@AlexHolly AlexHolly force-pushed the actions-with-modifier-keys branch 2 times, most recently from 6854b22 to 25327fe Compare October 1, 2019 05:45
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "alt"), "set_alt", "get_alt");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "shift"), "set_shift", "get_shift");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "control"), "set_control", "get_control");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "meta"), "set_metakey", "get_metakey");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "command"), "set_command", "get_command");

ADD_PROPERTY(PropertyInfo(Variant::BOOL, "isActionPressedOnModifier"), "set_action_pressed_on_modifier", "is_action_pressed_on_modifier");
Copy link
Member

Choose a reason for hiding this comment

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

You're a long time Godot user, you might have figured by now that all properties are snake_case ;)

Copy link
Member

Choose a reason for hiding this comment

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

And the property should be action_pressed_on_modifier, not with is_ prefix (that's the getter).

That being said, this is probably not a wanted addition, but just mentioning the style issue in any case.

@akien-mga
Copy link
Member

IMO this should go through https://github.com/godotengine/godot-proposals before making a PR, to define what should be implemented if anything.

@groud
Copy link
Member

groud commented Oct 1, 2019

I am kind of puzzled with the implementation. IMHO, this too specific to the problem. Whether or not to set is_action_pressed_on_modifier depends on the shortcut you assign to the action, while actions can have their shortcut reassigned.
I think the more general problem is more like having an action taking over another one.

Let's take an example with the 2D editor. You bind:

  • ctrl to the rotation,
  • s to the scale,
  • ctrl+s to save.

Here, if you press ctrl, then s (not releasing ctrl) you want the rotation to be active as soon a ctrl is pressed, but don't want to trigger the scale when pressing s. You also want the rotation to stay active the whole time to avoid the screen "blinking" when you press s. So basically the save action takes over the scale action but not the rotation one.

I think this has to be discussed a little bit more. There is probably a smart way to fix this. I was thinking about distinguishing "state" actions from "event" action, so that we can determine which one can coexist ? Or maybe a priority system could be implemented, whether by using a priority integer or by setting, for each action, a list of action that are taken over ?

I am not even sure this feature is desired, as in the original issue you could just have used an if/else statement to solve the problem. So it might not worth the trouble too.

@AlexHolly
Copy link
Contributor Author

@akien-mga issue #18793 is open since 11 May 2018 I don't think a third place is needed to discuss it.
@groud

Let's take an example with the 2D editor. You bind:

Can you make a script or proper example which can be tested like tinmanjuggernaut did in the issue, so I can test the current solution. Using some rotation and trying to save at the same time sounds very strange.
Only save causes an issue, on Control+s you want to save (no scale activation), Rotate will be actived which is fine there is no way to avod it (not sure why it should blink, save will only run once).

	add_key("Rotate", get_keyboard_event(KEY_CONTROL), true)
	add_key("Scale", get_keyboard_event(KEY_S), false)
	add_key("Save", get_combo([KEY_CONTROL], KEY_S), true)
...
	if(Input.is_action_just_pressed("Rotate")):
		print("Rotate")
	if(Input.is_action_just_pressed("Scale")):
		print("Scale")
	if(Input.is_action_just_pressed("Save")):
		print("Save")

I think this has to be discussed a little bit more. There is probably a smart way to fix this. I was thinking about distinguishing "state" actions from "event" action, so that we can determine which one can coexist ? Or maybe a priority system could be implemented, whether by using a priority integer or by setting, for each action, a list of action that are taken over ?

I am open for discussion, this solution is simple and seems to work.

I am not even sure this feature is desired, as in the original issue you could just have used an if/else statement to solve the problem. So it might not worth the trouble too.

Like I said #18793:

This works but I can't use it in my case

The Input checks happen in different scripts.

@SleepProgger
Copy link

This would be really nice if it could be accepted.
Another use case:
Assume ctrl+a selects all units and a moves the camera to the left.
There should be neither camera movement when selecting all units nor should all units be selected when moving to the left.
My current simple workaround in case it helps someone:

func _is_event_action(action, event):
    if not event is InputEventKey:
        return InputMap.action_has_event(action, event)
    for template in InputMap.get_action_list(action):
        var code = template.get_scancode_with_modifiers()
        if event.get_scancode_with_modifiers() == code:
            return true
    return false

@neqs20
Copy link

neqs20 commented Jun 27, 2020

Seems like the idea got abandoned. Is there any chance that this will be implemented in 4.0 ?

@aaronfranke
Copy link
Member

@AlexHolly Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

@AlexHolly
Copy link
Contributor Author

Obviously it is still desired, otherwise people would not ask.

My pull requests are not abandoned. You guys are just unable to merge them after I resolved the conflicts. Stop asking for it if you are not ready to merge. Let me help you to resolve this one.

@aaronfranke
Copy link
Member

aaronfranke commented Jul 1, 2020

@AlexHolly Try not to assume mal-intent where there is none. Godot has almost 800 open PRs and it's a very difficult task to look through them all. Sometimes PRs go stale for a long time, so we want to know if the author is still interested and active.

In your particular case, you were given feedback 9 months ago. You were told to open a proposal and haven't. Also, your PR is not ready to merge, there are style issues (ex: line 189 should not exist). If you rebased as asked, the CI checks would have caught this.

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

Successfully merging this pull request may close these issues.

modifier+key action shows also key action as pressed
6 participants