Fix wrong focus neighbor for grid-aligned 0 separation controls #95500
Conversation
tetrapod00
force-pushed
the
zero-separation-focus-neighbor
branch
from
9d6a8eb to
41918a7
Compare
|
Ah, there's another case, where tie-breaking by distance is incorrect. Video of my PR: godot.windows.editor.x86_64_Kv4dC3rqTG.mp4 |
|
This video shows tie-breaking the prospective neighbors by the which one is most aligned with the desired input direction. This works in the basic grid-aligned case, and it works well enough for the center button in the edge case presented. However, it does not work intuitively when moving from the large buttons to the small ones. godot.windows.editor.x86_64_WioxLWvdyS.mp4Satisfying all the intuitive behaviors of neighbor focus would require rethinking the entire selection algorithm, rather than just patching a tie-breaking step at the end. I propose solving the narrow problem of 0 separation grid-aligned controls in this PR, using tie-breaking by the closest aligned control. This solves the obvious pain-point of a 0 separation GridContainer, as well as some other grid-aligned cases. In other cases, users can add manual neighbor overrides (as they are probably already doing). Existing behavior in non-zero separation cases will remain unchanged. Updated MRP with the edge case: |
tetrapod00
force-pushed
the
zero-separation-focus-neighbor
branch
from
41918a7 to
9c66f49
Compare
tetrapod00
force-pushed
the
zero-separation-focus-neighbor
branch
from
09cf070 to
ade7c1e
Compare
|
In your new test project it's not possible to select corner buttons: godot.windows.editor.dev.x86_64_dqu2hROmQK.mp4You can select them using Tab, but the order is odd. With old behavior it was at least partially possible: godot_gCjmVNLvrB.mp4That's a corner case though, so probably fine. |
My goal with this PR was to make the common case behave correctly, and make it the user's responsibility to use manual neighbors to fix literal corner cases like this. As it is, the common case is already wrong and the user's responsibility to fix with manual neighbors, so this is an improvement If it's a hard requirement that every control on the screen must be accessible through some permutation of left, right, up, down inputs - that might be possible, but I'm not even sure if the existing system can guarantee that. I do think that the existing algorithm + this patch is not exactly what would be designed from first principles for an intuitive default neighbor selection algorithm - that would use some weighted combination of closeness and alignment with direction. But I wanted to make a change that does not affect the existing behavior except when tie-breaking. |
tetrapod00
force-pushed
the
zero-separation-focus-neighbor
branch
from
ade7c1e to
8985a77
Compare
tetrapod00
force-pushed
the
zero-separation-focus-neighbor
branch
from
8985a77 to
c5ef2e2
Compare
|
Thanks! |
| if (d < r_closest_dist) { | ||
| r_closest_dist = d; | ||
| *r_closest = c; | ||
| } else if (should_tiebreak && d == r_closest_dist) { | ||
| // Tie-break in favor of the control most aligned with p_dir. | ||
| if (p_dir.dot((center - p_center).normalized()) > p_dir.dot((closest_center - p_center).normalized())) { |
There was a problem hiding this comment.
Now we are using the offset angle, this is why the corner buttons cannot be selected.
Wouldn't it be better to compare the absolute value of the cross product here?
This makes it possible to select the corner buttons.
There was a problem hiding this comment.
That might be better, indeed. When I wrote this PR I only considered distance and dot product as options for tiebreaking.
It would probably be good to write some unit tests for focus neighbor behavior, so we can make changes like this safely.
Fixes #42965.
In these videos, I am inputting "ui_right" by pressing the right arrow key.
Current behavior:
Godot_v4.3-rc1_mono_win64_4MnReWYwAc.mp4
This fix:
godot.windows.editor.x86_64_AlkCfDeYt8.mp4
The current algorithm for finding focus neighbors seems to check each side of a control against each side of a potential closest neighbor, and choose the control which is closest. In cases where there is non-zero separation between controls, this works just fine. But in cases where there is zero separation between controls, and the controls are arranged in a grid, multiple potential controls are equally close (e.g, up-left, up. and left will all have a distance of 0). This introduces a bias towards the first found neighbor control (empirically, controls that are to the left and/or up from the current control).
This fix adds a tie-breaking step in the case where multiple controls are equally close. It sorts by the minimum distance between the centers of the current control and prospective closest neighbors, which works nicely for grid-aligned controls.
Since it only runs in the tied case, this shouldn't affect existing behavior in cases where there is any separation between controls. The default theme adds separation, and anyone using zero-separation grid containers is very unlikely to be dependent on incorrect existing behavior.
My C++ will need extensive changes for style and efficiency - it's been a while since I wrote any!
Edit: Okay, that's my best attempt at C++ style. I'm aware that my code is different in style than the surrounding function, but I wanted to leave in verbose names and comments in case the algorithm itself needs to be changed. I also don't know what I don't know in terms of helper functions to use.