Print suggestions when requesting a nonexistent InputMap action

[Calinou]

This should make debugging input actions a bit easier. In the future, we can extend this to get_node() errors, signal connection errors and much more. If no action name is close enough to the requested action, no suggestion will be made.

3.2 version of this PR: #45902

Preview

ERROR: event_get_action_status: The InputMap action "move_forwards" doesn't exist. Did you mean "move_forward"?
   At: core/input_map.cpp:241.

Note that this method is fairly slow (taking 2-5 ms to run in a debug build). Thankfully, it's only called in debug builds when the input action doesn't exist, so the impact should be minimal in a release build.

@Zylann Is there a way to optimize this? I'm not sure if putting the Comparator struct outside the function would improve the performance (to avoid creating it every time).

Edit: The method has been optimized, it takes about 100-150 µs to run now.

[Zylann]

@Zylann Is there a way to optimize this? I'm not sure if putting the Comparator struct outside the function would improve the performance (to avoid creating it every time).

Where the comparator resides doesn't cause any performance issues. It would be the fact that the similarity between strings would be computed multiple times during sorting (since it's done on the fly). Computing it once for each and sorting in a second pass would be faster, if you think there can be a lot of elements to sort.

Maybe something like:

	struct Pair {
		StringName action;
		int similarity; // dunno if it's float or int?
	};
	
	// Convert the List to a Vector to use a custom sorting function.
	for (int i = 0; i < actions.size(); i++) {
		actions_vector.write[i] = { actions[i], (String(actions[i])).similarity(base_name) };
	}

	// Sort the vector's elements from the most to least similar.
	struct Comparator {
		inline bool operator()(const Pair &a, const Pair &b) const {
			return a.similarity < b.similarity;
		}
	};

	SortArray<Pair, Comparator> sorter;
	sorter.compare.base_name = p_action;
	sorter.sort(actions_vector.ptrw(), actions_vector.size());

taking 2-5 ms to run in a debug build

Wow, debug is slow but that's still a lot... that's just the sorting part? How many elements is it sorting?
Also, I see you print only the first element of the final array. Do you need the others? If not, you could do a min algorithm instead of a sort.

[bojidar-bg]

It might be worth to somehow generalize this function, so that it can be used for missing variables / methods / etc.

[Calinou]

@Zylann It turns out I don't need to do any sorting, as I only need the most similar item. The new method is about 10 times faster on average 🙂

[Zylann]

@Calinou and if you want it even faster, iterate the list using for(List<StringName>::Element *E = actions.front(); E; E = E->next()), instead of [i], because the latter will search from first element every time to find the i-th one :p

[Calinou]

@Zylann Thanks for the suggestion, it's even faster now (100-150 µs).

[aaronfranke]

@Calinou Is there any update on this? This looks like it would be a great help for beginners and anyone prone to typos.

[akien-mga]

Needs a rebase, otherwise should be fine to merge.

To be fair I'm not convinced that adding such contextual info to runtime errors is really a good UX, but I guess it's better than nothing.

[aaronfranke]

What if this was inside of #ifdef DEBUG_ENABLED or maybe #ifdef TOOLS_ENABLED?

[groud]

To be fair I'm not convinced that adding such contextual info to runtime errors is really a good UX, but I guess it's better than nothing.

I remember someone asking on reddit why their Input.is_action_pressed("ui-left") call was not working.
I definitely think that this PR would have helped there, especially because I think such runtime errors are sometimes hard to fix.

Edit: see:
https://www.reddit.com/r/godot/comments/81nnwn/having_problems_with_inputis_action_pressed/
https://www.reddit.com/r/godot/comments/ld0frp/get_action_strength_not_working/

[akien-mga]

What if this was inside of #ifdef DEBUG_ENABLED or maybe #ifdef TOOLS_ENABLED?

That shouldn't be necessary, the error macros already ignore the message if DEBUG_ENABLED is not defined. (See DEBUG_STR define.)

[akien-mga]

To be fair I'm not convinced that adding such contextual info to runtime errors is really a good UX, but I guess it's better than nothing.

I remember someone asking on reddit why their Input.is_action_pressed("ui-left") call was not working.
I definitely think that this PR would have helped there, especially because I think such runtime errors are sometimes hard to fix.

Edit: see:
https://www.reddit.com/r/godot/comments/81nnwn/having_problems_with_inputis_action_pressed/
https://www.reddit.com/r/godot/comments/ld0frp/get_action_strength_not_working/

Yeah, it's just that from those two threads it doesn't appear like those users even looked at the errors in the debugger/console, so whether the error says Request for nonexistent InputMap action 'ul_right'. or Request for nonexistent InputMap action "ul_right". Did you mean "ui_right"? will maybe not make much difference.

To me, it's clear enough if you're told that a given action name doesn't exist, that you might indeed be using an action that doesn't exist - so you check the InputMap and your code to make sure it's correct. If those two users failed to do that, they likely did not even see the error, which is a different UX problem.

All that being said, I'm fine with the change if it can help some users at least.

[Calinou]

Yeah, it's just that from those two threads it doesn't appear like those users even looked at the errors in the debugger/console, so whether the error says Request for nonexistent InputMap action 'ul_right'. or Request for nonexistent InputMap action "ul_right". Did you mean "ui_right"? will maybe not make much difference.

I think this is the kind of error that should pause project execution and raise the editor window, similar to GDScript errors. (get_node() calls with invalid paths should also be modified to do that.) However, we can't implement that yet.

[Calinou]

Rebased.

PS: I noticed that no error message is printed at all when using Input.is_action_pressed (it's only printed when using event.is_action_pressed). This should be addressed in a future PR.

[Calinou]

Rebased and tested on the latest master branch, it works successfully. One limitation is that suggestions are only displayed for checks of style if event.is_action_pressed(...) (where event is an InputEvent), not if Input.is_action_pressed(...). I'd prefer to address this in a future PR so we can keep this one as close as possible to the 3.x version.

Since the 3.x version of this PR has been merged, we should merge this one as well so I can keep working on this. This way, I can make suggestions more universal in both 3.x and master simultaneously 🙂

[akien-mga]

Thanks!