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

InputEvent as_text now returns readable string. Added to_string for debug strings #43660

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Nov 19, 2020

InputEvent changed as_text() to return a display-friendly and human readable string, to be used for UI display. to_string() can be used to get a non-display friendly version which has more information for debugging. Before, some as_text() methods returned a nice readable string, and some displayed more debug-style information.

Based on Calinou comment #38326 (comment)

This is the first commit of a larger series based on #42600.

@EricEzaM EricEzaM force-pushed the PR/INP1-inputevent_as_text_and_to_string branch 2 times, most recently from a1aa690 to 8da03f4 Compare November 19, 2020 00:41
@akien-mga akien-mga added this to the 4.0 milestone Nov 19, 2020
@akien-mga
Copy link
Member

akien-mga commented Nov 19, 2020

This will likely conflict with #43591 (or the other way around, depending on which is merged first).

On the other hand it can go in the same direction as the proposal on #43591 (comment) to remove these methods:

    int get_joy_axis_index_from_string ( String axis )
    String get_joy_axis_string ( int axis_index )
    int get_joy_button_index_from_string ( String button )
    String get_joy_button_string ( int button_index )

Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

See below for specifics, but this PR includes a duplication of descriptions for joypad buttons and axes that needs to be addressed.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 19, 2020

@madmiraal ah yep in the original pr of #42600 I completely remove the input map editor, as I make it generic (action_map_editor.cpp), so in that pr this issue does not exist. Here it seems I will have to edit the input map editor to use as_text rather than constructing its own strings everywhere.

Seeing input map editor construct the representation of the events on its own is what made me want to do this change in the first place, since the text representation of an event should be consistent across the engine.

@EricEzaM
Copy link
Contributor Author

@madmiraal @akien-mga Ok, finished the above reviews up. Also added some text to the descriptions as per https://github.com/godotengine/godot/pull/43591/files#r52860139, and also rebased with newest master, which includes #43591

@EricEzaM EricEzaM force-pushed the PR/INP1-inputevent_as_text_and_to_string branch 2 times, most recently from 6ee328c to 5b0399f Compare November 23, 2020 11:54
}

return vformat(RTR("Joypad Motion on Axis %s%s with Value %s"), itos(axis), description, String(Variant(axis_value)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is something wrong here. It looks like you're missing the + or - after the axis number and before the description in parentheses and instead are adding "with Value ±1" to the end. Is this intentional? This is what the Input Map Editor currently presents:

Axis Selection

Copy link
Contributor Author

@EricEzaM EricEzaM Nov 23, 2020

Choose a reason for hiding this comment

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

Yeah it looks wrong:

image

What about Joypad Motion on Axis 0, Value -1 (Left Stick Left, Joystick 0 Left), i.e. putting the value before the description. I think I actually meant to do this, as how I currently have it in the image above doesn't make much sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps there does need to be a separation between the as_text() for the event and the 20 direction descriptions used by Input Map Editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice if it could be avoided but maybe it can't. To keep the descriptions in one place though, a method to get the axis/button descriptions would need to be introduced (i.e. get_axis_description(int axis))

Is there a particular issue you have with Joypad Motion on Axis 0, Value -1 (Left Stick Left, Joystick 0 Left)? This will also likely come along with a different event editor (shown below) which was a part of #42600 which I have now split up into multiple prs.

new_dialog_demo

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular issue you have with Joypad Motion on Axis 0, Value -1 (Left Stick Left, Joystick 0 Left)?

I just think there is a difference between the event description, which is axis specific and the Input Map description, which is half axis specific:
Event description: Joypad Motion on Axis 0, (Left Stick X-Axis, Joystick 0 X-Axis) with Value -0.3
Input Map description: Joypad Axis 0 - (Left Stick Left, Joystick 0 Left)

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think the differences between the event descriptions (10 axis specific), and the Input Map descriptions (20 half-axis specific) are enough to justify two different lists of descriptions for the two different needs:

Event descriptions:

  • Joypad Motion on Axis 0, (Left Stick X-Axis, Joystick 0 X-Axis) with Value -0.3
  • Joypad Motion on Axis 0, (Left Stick X-Axis, Joystick 0 X-Axis) with Value 0.3
  • Joypad Motion on Axis 1, (Left Stick Y-Axis, Joystick 0 Y-Axis) with Value -0.3
  • Joypad Motion on Axis 1, (Left Stick Y-Axis, Joystick 0 Y-Axis) with Value 0.3

Input Map descriptions:

  • Joypad Axis 0 (Left Stick Left, Joystick 0 Left)
  • Joypad Axis 0 + (Left Stick Right, Joystick 0 Right)
  • Joypad Axis 1 (Left Stick Up, Joystick 0 Up)
  • Joypad Axis 1 + (Left Stick Down, Joystick 0 Down)

I've highlighted the differences above for one joystick (two axes and the four directions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you propose a method specific for InputEventJoypadMotion? In addition to as_text(), something like input_map_description() (bad example but you know what I mean)?

Copy link
Member

Choose a reason for hiding this comment

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

If it's specific to the input map, it should only be in the input map editor code IMO and not an extra method in InputEvents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, all done. Changed as_text() to contain 10 options for InputEventJoyMotion, and added joy axis descriptions to Input Map Editor such that the description meets the above examples by @madmiraal.

See lines 358-390 of input_map_editor.cpp for the main changes in this iteration

@EricEzaM EricEzaM force-pushed the PR/INP1-inputevent_as_text_and_to_string branch from 5b0399f to 6f438c1 Compare November 24, 2020 07:49
@EricEzaM EricEzaM requested a review from madmiraal November 25, 2020 02:20
@EricEzaM EricEzaM force-pushed the PR/INP1-inputevent_as_text_and_to_string branch from 6f438c1 to 50e2be7 Compare November 26, 2020 02:46
Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

Aside from the typo above, I think it's good to go.

…ebug strings

Made InputEvent as_text() return a readable and presentable string. Added to_string() overrides for each which returns a 'debug-friendly' version which is not as presentable but provides more information and in a more structured fashion. Use as_text() for UI display scenarions and to_string() for debug cases
@EricEzaM EricEzaM force-pushed the PR/INP1-inputevent_as_text_and_to_string branch from 50e2be7 to dfe4c5f Compare November 26, 2020 14:42
@EricEzaM
Copy link
Contributor Author

All sorted now, thanks!

@akien-mga akien-mga merged commit df53bf6 into godotengine:master Dec 3, 2020
@akien-mga
Copy link
Member

Thanks!

@hsandt
Copy link
Contributor

hsandt commented Oct 30, 2022

Hi, I just got the issue of listening to gamepad axis showing the long readable string, but not the one needed to add an input mapping, in v4.0.beta.custom_build [767f8fb]

image

I'm looking at the code in editor/input_event_configuration_dialog.cpp and can clearly see the quickfix under

// Joypad motion events will display slightly differently than what the event->as_text() provides. See #43660.

but it doesn't seem to be applied.

I couldn't make debugging work in my VS Code so I don't know why the quickfix doesn't work exactly (I'll try to rebuild the whole project with llvm but it will take time, and I have a few things to finish on my project first).

Should I open a new bug report, or let you handle this in this issue?

@Zireael07
Copy link
Contributor

This PR has been merged 2 years ago. Open a new issue.

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.

6 participants