Anchors now adjust to size & scale changes - #1317
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesControl layout preset alignment
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
align-scaled-issue.zip |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scene/gui/control.cpp (1)
1565-1567: 📐 Maintainability & Code Quality | 🔵 TrivialRepeated
current_preset <= (int)PRESET_CENTERmagic-number check across 3 functions.The corner/center-vs-wide split relies on enum ordering (
<= PRESET_CENTER) duplicated inset_size(),set_scale(), andset_pivot_offset(). Given the case-label mix-ups already found in_compute_preset_offsets(), a small shared helper (e.g._is_wide_preset(preset)) would reduce the risk of this enum-ordering assumption silently breaking again if presets are reordered.Also applies to: 1646-1649, 1701-1704
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.cpp` around lines 1565 - 1567, Replace the duplicated current_preset <= (int)PRESET_CENTER checks in set_size(), set_scale(), and set_pivot_offset() with a shared helper such as _is_wide_preset(). Implement the helper using the existing preset classification rule and use it consistently in all three functions, preserving their current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scene/gui/control.cpp`:
- Around line 1696-1706: The anchor-preset reapplication in set_pivot_offset()
must preserve existing margins and control size, avoiding the margin loss and
wide-preset MINSIZE reset also present in set_scale(). Adjust this path to use
the same corrected preset-handling behavior as set_scale(), while retaining
support for LAYOUT_MODE_ANCHORS and LAYOUT_MODE_UNCONTROLLED.
- Around line 1560-1571: Update the preset reapplication logic in set_size() to
retain and pass the margin previously configured by set_offsets_preset() when
calling _compute_preset_offsets(), rather than using the default margin. Also
apply this automatic preset repositioning for both LAYOUT_MODE_ANCHORS and
LAYOUT_MODE_UNCONTROLLED, while preserving the existing preset filtering and
resize notification behavior.
- Around line 1641-1651: Update the anchor-preset handling in set_scale() to
match set_size(): preserve existing margins, include LAYOUT_MODE_UNCONTROLLED
where required, and avoid reapplying wide/full presets that would resize custom
dimensions via PRESET_MODE_MINSIZE. Reapply only the safe presets and modes
needed to maintain visual alignment without changing the control’s user-defined
size.
---
Nitpick comments:
In `@scene/gui/control.cpp`:
- Around line 1565-1567: Replace the duplicated current_preset <=
(int)PRESET_CENTER checks in set_size(), set_scale(), and set_pivot_offset()
with a shared helper such as _is_wide_preset(). Implement the helper using the
existing preset classification rule and use it consistently in all three
functions, preserving their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f185050f-66d9-4a99-afef-83f5b320c87e
📒 Files selected for processing (2)
scene/gui/control.cppscene/gui/control.h
fc91851 to
a1c1d0f
Compare
a1c1d0f to
004fc5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scene/gui/control.cpp (1)
1645-1654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate anchor-preset-reapply block in
set_scale()andset_pivot_offset().Both blocks are identical: guard on
is_inside_tree() && data.anchor_preset_active, resolvecurrent_preset, and call_compute_preset_offsets(..., get_size(), data.anchor_preset_margin)+_size_changed(). Past review rounds already had to fix the same bug (margin loss, MINSIZE clobbering) independently in both spots — extracting a shared helper would prevent this class of divergence from recurring.♻️ Suggested helper
void Control::_reapply_anchor_preset_offsets() { if (!is_inside_tree() || !data.anchor_preset_active) { return; } int current_preset = _get_anchors_layout_preset(); if (current_preset == -1) { return; } _compute_preset_offsets((LayoutPreset)current_preset, get_size(), data.anchor_preset_margin); _size_changed(); }Then both
set_scale()andset_pivot_offset()reduce to a single call to_reapply_anchor_preset_offsets();.Also applies to: 1699-1708
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.cpp` around lines 1645 - 1654, Extract the duplicated anchor-preset reapplication logic from set_scale() and set_pivot_offset() into a shared Control helper, such as _reapply_anchor_preset_offsets(). Have it perform the existing tree/active guard, preset lookup, offset computation, and _size_changed() call, then replace both inline blocks with a single helper call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scene/gui/control.cpp`:
- Around line 1254-1260: Fix the RTL width calculation in
Control::_compute_preset_offsets so x remains the parent rectangle’s actual
width in both layout directions; do not mirror or reassign it using p_size.x.
Preserve the existing preset formulas, including the stretching-axis cases such
as PRESET_TOP_WIDE, PRESET_BOTTOM_WIDE, PRESET_HCENTER_WIDE, and
PRESET_FULL_RECT, so they divide and offset against parent_rect.size.x rather
than -p_size.x.
- Around line 1233-1240: Update Control::set_offsets_preset() so applying a
preset does not leave data.anchor_preset_active enabled for subsequent geometry
changes. Preserve the immediate preset application while ensuring later
set_size(), set_scale(), set_pivot_offset(), or manual set_offset() calls are
not overridden by stale preset state.
- Around line 606-609: Update the property-usage logic around the use_anchors
check in the Control property list so position is marked
PROPERTY_USAGE_READ_ONLY for parentless Controls whenever anchor-based layout is
active, including the no-parent editor branch. Keep the existing anchored
behavior for parented Controls and ensure the anchors_preset/anchor_* properties
remain exposed consistently.
---
Nitpick comments:
In `@scene/gui/control.cpp`:
- Around line 1645-1654: Extract the duplicated anchor-preset reapplication
logic from set_scale() and set_pivot_offset() into a shared Control helper, such
as _reapply_anchor_preset_offsets(). Have it perform the existing tree/active
guard, preset lookup, offset computation, and _size_changed() call, then replace
both inline blocks with a single helper call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13d16e2e-b2ab-494c-87ff-57168b1a6705
📒 Files selected for processing (2)
scene/gui/control.cppscene/gui/control.h
🚧 Files skipped from review as they are similar to previous changes (1)
- scene/gui/control.h
|
Hmm, mulling this, auto-updating the "anchored" control makes sense, but it does raise the thought of the API itself, and if this affects calling a change of position. Could probably add the is_in_editor() hint, but wouldn't want to confuse the user. |
|
I am adding the "breaks compatibility" label to this one. This has been a long standing problem / behavior in Godot for ages. My thought process is, that anchoring is a pretty core feature in UI design, and countless projects are using this feature in Godot and Redot. The existing behavior is obviously incorrect and should be fixed, but this is something I feel a lot of users will have ran into and adjusted their code and UIs to workaround. We'll need to make sure that once this is merged, that we make sure that this is clearly notated in the release notes, and we will need to make sure that we make mention of this in the eventual migration guide between 26.2 and 26.3. |
I agree with you, this does indeed break compat with Godot & older versions of Redot but since we're becoming a hard fork anyways, mentioning this in the release notes may work but people often don't read that sorta stuff. Maybe we could add a warning for projects migrating from older versions of Redot to 26.3 to let them know that this version breaks compatibility? |
VictorSohier
left a comment
VictorSohier
left a comment
There was a problem hiding this comment.
The requested changes aren't mandatory, but they'd make this file a lot easier to read and might also be a slight performance gain. Otherwise, the changes look inoffensive.
| @@ -1279,26 +1308,37 @@ void Control::set_offsets_preset(LayoutPreset p_preset, LayoutPresetMode p_resiz | |||
| case PRESET_TOP_LEFT: | |||
| case PRESET_TOP_RIGHT: | |||
| case PRESET_CENTER_TOP: | |||
| case PRESET_LEFT_WIDE: | |||
| case PRESET_RIGHT_WIDE: | |||
| case PRESET_TOP_WIDE: | |||
| case PRESET_VCENTER_WIDE: | |||
| case PRESET_FULL_RECT: | |||
| data.offset[1] = parent_rect.size.y * (0.0 - data.anchor[1]) + p_margin + parent_rect.position.y; | |||
| // Fixed-size axis: visual top edge at p_margin. | |||
| data.offset[1] = parent_rect.size.y * (0.0 - data.anchor[1]) + pivot_corr_y + p_preset_margin + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_BOTTOM_WIDE: | |||
| // Fixed-size axis: visual top edge at parent_bottom - scaled_height - margin. | |||
| data.offset[1] = parent_rect.size.y * (1.0 - data.anchor[1]) + pivot_corr_y - scaled_height - p_preset_margin + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_CENTER_LEFT: | |||
| case PRESET_CENTER_RIGHT: | |||
| case PRESET_CENTER: | |||
| case PRESET_HCENTER_WIDE: | |||
| data.offset[1] = parent_rect.size.y * (0.5 - data.anchor[1]) - new_size.y / 2 + parent_rect.position.y; | |||
| // Fixed-size axis: visual center at parent vertical center. | |||
| data.offset[1] = parent_rect.size.y * (0.5 - data.anchor[1]) + pivot_corr_y - scaled_height / 2.0f + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_BOTTOM_LEFT: | |||
| case PRESET_BOTTOM_RIGHT: | |||
| case PRESET_CENTER_BOTTOM: | |||
| case PRESET_BOTTOM_WIDE: | |||
| data.offset[1] = parent_rect.size.y * (1.0 - data.anchor[1]) - new_size.y - p_margin + parent_rect.position.y; | |||
| // Fixed-size axis: visual bottom edge at parent_bottom - margin. | |||
| data.offset[1] = parent_rect.size.y * (1.0 - data.anchor[1]) + pivot_corr_y - scaled_height - p_preset_margin + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_LEFT_WIDE: | |||
| case PRESET_RIGHT_WIDE: | |||
| case PRESET_VCENTER_WIDE: | |||
| case PRESET_FULL_RECT: | |||
| // Stretching axis: visual top edge at margin, accounting for pivot. | |||
| data.offset[1] = parent_rect.size.y * (0.0 - data.anchor[1]) + pivot_corr_y + p_preset_margin + parent_rect.position.y; | |||
| break; | |||
| } | |||
There was a problem hiding this comment.
It'd be preferable to turn this into either a table, or a table that indexes to another table while you're at it
| @@ -1336,29 +1384,35 @@ void Control::set_offsets_preset(LayoutPreset p_preset, LayoutPresetMode p_resiz | |||
| case PRESET_TOP_RIGHT: | |||
| case PRESET_CENTER_TOP: | |||
| case PRESET_TOP_WIDE: | |||
| data.offset[3] = parent_rect.size.y * (0.0 - data.anchor[3]) + new_size.y + p_margin + parent_rect.position.y; | |||
| // Fixed-size axis: bottom = logical_top + p_size.y. | |||
| data.offset[3] = parent_rect.size.y * (0.0 - data.anchor[3]) + pivot_corr_y + p_size.y + p_preset_margin + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_CENTER_LEFT: | |||
| case PRESET_CENTER_RIGHT: | |||
| case PRESET_CENTER: | |||
| case PRESET_HCENTER_WIDE: | |||
| data.offset[3] = parent_rect.size.y * (0.5 - data.anchor[3]) + new_size.y / 2 + parent_rect.position.y; | |||
| // Fixed-size axis: centered. | |||
| data.offset[3] = parent_rect.size.y * (0.5 - data.anchor[3]) + pivot_corr_y + p_size.y - scaled_height / 2.0f + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_BOTTOM_LEFT: | |||
| case PRESET_BOTTOM_RIGHT: | |||
| case PRESET_CENTER_BOTTOM: | |||
| case PRESET_BOTTOM_WIDE: | |||
| // Fixed-size axis: bottom edge at parent_bottom - margin. | |||
| data.offset[3] = parent_rect.size.y * (1.0 - data.anchor[3]) + pivot_corr_y + p_size.y * (1.0f - data.scale.y) - p_preset_margin + parent_rect.position.y; | |||
| break; | |||
|
|
|||
| case PRESET_LEFT_WIDE: | |||
| case PRESET_RIGHT_WIDE: | |||
| case PRESET_BOTTOM_WIDE: | |||
| case PRESET_VCENTER_WIDE: | |||
| case PRESET_FULL_RECT: | |||
| data.offset[3] = parent_rect.size.y * (1.0 - data.anchor[3]) - p_margin + parent_rect.position.y; | |||
| // Stretching axis: visual bottom edge at parent_bottom - margin. | |||
| // Divide by scale so visual_height = parent_height - 2*margin regardless of scale. | |||
| data.offset[3] = pivot_corr_y + p_preset_margin + (parent_rect.size.y - 2.0f * p_preset_margin) / data.scale.y - parent_rect.size.y + parent_rect.position.y; | |||
| break; | |||
| } | |||
d6767c5 to
a3b09f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
scene/gui/control.h (3)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude set doesn't match usage.
<map>isn't used by any of the added code, while<array>and<tuple>(needed forstd::array/std::tupleat Lines 405-408) are only reaching this TU transitively.♻️ Proposed fix
+#include <array> `#include` <functional> -#include <map> +#include <tuple>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 46 - 47, Update the include list in control.h: remove the unused <map> include and add direct includes for <array> and <tuple>, which are required by the std::array and std::tuple usages in the added code.
316-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
current_offsetis never read.None of the 12 calculators reference
p.current_offset, yet_compute_preset_offsets()populates it viastd::copyon every call (scene/gui/control.cppLine 1174). Drop the field and the copy unless a future calculator needs it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 316 - 333, Remove the unused current_offset field from OffsetCalcParams and delete the corresponding std::copy population in _compute_preset_offsets(). Ensure the remaining offset calculators and parameter initialization continue using only the fields they reference.
402-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse plain function pointers and make the table
constexpr.Every entry is a stateless free function, so
std::functionbuys nothing here and costs a type-erased indirect call plus a non-trivial global constructor. It also leavesanchor_preset_calc_tableas mutable global state (it'sinline staticand non-const), writable from anywhere. A raw function pointer alias makes the whole tableconstexpr, immutable, and constant-initialized.♻️ Proposed refactor
- using OffsetCalculator = std::function<real_t(const OffsetCalcParams &)>; + using OffsetCalculator = real_t (*)(const OffsetCalcParams &); // Each OffsetCalculator holds a function to calculate offset: LEFT, TOP, RIGHT, BOTTOM - inline static std::array<std::tuple<OffsetCalculator, OffsetCalculator, OffsetCalculator, OffsetCalculator>, + static constexpr std::array<std::tuple<OffsetCalculator, OffsetCalculator, OffsetCalculator, OffsetCalculator>, 16 // MUST be kept the size of the LayoutPreset enum > anchor_preset_calc_table = { {With this change
<functional>is no longer needed either.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 402 - 425, Replace the std::function-based OffsetCalculator alias with a plain function-pointer type matching the existing stateless offset helpers, then declare anchor_preset_calc_table as constexpr so it is immutable and constant-initialized. Preserve all table entries and ordering, and remove the now-unused functional header.scene/gui/control.cpp (1)
1064-1093: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a
constexprtable over the lambda-initialized static.The lookup contents are compile-time constants; the immediately-invoked lambda adds a guarded first-call initialization and returns a pointer to a function-local array for no benefit. A
static constexpr int PRESET_LOOKUP[256]with designated or explicitly-indexed initialization (or a smallswitchonkey) is simpler and constant-folded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.cpp` around lines 1064 - 1093, Replace the lambda-initialized static PRESET_LOOKUP in the preset lookup logic with a directly declared static constexpr table or an equivalent small switch on key. Preserve every existing valid LayoutPreset mapping and return -1 for all invalid or custom keys without runtime initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scene/gui/control.cpp`:
- Around line 1333-1348: Align the comment in the resize logic with the behavior
of the preset branch around _get_anchors_layout_preset and
_compute_preset_offsets: either restrict preset_applied to exclude wide/full
presets as described, or remove the inaccurate statement that they are skipped.
Ensure the final comment accurately reflects how PRESET_FULL_RECT and other wide
presets are handled.
- Around line 1044-1052: Change _get_anchors_layout_preset() to return a
distinct “no preset” sentinel for non-anchor layout modes instead of
PRESET_TOP_LEFT, preserving genuine top-left preset detection. Update all
consumers—_get_layout_mode(), _validate_property(), and the reapply paths in
set_size(), set_scale(), and set_pivot_offset()—to recognize the new sentinel
and use the pos_cache/normal fallback without applying a preset.
- Around line 1264-1265: Update Control::set_position() so the ERR_FAIL_COND_MSG
check for LayoutMode::LAYOUT_MODE_ANCHORS is enforced only when the editor
anchoring mode is active, rather than for runtime scripting calls. Ensure
runtime assignments still update the position and remain observable through
get_position(), while preserving the existing editor warning/blocking behavior.
- Around line 1054-1062: Update the local encode lambda used to build key so it
returns preset bucket values only when the anchor is approximately equal to 0.0,
0.5, or 1.0; return the existing custom-anchor sentinel for every other value,
including NaN. Preserve the bit-packing order in key and use the project’s
established approximate-equality helper.
In `@scene/gui/control.h`:
- Around line 378-380: Update _calculate_right_offset_wide and the corresponding
wide-preset calculation around the referenced second location to guard against
near-zero scale_x before division. Clamp the divisor magnitude or use the prior
scale-independent formula when its absolute value is below a sane threshold,
while preserving valid-scale behavior and avoiding unintended negative-scale
flipping.
---
Nitpick comments:
In `@scene/gui/control.cpp`:
- Around line 1064-1093: Replace the lambda-initialized static PRESET_LOOKUP in
the preset lookup logic with a directly declared static constexpr table or an
equivalent small switch on key. Preserve every existing valid LayoutPreset
mapping and return -1 for all invalid or custom keys without runtime
initialization.
In `@scene/gui/control.h`:
- Around line 46-47: Update the include list in control.h: remove the unused
<map> include and add direct includes for <array> and <tuple>, which are
required by the std::array and std::tuple usages in the added code.
- Around line 316-333: Remove the unused current_offset field from
OffsetCalcParams and delete the corresponding std::copy population in
_compute_preset_offsets(). Ensure the remaining offset calculators and parameter
initialization continue using only the fields they reference.
- Around line 402-425: Replace the std::function-based OffsetCalculator alias
with a plain function-pointer type matching the existing stateless offset
helpers, then declare anchor_preset_calc_table as constexpr so it is immutable
and constant-initialized. Preserve all table entries and ordering, and remove
the now-unused functional header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06d9676d-1e05-4afd-ab22-2f6900f1cdc5
📒 Files selected for processing (2)
scene/gui/control.cppscene/gui/control.h
| static real_t _calculate_right_offset_wide(const OffsetCalcParams &p) { | ||
| return p.pivot_x_correction + p.p_preset_margin + (p.parent_x_size - 2.0f * p.p_preset_margin) / p.scale_x - p.parent_x_size + p.parent_x_position; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wide presets now divide by scale, which explodes at near-zero scale.
set_scale() clamps a zero component to CMP_EPSILON rather than rejecting it, so set_scale(Vector2(0, 1)) is a legal call that makes (p.parent_x_size - 2.0f * p.p_preset_margin) / p.scale_x produce ~1e19 and poison data.offset[2]/size_cache for the rest of the frame. Pre-PR the wide offsets were scale-independent, so this is newly reachable. Negative scale flips the rect as well.
Consider clamping the divisor magnitude (or bailing out to the unscaled formula) when |scale| is below a sane threshold.
Also applies to: 397-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scene/gui/control.h` around lines 378 - 380, Update
_calculate_right_offset_wide and the corresponding wide-preset calculation
around the referenced second location to guard against near-zero scale_x before
division. Clamp the divisor magnitude or use the prior scale-independent formula
when its absolute value is below a sane threshold, while preserving valid-scale
behavior and avoiding unintended negative-scale flipping.
| auto encode = [](real_t val) { | ||
| return (val < 0.25f) ? 0 : (val < 0.75f) ? 1 | ||
| : 2; | ||
| }; |
There was a problem hiding this comment.
Somewhat of a preference thing, but also, the rest of the engine doesn't use auto. Might be a good idea to statically type things.
| inline static std::array<std::tuple<OffsetCalculator, OffsetCalculator, OffsetCalculator, OffsetCalculator>, | ||
| 16 // MUST be kept the size of the LayoutPreset enum | ||
| > | ||
| anchor_preset_calc_table = { { | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_top_justified, _calculate_right_offset_left_justified, _calculate_bottom_offset_top_justified }, // PRESET_TOP_LEFT | ||
| { _calculate_left_offset_right_justified, _calculate_top_offset_top_justified, _calculate_right_offset_right_justified, _calculate_bottom_offset_top_justified }, // PRESET_TOP_RIGHT | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_bottom_justified, _calculate_right_offset_left_justified, _calculate_bottom_offset_bottom_justified }, // PRESET_BOTTOM_LEFT | ||
| { _calculate_left_offset_right_justified, _calculate_top_offset_bottom_justified, _calculate_right_offset_right_justified, _calculate_bottom_offset_bottom_justified }, // PRESET_BOTTOM_RIGHT | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_center_justified, _calculate_right_offset_left_justified, _calculate_bottom_offset_center_justified }, // PRESET_CENTER_LEFT | ||
| { _calculate_left_offset_center_justified, _calculate_top_offset_top_justified, _calculate_right_offset_center_justified, _calculate_bottom_offset_top_justified }, // PRESET_CENTER_TOP | ||
| { _calculate_left_offset_right_justified, _calculate_top_offset_center_justified, _calculate_right_offset_right_justified, _calculate_bottom_offset_center_justified }, // PRESET_CENTER_RIGHT | ||
| { _calculate_left_offset_center_justified, _calculate_top_offset_bottom_justified, _calculate_right_offset_center_justified, _calculate_bottom_offset_bottom_justified }, // PRESET_CENTER_BOTTOM | ||
| { _calculate_left_offset_center_justified, _calculate_top_offset_center_justified, _calculate_right_offset_center_justified, _calculate_bottom_offset_center_justified }, // PRESET_CENTER | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_top_justified, _calculate_right_offset_left_justified, _calculate_bottom_offset_wide }, // PRESET_LEFT_WIDE | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_top_justified, _calculate_right_offset_wide, _calculate_bottom_offset_top_justified }, // PRESET_TOP_WIDE | ||
| { _calculate_left_offset_right_justified, _calculate_top_offset_top_justified, _calculate_right_offset_right_justified, _calculate_bottom_offset_wide }, // PRESET_RIGHT_WIDE | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_bottom_justified, _calculate_right_offset_wide, _calculate_bottom_offset_bottom_justified }, // PRESET_BOTTOM_WIDE | ||
| { _calculate_left_offset_center_justified, _calculate_top_offset_top_justified, _calculate_right_offset_center_justified, _calculate_bottom_offset_wide }, // PRESET_VCENTER_WIDE | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_center_justified, _calculate_right_offset_wide, _calculate_bottom_offset_center_justified }, // PRESET_HCENTER_WIDE | ||
| { _calculate_left_offset_left_justified, _calculate_top_offset_top_justified, _calculate_right_offset_wide, _calculate_bottom_offset_wide } // PRESET_FULL_RECT | ||
| } }; |
There was a problem hiding this comment.
I'll just point out that if all you are doing is dispatching to functions, you lose if they would've been inlined. It might be better to just compute all the possible results in a table and copy from there. CPUs are great at computing quite a few independent results simultaneously. There are only 14 entries here, if you wanted to ensure that you only do those 14 computations, have another table of indices that index into the computed table then.
There was a problem hiding this comment.
So, I believe we do need the calculation to stay. The engine supports custom anchor presets, so the values, themselves, can theoretically be anything. Additionally, I think the functions, while a little nauseating, kind of self document how the work is being done.
But, I'll see if there's a way to get rid of the function calling overhead.
There was a problem hiding this comment.
You can perform the calculation in the array declaration in the function. Pipelining should be able to take care of it if it's just 14 calculations with 8 ops. This is one thing compilers are good at managing.
a3b09f9 to
e302410
Compare
|
Think this is cleaned up pretty well. EDIT...besides the failed unit tests xD...tomorrow... Posterity note: I came across what I think is a separate bug -- When changing layout direction to right-to-left, it horizontally mirrors the control node, regardless of the anchor preset. |
e302410 to
3699a54
Compare
|
align-scaled-issue-2.zip Additionally, it manipulates one of the anchor offsets in code, which squishes the icon. In line with the warning text, overriding the offset values, themselves, is what makes sense when an anchor preset is used, not the position--because the anchor offset manipulates the position. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
scene/gui/control.h (4)
405-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndefine
LAYOUT_PRESET_LISTafter use.
Xis undefined at line 436, butLAYOUT_PRESET_LISTstays defined.control.his included by a large part of the engine, so this macro leaks into every consuming translation unit and can collide with other definitions. Add#undef LAYOUT_PRESET_LISTafter the switch.Also consider a
default:label in the switch._set_anchors_layout_preset()casts anintintoLayoutPreset, so an out-of-range value currently leavesdata.offsetuntouched with no diagnostic.♻️ Proposed cleanup
LAYOUT_PRESET_LIST `#undef` X + default: + ERR_FAIL_MSG("Unknown layout preset."); } } +#undef LAYOUT_PRESET_LISTAlso applies to: 435-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 405 - 421, Undefine LAYOUT_PRESET_LIST immediately after its final expansion in the switch so the helper macro does not leak from control.h; also add a default label to _set_anchors_layout_preset() to handle out-of-range LayoutPreset values with an explicit diagnostic or safe fallback, while preserving existing behavior for valid presets.
326-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
p_-prefixed struct members.In this codebase
p_marks function parameters.p_size_x,p_size_y, andp_preset_marginare struct members, so the prefix is misleading. Usesize_x,size_y, andpreset_margin, and update the designated initializers in_compute_preset_offsets()and the calculator functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 326 - 330, Rename the struct members p_size_x, p_size_y, and p_preset_margin to size_x, size_y, and preset_margin, respectively, and update all designated initializers and member references in _compute_preset_offsets() and the related calculator functions to use the new names.
318-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
current_offsetfromOffsetCalcParams.No calculator function reads
current_offset._compute_preset_offsets()inscene/gui/control.cppstill copiesdata.offsetinto it on every call, which is dead work in a layout path that runs on each size, scale, and pivot change. Remove the field and the copy, or add the calculator that consumes it.
anchor_positionis used (indices 0-3), so keep that field.♻️ Proposed cleanup
struct OffsetCalcParams { - real_t current_offset[4] = { 0.0f, 0.0f, 0.0f, 0.0f }; ///< The value the offset is to start with real_t anchor_position[4] = { 0.0f, 0.0f, 0.0f, 0.0f }; ///< 0.0 - 1.0Then remove the matching copy in
scene/gui/control.cpp:- std::copy(std::begin(data.offset), std::end(data.offset), std::begin(params.current_offset)); std::copy(std::begin(data.anchor), std::end(data.anchor), std::begin(params.anchor_position));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 318 - 319, Remove the unused current_offset field from OffsetCalcParams while preserving anchor_position. Update _compute_preset_offsets() to stop copying data.offset into current_offset, unless a calculator is added that actually consumes it.
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
<functional>and<map>includes.
control.hdoes not usestd::function,std::map, orstd::multimap; the copy operations incontrol.cppneed<algorithm>, not these headers. Remove them to avoid adding unnecessary compile dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/gui/control.h` around lines 46 - 47, Remove the unused <functional> and <map> includes from control.h, leaving the required header for copy operations in control.cpp unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scene/gui/control.h`:
- Around line 405-421: Undefine LAYOUT_PRESET_LIST immediately after its final
expansion in the switch so the helper macro does not leak from control.h; also
add a default label to _set_anchors_layout_preset() to handle out-of-range
LayoutPreset values with an explicit diagnostic or safe fallback, while
preserving existing behavior for valid presets.
- Around line 326-330: Rename the struct members p_size_x, p_size_y, and
p_preset_margin to size_x, size_y, and preset_margin, respectively, and update
all designated initializers and member references in _compute_preset_offsets()
and the related calculator functions to use the new names.
- Around line 318-319: Remove the unused current_offset field from
OffsetCalcParams while preserving anchor_position. Update
_compute_preset_offsets() to stop copying data.offset into current_offset,
unless a calculator is added that actually consumes it.
- Around line 46-47: Remove the unused <functional> and <map> includes from
control.h, leaving the required header for copy operations in control.cpp
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96f1f933-04f5-4e14-b3d4-cb65e384b2da
📒 Files selected for processing (2)
scene/gui/control.cppscene/gui/control.h
🚧 Files skipped from review as they are similar to previous changes (1)
- scene/gui/control.cpp
3699a54 to
396264d
Compare
396264d to
0d06e20
Compare
4 participants
Fixes godotengine/godot#19068
The problem described was really 1 piece of additional issues. The anchor presets simply did not take scale or size into account. Additionally (a natural consequence), adjusting those properties did not update per the anchor preset. This PR addresses all of that:
I didn't try to do this with rotation because the expected behavior is ambiguous, plus, ugh...
Summary by CodeRabbit
Bug Fixes
Documentation