From cc6a181ed8e5223cb2eb8677adf32900e9119244 Mon Sep 17 00:00:00 2001 From: Eric M Date: Fri, 25 Mar 2022 23:56:25 +1000 Subject: [PATCH] Action Map Editor fixes and improvements Multiple fixes: * Fixed device not being updated correctly (changing the device dropdown did not actually change the underlying event's device) * Device selection now defaults to `All Devices` * Device is now displayed in the text shown in the input event editor, and in the action list. * There was code running twice previously because of the following interaction: 1) input via "Listen for Input" tab. 2) `_set_event` gets called. 3) input list tree selection gets updated to match the listened event. 4) tree "item_selection" signal is emitted. 5) eventually `_set_event` is called again. * The only reason this was not an infinite loop is because reselection is disabled on the tree. So, the code runs twice - not a big deal, but it is unnecessary processing and it could cause an issue in the future. If someone turns on reselection on the tree, the whole thing comes crashing down. This should prevent that scenario and make the code a bit safer and more robust. --- editor/action_map_editor.cpp | 105 +++++++++++++++++++++++------------ editor/action_map_editor.h | 9 +-- 2 files changed, 75 insertions(+), 39 deletions(-) diff --git a/editor/action_map_editor.cpp b/editor/action_map_editor.cpp index 96931efd3b3..86b40a311e3 100644 --- a/editor/action_map_editor.cpp +++ b/editor/action_map_editor.cpp @@ -61,29 +61,37 @@ static const char *_joy_axis_descriptions[(size_t)JoyAxis::MAX * 2] = { TTRC("Joystick 4 Down"), }; -String InputEventConfigurationDialog::get_event_text(const Ref &p_event) { +String InputEventConfigurationDialog::get_event_text(const Ref &p_event, bool p_include_device) const { ERR_FAIL_COND_V_MSG(p_event.is_null(), String(), "Provided event is not a valid instance of InputEvent"); - // Joypad motion events will display slightly differently than what the event->as_text() provides. See #43660. - Ref jpmotion = p_event; - if (jpmotion.is_valid()) { + String text = p_event->as_text(); + + Ref mouse = p_event; + Ref jp_motion = p_event; + Ref jp_button = p_event; + if (jp_motion.is_valid()) { + // Joypad motion events will display slightly differently than what the event->as_text() provides. See #43660. String desc = TTR("Unknown Joypad Axis"); - if (jpmotion->get_axis() < JoyAxis::MAX) { - desc = RTR(_joy_axis_descriptions[2 * (size_t)jpmotion->get_axis() + (jpmotion->get_axis_value() < 0 ? 0 : 1)]); + if (jp_motion->get_axis() < JoyAxis::MAX) { + desc = RTR(_joy_axis_descriptions[2 * (size_t)jp_motion->get_axis() + (jp_motion->get_axis_value() < 0 ? 0 : 1)]); } - return vformat("Joypad Axis %s %s (%s)", itos((int64_t)jpmotion->get_axis()), jpmotion->get_axis_value() < 0 ? "-" : "+", desc); - } else { - return p_event->as_text(); + text = vformat("Joypad Axis %s %s (%s)", itos((int64_t)jp_motion->get_axis()), jp_motion->get_axis_value() < 0 ? "-" : "+", desc); } + if (p_include_device && (mouse.is_valid() || jp_button.is_valid() || jp_motion.is_valid())) { + String device_string = _get_device_string(p_event->get_device()); + text += vformat(" - %s", device_string); + } + + return text; } -void InputEventConfigurationDialog::_set_event(const Ref &p_event) { +void InputEventConfigurationDialog::_set_event(const Ref &p_event, bool p_update_input_list_selection) { if (p_event.is_valid()) { event = p_event; // Update Label - event_as_text->set_text(get_event_text(event)); + event_as_text->set_text(get_event_text(event, true)); Ref k = p_event; Ref mb = p_event; @@ -122,7 +130,7 @@ void InputEventConfigurationDialog::_set_event(const Ref &p_event) { additional_options_container->show(); // Update selected item in input list. - if (k.is_valid() || joyb.is_valid() || joym.is_valid() || mb.is_valid()) { + if (p_update_input_list_selection && (k.is_valid() || joyb.is_valid() || joym.is_valid() || mb.is_valid())) { TreeItem *category = input_list_tree->get_root()->get_first_child(); while (category) { TreeItem *input_item = category->get_first_child(); @@ -234,10 +242,13 @@ void InputEventConfigurationDialog::_listen_window_input(const Ref & } } + // Create an editable reference + Ref received_event = p_event; + // Check what the type is and if it is allowed. - Ref k = p_event; - Ref joyb = p_event; - Ref joym = p_event; + Ref k = received_event; + Ref joyb = received_event; + Ref joym = received_event; int type = 0; if (k.is_valid()) { @@ -266,7 +277,7 @@ void InputEventConfigurationDialog::_listen_window_input(const Ref & } if (k.is_valid()) { - k->set_pressed(false); // to avoid serialisation of 'pressed' property - doesn't matter for actions anyway. + k->set_pressed(false); // To avoid serialisation of 'pressed' property - doesn't matter for actions anyway. // Maintain physical keycode option state if (physical_key_checkbox->is_pressed()) { k->set_keycode(Key::NONE); @@ -275,15 +286,17 @@ void InputEventConfigurationDialog::_listen_window_input(const Ref & } } - Ref mod = p_event; + Ref mod = received_event; if (mod.is_valid()) { // Maintain store command option state mod->set_store_command(store_command_checkbox->is_pressed()); - mod->set_window_id(0); } - _set_event(p_event); + // Maintain device selection. + received_event->set_device(_get_current_device()); + + _set_event(received_event); set_input_as_handled(); } @@ -331,7 +344,7 @@ void InputEventConfigurationDialog::_update_input_list() { Ref mb; mb.instantiate(); mb->set_button_index(mouse_buttons[i]); - String desc = get_event_text(mb); + String desc = get_event_text(mb, false); if (!search_term.is_empty() && desc.findn(search_term) == -1) { continue; @@ -354,7 +367,7 @@ void InputEventConfigurationDialog::_update_input_list() { Ref joyb; joyb.instantiate(); joyb->set_button_index((JoyButton)i); - String desc = get_event_text(joyb); + String desc = get_event_text(joyb, false); if (!search_term.is_empty() && desc.findn(search_term) == -1) { continue; @@ -380,7 +393,7 @@ void InputEventConfigurationDialog::_update_input_list() { joym.instantiate(); joym->set_axis((JoyAxis)axis); joym->set_axis_value(direction); - String desc = get_event_text(joym); + String desc = get_event_text(joym, false); if (!search_term.is_empty() && desc.findn(search_term) == -1) { continue; @@ -495,7 +508,7 @@ void InputEventConfigurationDialog::_input_list_item_selected() { k->set_meta_pressed(mod_checkboxes[MOD_META]->is_pressed()); k->set_store_command(store_command_checkbox->is_pressed()); - _set_event(k); + _set_event(k, false); } break; case InputEventConfigurationDialog::INPUT_MOUSE_BUTTON: { MouseButton idx = (MouseButton)(int)selected->get_meta("__index"); @@ -510,12 +523,19 @@ void InputEventConfigurationDialog::_input_list_item_selected() { mb->set_meta_pressed(mod_checkboxes[MOD_META]->is_pressed()); mb->set_store_command(store_command_checkbox->is_pressed()); - _set_event(mb); + // Maintain selected device + mb->set_device(_get_current_device()); + + _set_event(mb, false); } break; case InputEventConfigurationDialog::INPUT_JOY_BUTTON: { JoyButton idx = (JoyButton)(int)selected->get_meta("__index"); Ref jb = InputEventJoypadButton::create_reference(idx); - _set_event(jb); + + // Maintain selected device + jb->set_device(_get_current_device()); + + _set_event(jb, false); } break; case InputEventConfigurationDialog::INPUT_JOY_MOTION: { JoyAxis axis = (JoyAxis)(int)selected->get_meta("__axis"); @@ -525,24 +545,35 @@ void InputEventConfigurationDialog::_input_list_item_selected() { jm.instantiate(); jm->set_axis(axis); jm->set_axis_value(value); - _set_event(jm); + + // Maintain selected device + jm->set_device(_get_current_device()); + + _set_event(jm, false); } break; } } -void InputEventConfigurationDialog::_set_current_device(int i_device) { - device_id_option->select(i_device + 1); +void InputEventConfigurationDialog::_device_selection_changed(int p_option_button_index) { + // Subtract 1 as option index 0 corresponds to "All Devices" (value of -1) + // and option index 1 corresponds to device 0, etc... + event->set_device(p_option_button_index - 1); + event_as_text->set_text(get_event_text(event, true)); +} + +void InputEventConfigurationDialog::_set_current_device(int p_device) { + device_id_option->select(p_device + 1); } int InputEventConfigurationDialog::_get_current_device() const { return device_id_option->get_selected() - 1; } -String InputEventConfigurationDialog::_get_device_string(int i_device) const { - if (i_device == InputMap::ALL_DEVICES) { +String InputEventConfigurationDialog::_get_device_string(int p_device) const { + if (p_device == InputMap::ALL_DEVICES) { return TTR("All Devices"); } - return TTR("Device") + " " + itos(i_device); + return TTR("Device") + " " + itos(p_device); } void InputEventConfigurationDialog::_notification(int p_what) { @@ -588,6 +619,9 @@ void InputEventConfigurationDialog::popup_and_configure(const Ref &p // Switch to "Listen" tab tab_container->set_current_tab(0); + + // Select "All Devices" by default. + device_id_option->select(0); } popup_centered(); @@ -673,12 +707,13 @@ InputEventConfigurationDialog::InputEventConfigurationDialog() { device_id_option = memnew(OptionButton); device_id_option->set_h_size_flags(Control::SIZE_EXPAND_FILL); - device_container->add_child(device_id_option); - for (int i = -1; i < 8; i++) { device_id_option->add_item(_get_device_string(i)); } - _set_current_device(0); + device_id_option->connect("item_selected", callable_mp(this, &InputEventConfigurationDialog::_device_selection_changed)); + _set_current_device(InputMap::ALL_DEVICES); + device_container->add_child(device_id_option); + device_container->hide(); additional_options_container->add_child(device_container); @@ -1096,7 +1131,7 @@ void ActionMapEditor::update_action_list(const Vector &p_action_info TreeItem *event_item = action_tree->create_item(action_item); // First Column - Text - event_item->set_text(0, event_config_dialog->get_event_text(event)); // Need to us the special description for JoypadMotion here, so don't use as_text() directly. + event_item->set_text(0, event_config_dialog->get_event_text(event, true)); event_item->set_meta("__event", event); event_item->set_meta("__index", evnt_idx); diff --git a/editor/action_map_editor.h b/editor/action_map_editor.h index 34c70c942e5..48757ccb581 100644 --- a/editor/action_map_editor.h +++ b/editor/action_map_editor.h @@ -97,7 +97,7 @@ private: CheckBox *physical_key_checkbox; - void _set_event(const Ref &p_event); + void _set_event(const Ref &p_event, bool p_update_input_list_selection = true); void _tab_selected(int p_tab); void _listen_window_input(const Ref &p_event); @@ -110,9 +110,10 @@ private: void _store_command_toggled(bool p_checked); void _physical_keycode_toggled(bool p_checked); - void _set_current_device(int i_device); + void _device_selection_changed(int p_option_button_index); + void _set_current_device(int p_device); int _get_current_device() const; - String _get_device_string(int i_device) const; + String _get_device_string(int p_device) const; protected: void _notification(int p_what); @@ -121,7 +122,7 @@ public: // Pass an existing event to configure it. Alternatively, pass no event to start with a blank configuration. void popup_and_configure(const Ref &p_event = Ref()); Ref get_event() const; - String get_event_text(const Ref &p_event); + String get_event_text(const Ref &p_event, bool p_include_device) const; void set_allowed_input_types(int p_type_masks);