Add new default algorithm of automatic focus strategy for Control nodes#120631
Add new default algorithm of automatic focus strategy for Control nodes#120631rsubtil wants to merge 1 commit into
Control nodes#120631Conversation
| if (Math::is_zero_approx(dir.x)) { | ||
| dir.x = 1e-10; | ||
| } | ||
| if (Math::is_zero_approx(dir.y)) { | ||
| dir.y = 1e-10; | ||
| } |
There was a problem hiding this comment.
If we can rely on IEEE 754 behavior, we can avoid this check and allow division by zero. This results in INF / -INF values which are correctly handled by this algorithm. See https://tavianator.com/fast-branchless-raybounding-box-intersections/
Not sure if we can do that though, as e.g. I've found explicit warning silence #pragma for compiler warnings in regards to division by zero, so need to confirm.
| return touch; | ||
| } | ||
|
|
||
| real_t Control::_focus_strategy_balloon(const Vector2 &p_dir, const Control &p_candidate, const Rect2 &p_rect, const Rect2 &p_clamp, real_t p_min) { |
There was a problem hiding this comment.
p_rect and p_min are currently not used by the algorithm at all. Need to understand how behavior changes with ScrollContainer nodes (in practice, waiting for clarification on #120631 (comment))
| } | ||
|
|
||
| real_t Control::_focus_strategy_balloon(const Vector2 &p_dir, const Control &p_candidate, const Rect2 &p_rect, const Rect2 &p_clamp, real_t p_min) { | ||
| // Algorithm proposed and designed by Rune Skovbo Johansen & Adriaan de Jongh |
There was a problem hiding this comment.
@runevision @AdriaandeJongh I advocate for proper attribution when possible, and I want to make it clear the origins of this algorithm. Let me know what do you think of this attribution and any changes if you want 🙂
There was a problem hiding this comment.
Sure, I appreciate the shoutout! But... not sure how common this is in the Godot codebase 😅 @AThousandShips wdyt?
| // TODO: Should be impossible to not have an intersection for rays starting within the Rect2 (even for dir == Vector2.ZER0, as the algorithm applies an epsilon) | ||
| // Do we assert? [[ unlikely ]]? ERR_FAIL_COND? | ||
| return NO_SCORE; |
There was a problem hiding this comment.
Need clarification on how Godot handles theoretically-impossible scenarios.
| #define MAX_NEIGHBOR_SEARCH_COUNT 512 | ||
| #define NO_SCORE 1e14 | ||
|
|
||
| Control *Control::_get_focus_neighbor(Side p_side, int p_count) { |
There was a problem hiding this comment.
This function still has some logic which I believe is tied to the now legacy algorithm, especially when concerning nodes within a ScrollContainer. I haven't yet confirmed how the new algorithm behaves in these cases, and still need to research what issues prompted special handling for ScrollContainer to fully understand.
cc @Rindbee
There was a problem hiding this comment.
This refers to a situation where focus moves from a sibling control of the ScrollContainer to a child control within the ScrollContainer (or vice versa). This can be viewed as two layers. This is cross-layer movement. Control overlap may occur in this case.
8ae99d8 to
6197898
Compare
| GLOBAL_DEF_BASIC("gui/common/snap_controls_to_pixels", true); | ||
| GLOBAL_DEF(PropertyInfo(Variant::INT, "gui/common/show_focus_state_on_pointer_event", PROPERTY_HINT_ENUM, "Never,Text Input Controls,Always"), 1); | ||
| GLOBAL_DEF_BASIC("gui/fonts/dynamic_fonts/use_oversampling", true); | ||
| GLOBAL_DEF(PropertyInfo(Variant::INT, "gui/common/auto_focus_strategy", PROPERTY_HINT_ENUM, "Legacy,Balloon"), 0); |
There was a problem hiding this comment.
I'd make the new algorithm the default.
There was a problem hiding this comment.
In the current state, any newly created project will use the new algorithm by default, while current projects maintain the legacy one. This follows the same "maintaing old behavior" mechanism as some other settings (e.g. Jolt Physics on 3D, D3D12 renderer on Windows).
So far this algorithm behaves better in all scenarios I've tested (still need to check ScrollContainer's though), and I agree in making this the default option going forward, but since there is concern about behavior changes in the issue/proposal discussion, I've left the legacy behavior for current projects.
Calinou
left a comment
There was a problem hiding this comment.
Tested locally on all GUI demos, it works as expected. Code looks good to me.
I couldn't spot any regressions in previous/next focus behavior.
Note that this PR does not affect left/top/right/bottom neighbor determination when navigating with the arrow keys or d-pad. It's still broken out of the box on the Drag and Drop demo:
focus_neighbor_arrow_key_navigation.mp4
This can be addressed by assigning explicit neighbors to all buttons on the edges (only the one at the center doesn't need explicit neighbors).
| Override for [member filesystem/import/fbx2gltf/enabled] on the Web where FBX2glTF can't easily be accessed from Godot. | ||
| </member> | ||
| <member name="gui/common/auto_focus_strategy" type="int" setter="" getter="" default="0"> | ||
| Determines what strategy to use when inferring the next [Control] to focus if no neighbor is defined. |
There was a problem hiding this comment.
| Determines what strategy to use when inferring the next [Control] to focus if no neighbor is defined. | |
| Determines what strategy to use when inferring the previous/next [Control] to focus if no neighbor is defined. This does not affect the determination of top/left/right/bottom neighbors when using the arrow keys or gamepad to navigate menus. |
@Calinou that's strange, it should be the exact opposite. This PR handles left/top/right/bottom neighbor determination. It only didn't touch next/previous determination. I'm testing the drag&drop demo, and you're right, something strange is happening, because when a candidate fails to be found, it seems to fallback to some other logic which selects a different node. Switching to the GDScript version of the algorithm works as expected, because it completely bypasses Godot's auto focus mechanism. There's probably some behavior running elsewhere which I didn't account for, will need to find it. recording_2026-06-30_15.17.06.mp4 |
6197898 to
25954dd
Compare
|
Found the issue; I forgot to discard any result returning the I also changed the |
What problem(s) does this PR solve?
Controlnodes godot-proposals#14659Additional information
Note
This is ready to review, but still in Draft as there's some doubts I need to check first. I've left comments in the PR to clarify.
This PR, at it's core, introduces a new algorithm for inferring focus Controls without neighbors, dubbed the "balloon algorithm", which was discussed and iterated under #103895. To go into a bit more detail, this PR:
_focus_strategy_legacy)_focus_strategy_balloon)gui/common/auto_focus_strategy) and this new algorithm as the default solution for newer projects.Tip
You can test this PR in this web demo: https://rsubtil.itch.io/godot-focus-tests-demo
This demo will be updated for any relevant changes with this MR. It allows to compare the old and new behavior as implemented in C++, as well as GDScript versions of this algorithm which allows for debug visualization to better understand it's functionality.
cc @AdriaandeJongh @runevision @Rindbee