Control Node Transform#1328
Open
GeneralProtectionFault wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughControl gains pivot-offset ratios and a configurable offset-transform API. Layout and anchor preset calculations use the combined pivot and transform state, while the editor tracks pivot changes and renders an offset-transform preview. ChangesControl transform API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant Control
participant ControlOffsetTransformPreview
participant CanvasOverlay
Editor->>Control: select and edit Control
Control->>ControlOffsetTransformPreview: expose transform state
ControlOffsetTransformPreview->>CanvasOverlay: render transformed outline
Control->>Editor: report pivot-ratio changes
Editor->>CanvasOverlay: queue viewport redraw
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
scene/gui/control.cpp (3)
726-750: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the accessor over the raw member for
visual_only.
is_offset_transform_enabled()guarantees non-null so the dereference is safe, but usingis_offset_transform_visual_only()keeps these two call sites consistent with the rest of the API and immune to future reordering of the&&operands.♻️ Suggested tweak
- if (is_offset_transform_enabled() && !data.offset_transform->visual_only) { + if (is_offset_transform_enabled() && !is_offset_transform_visual_only()) {🤖 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 726 - 750, Replace the direct data.offset_transform->visual_only checks in Control::_get_internal_transform() and Control::_update_canvas_item_transform() with the is_offset_transform_visual_only() accessor, preserving the existing enabled-state conditions and transform behavior.
4389-4405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
get_offset_transform()is public but not bound.Every other offset-transform accessor is exposed to scripting here;
get_offset_transform()is not, and it is also absent fromdoc/classes/Control.xml. If it is meant to be script-visible (the editor preview relies on it), bind and document it; otherwise consider making it non-public.🤖 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 4389 - 4405, Expose Control::get_offset_transform() consistently with the other offset-transform accessors by adding its ClassDB binding alongside the existing bindings and documenting it in Control.xml. Keep the method public so editor preview and scripting can use the same accessor.
1281-1318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate preset branches can be merged.
PRESET_TOP_WIDE/BOTTOM_WIDE/HCENTER_WIDE/FULL_RECT(Line 1316) compute exactly the same expression as the left-edge fixed group (Line 1288); the same duplication exists in the "Top" switch (Lines 1327 and 1355). Folding the identical cases together would shrink these switches noticeably.Also applies to: 1320-1357, 1359-1383, 1395-1419
🤖 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 1281 - 1318, Merge the duplicate preset case branches in the preset-positioning switches, including the horizontal “Left” group shown and the corresponding “Top” and other axis groups referenced by the comment. Fold presets that compute identical expressions into the existing shared case block, preserving each expression and all preset behavior while removing redundant branches.scene/gui/control.h (1)
362-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameter name drift between declaration and definition.
_compute_preset_offsetsis declared withp_marginhere, butscene/gui/control.cppdefines it (andset_offsets_preset) withp_preset_margin. The Doxygen block at Lines 542-545 also documents@param p_margin, which no longer matches the implementation signature. Align on one name.Also applies to: 542-546
🤖 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` at line 362, Align the parameter name for _compute_preset_offsets across its declaration, definition, and Doxygen documentation, using one consistent name instead of mixing p_margin and p_preset_margin. Update the related set_offsets_preset definition if it has the same drift, while preserving behavior.editor/scene/gui/control_editor_plugin.h (1)
256-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ControlOffsetTransformPreviewdoesn't need to be aControl(or a friend).It holds a plugin pointer plus two methods and never draws itself — the drawing goes to
p_overlay. Deriving fromControlforces it into the editor's GUI tree (see theadd_childincontrol_editor_plugin.cpp), where it participates in layout/focus for no reason. A plainObject-derived helper, or just folding the state intoControlEditorPlugin, would be simpler. Thefriend class ControlEditorPlugindeclaration is also redundant since all used members are public.🤖 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 `@editor/scene/gui/control_editor_plugin.h` around lines 256 - 270, Refactor ControlOffsetTransformPreview to be a non-Control helper, preferably deriving from Object, since it only stores plugin/selection state and draws through p_overlay. Update its construction and the add_child usage in ControlEditorPlugin so it is no longer inserted into the GUI tree, while preserving edit and forward_canvas_draw_over_viewport behavior. Remove the now-unnecessary friend class ControlEditorPlugin declaration.
🤖 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 `@doc/classes/Control.xml`:
- Around line 1123-1127: In the `offset_transform_visual_only` documentation
member, correct the grammar by changing “the offset transforms is only applied”
to “the offset transform is only applied,” without altering the surrounding
behavior description.
In `@editor/scene/gui/control_editor_plugin.cpp`:
- Around line 1163-1181: Update ControlOffsetTransformPreview::edit and
forward_canvas_draw_over_viewport to avoid dereferencing a freed
selected_control: track the selected Control with ObjectID or clear the
selection when the control exits/is deleted, then validate the object before
disconnecting, drawing, or otherwise using it. Preserve the existing overlay
connection and update behavior for live controls.
- Around line 1216-1226: Restrict ControlEditorPlugin::handles() to Controls
that support offset transform editing instead of every Control, and apply the
same capability check in edit() and related inspection paths. Preserve overlay
drawing for unsupported Controls while preventing this plugin from participating
in selection handling or main-editor lookup for them.
In `@scene/gui/control.cpp`:
- Around line 2054-2069: Clamp both components of p_scale to CMP_EPSILON in
Control::set_offset_transform_scale(), matching set_scale(). Also update
editor/scene/gui/control_editor_plugin.cpp lines 1183-1197 to skip drawing when
the offset transform determinant is zero before calling affine_inverse(); both
changes must prevent singular offset transforms from being inverted.
- Around line 1578-1594: Update the preset handling in the p_keep_offsets false
branch around _get_anchors_layout_preset and _compute_preset_offsets so
PRESET_FULL_RECT and all *_WIDE presets bypass _compute_preset_offsets and use
the existing _compute_offsets path; keep explicit corner/center presets applying
_compute_preset_offsets.
- Around line 1385-1391: Update the stretched-preset calculations in the
PRESET_TOP_WIDE/PRESET_BOTTOM_WIDE/PRESET_HCENTER_WIDE/PRESET_FULL_RECT branches
and the corresponding logic around the alternate highlighted lines to avoid
dividing by near-zero data.scale.x or data.scale.y. Clamp each divisor to a
finite, sensible minimum magnitude and preserve the intended positive stretching
direction so offsets cannot become infinite or invert from negative scale
values.
- Around line 4605-4606: Update the pivot_offset PropertyInfo registration in
the control property declarations to remove the explicit PROPERTY_USAGE_EDITOR
argument while preserving the suffix:px hint, allowing the default storage usage
to remain active. Leave pivot_offset_ratio and the existing setter/getter
bindings unchanged.
- Around line 1276-1279: Update the preset alignment calculations in the
relevant transform helper to use the combined pivot offset, including
pivot_offset_ratio × size, for both pivot corrections when scaling. Also update
set_pivot_offset_ratio() to reapply preset offsets in the same way as
set_pivot_offset(), keeping preset alignment current when either pivot component
changes.
---
Nitpick comments:
In `@editor/scene/gui/control_editor_plugin.h`:
- Around line 256-270: Refactor ControlOffsetTransformPreview to be a
non-Control helper, preferably deriving from Object, since it only stores
plugin/selection state and draws through p_overlay. Update its construction and
the add_child usage in ControlEditorPlugin so it is no longer inserted into the
GUI tree, while preserving edit and forward_canvas_draw_over_viewport behavior.
Remove the now-unnecessary friend class ControlEditorPlugin declaration.
In `@scene/gui/control.cpp`:
- Around line 726-750: Replace the direct data.offset_transform->visual_only
checks in Control::_get_internal_transform() and
Control::_update_canvas_item_transform() with the
is_offset_transform_visual_only() accessor, preserving the existing
enabled-state conditions and transform behavior.
- Around line 4389-4405: Expose Control::get_offset_transform() consistently
with the other offset-transform accessors by adding its ClassDB binding
alongside the existing bindings and documenting it in Control.xml. Keep the
method public so editor preview and scripting can use the same accessor.
- Around line 1281-1318: Merge the duplicate preset case branches in the
preset-positioning switches, including the horizontal “Left” group shown and the
corresponding “Top” and other axis groups referenced by the comment. Fold
presets that compute identical expressions into the existing shared case block,
preserving each expression and all preset behavior while removing redundant
branches.
In `@scene/gui/control.h`:
- Line 362: Align the parameter name for _compute_preset_offsets across its
declaration, definition, and Doxygen documentation, using one consistent name
instead of mixing p_margin and p_preset_margin. Update the related
set_offsets_preset definition if it has the same drift, while preserving
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 Plus
Run ID: ed1003b6-0a11-4f03-bcd3-2424e51822b5
📒 Files selected for processing (7)
doc/classes/Control.xmleditor/scene/canvas_item_editor_plugin.cppeditor/scene/canvas_item_editor_plugin.heditor/scene/gui/control_editor_plugin.cppeditor/scene/gui/control_editor_plugin.hscene/gui/control.cppscene/gui/control.h
Comment on lines
+1123
to
+1127
| <member name="offset_transform_visual_only" type="bool" setter="set_offset_transform_visual_only" getter="is_offset_transform_visual_only" default="true"> | ||
| If [code]true[/code], the offset transforms is only applied visually and does not affect input. In other words, this Control will still receive input events at its original location before the offset transform is applied. | ||
| If [code]false[/code], the entire transform of this Control is affected and input events will register where the Control is visually. | ||
| Has no effect unless [member offset_transform_enabled] is [code]true[/code]. | ||
| </member> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Grammar: "the offset transforms is only applied".
Should be "the offset transform is only applied".
🤖 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 `@doc/classes/Control.xml` around lines 1123 - 1127, In the
`offset_transform_visual_only` documentation member, correct the grammar by
changing “the offset transforms is only applied” to “the offset transform is
only applied,” without altering the surrounding behavior description.
Comment on lines
+1163
to
+1181
| void ControlOffsetTransformPreview::edit(Control *p_control) { | ||
| if (p_control == selected_control) { | ||
| return; | ||
| } | ||
|
|
||
| const Callable update_overlays = callable_mp(plugin, &EditorPlugin::update_overlays); | ||
|
|
||
| if (selected_control) { | ||
| selected_control->disconnect(SceneStringName(draw), update_overlays); | ||
| } | ||
|
|
||
| selected_control = p_control; | ||
|
|
||
| if (selected_control) { | ||
| selected_control->connect(SceneStringName(draw), update_overlays); | ||
| } | ||
|
|
||
| plugin->update_overlays(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
selected_control is a raw pointer that can dangle before the next edit() call.
If the previously edited node is freed while still selected, selected_control->disconnect(...) (Line 1171) — and forward_canvas_draw_over_viewport() — dereference freed memory. Track it via ObjectID, or connect to tree_exiting/NOTIFICATION_PREDELETE on the control to clear the pointer.
🤖 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 `@editor/scene/gui/control_editor_plugin.cpp` around lines 1163 - 1181, Update
ControlOffsetTransformPreview::edit and forward_canvas_draw_over_viewport to
avoid dereferencing a freed selected_control: track the selected Control with
ObjectID or clear the selection when the control exits/is deleted, then validate
the object before disconnecting, drawing, or otherwise using it. Preserve the
existing overlay connection and update behavior for live controls.
Comment on lines
+1216
to
+1226
| void ControlEditorPlugin::edit(Object *p_object) { | ||
| offset_transform_preview->edit(Object::cast_to<Control>(p_object)); | ||
| } | ||
|
|
||
| bool ControlEditorPlugin::handles(Object *p_object) const { | ||
| return Object::cast_to<Control>(p_object) != nullptr; | ||
| } | ||
|
|
||
| void ControlEditorPlugin::forward_canvas_draw_over_viewport(Control *p_overlay) { | ||
| offset_transform_preview->forward_canvas_draw_over_viewport(p_overlay); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other editor plugins that handle Control-derived objects.
rg -nP --type=cpp -C2 'bool \w+EditorPlugin::handles\(Object \*p_object\) const' editor/ | head -80Repository: Redot-Engine/redot-engine
Length of output: 5392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate lines =="
sed -n '1180,1245p' editor/scene/gui/control_editor_plugin.cpp
echo
echo "== relevant class declarations =="
rg -n "class ControlEditorPlugin|ControlEditorPlugin" editor/scene/gui/control_editor_plugin.cpp editor/scene/gui/control_editor_plugin.h || true
echo
echo "== ControlEditorPlugin symbols in editor files =="
rg -n "ControlEditorPlugin" editor/scene/gui editor/scene editor || true
echo
echo "== EditorPlugin dispatch snippets =="
rg -n "handles\\(Object \\*p_object\\)|make_visible\\(Object|get_selected_object\\(|selected->edit|current_selected|selected" editor editor --type=cpp | rg "EditorNode|EditorPlugin|selected|handles\\(|edit\\(" | head -160Repository: Redot-Engine/redot-engine
Length of output: 41607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only behavioral model for the editor plugin dispatch order that appears in EditorNode:
# First selected plugin handles(), then make_visible(); if handled, selected->edit() calls it last.
python3 - <<'PY'
from pathlib import Path
p = Path('editor/editor_node.cpp')
text = p.read_text(errors='ignore')
for needle in ['_plugins_count', '_plugins[', 'selected->make_visible', 'handles(', 'selected->edit', 'EditorNode::make_visible']:
print(f'--- {needle} ---')
try:
i = text.index(needle)
start=max(0,i-180); end=min(len(text),i+800)
print(text[start:end])
except ValueError:
print('not found')
PYRepository: Redot-Engine/redot-engine
Length of output: 2310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Editor plugins list locations =="
rg -n "add_editor_plugin|EditorData::add_editor_plugin|vector<EditorPlugin|List<EditorPlugin|for\\(.*EditorPlugin|EditorPlugin\\*" editor/editor* editor editor_node* --type=cpp | head -220
echo
echo "== editor_data.h candidate sections =="
fd -a editor_data.h editor | rargs -N sh -c 'nl -ba "$1" | sed -n "1,260p"' sh --
echo
echo "== EditorNode selection-related sections =="
rg -n -C8 "handles\\(|selected->|current_selected|plugin_over|_plugin_selected|_node_selected" editor/editor_node.cpp editor/editor_data.cpp editor --type=cpp | head -260Repository: Redot-Engine/redot-engine
Length of output: 4171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== EditorData selection management sections =="
nl -ba editor/editor_data.cpp | sed -n '1140,1365p'
echo
echo "== EditorPlugin handles declaration/defaults =="
rg -n "bool handles\\(Object \\*p_object\\) const|handles\\(" editor/editor_plugin.h editor/editor_plugin.cpp editor/editor_node.cpp --type=cpp | head -80
echo
echo "== canvas item plugin controls/selected handling =="
rg -n -C6 "handles\\(|add_editor_plugin\\(this\\)|selection_changed|edit\\(|make_visible\\(|selected->" editor/scene/canvas_item_editor_plugin.cpp --type=cpp | head -260Repository: Redot-Engine/redot-engine
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== EditorData selection management sections =="
sed -n '1140,1365p' editor/editor_data.cpp | awk '{printf "%6d\t%s\n", NR+1139, $0}'
echo
echo "== EditorPlugin handles declaration/defaults =="
rg -n "bool handles\\(Object \\*p_object\\) const|handles\\(|bool handles\\(" editor/editor_plugin.h editor/editor_plugin.cpp editor/editor_node.cpp --type=cpp | head -80
echo
echo "== canvas item plugin controls/selected handling =="
rg -n -C6 "handles\\(|add_editor_plugin\\(this\\)|selection_changed|edit\\(|make_visible\\(|selected->" editor/scene/canvas_item_editor_plugin.cpp --type=cpp | head -260Repository: Redot-Engine/redot-engine
Length of output: 7897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find EditorPlugin definitions =="
fd -H -a editor_editor_plugin.h editor_editor_plugin.cpp 2>/dev/null || true
rg -n "class EditorPlugin|bool handles\\(Object \\*p_object\\)|handles\\(Object \\*p_object\\) const" editor --type=cpp --glob '*editor_plugin*' | head -80
echo
echo "== files with EditorEditorPlugin =="
fd -i 'editor_plugin' editor -t f | sed -n '1,80p'
rg -n -C6 "bool handles\\(Object \\*p_object\\) const override|bool handles\\(Object \\*p_object\\) const|handles\\(Object \\*p_object\\) const" editor/editor --type=cpp --glob 'editor_plugin.cpp' | head -80
echo
echo "== canvas item editor controls and selection ownership =="
rg -n -C6 "handles\\(Object \\*p_object\\)|add_editor_plugin\\(this\\)|selection_changed|selected_nodes|edit\\(|make_visible\\(|_get_editor_data" editor/scene/canvas_item_editor_plugin.cpp --type=cpp | sed -n '5200,5380p;5800,5860p;4800,4920p'
echo
echo "== editor_data selection ownership implementation =="
rg -n -C12 "selected_nodes\\(|clear_selected|add_node\\(|selected\\[" editor/editor_data.cpp --type=cpp | sed -n '1360,1485p;1520,1640p;1660,1780p'Repository: Redot-Engine/redot-engine
Length of output: 12213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== EditorPlugin base handles implementation =="
sed -n '140,220p' editor/plugins/editor_plugin.h | sed -n '140,220p' || true
sed -n '340,360p' editor/plugins/editor_plugin.cpp | sed -n '340,360p' || true
echo
echo "== EditorData selection ownership implementation =="
sed -n '1380,1820p' editor/editor_data.cpp | sed -n '1380,1820p'
echo
echo "== CanvasItemEditorPlugin add_editor_plugin and handles context =="
sed -n '5330,5385p' editor/scene/canvas_item_editor_plugin.cpp | sed -n '5330,5385p'
sed -n '5820,5845p' editor/scene/canvas_item_editor_plugin.cpp | sed -n '5820,5845p'
echo
echo "== ControlEditorPlugin header declarations =="
sed -n '260,295p' editor/scene/gui/control_editor_plugin.h | sed -n '260,295p'Repository: Redot-Engine/redot-engine
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== raw file sizes =="
stat -c '%n %s' editor/plugins/editor_plugin.h editor/plugins/editor_plugin.cpp editor/editor_data.cpp editor/scene/canvas_item_editor_plugin.cpp editor/scene/gui/control_editor_plugin.h editor/scene/gui/control_editor_plugin.cpp
echo
echo "== EditorPlugin.cpp around handles/default =="
awk '{printf "%6d\t%s\n", NR, $0}' editor/plugins/editor_plugin.cpp | awk 'NR>=330 && NR<=360'
echo
echo "== EditorPlugin.h around declaration =="
awk '{printf "%6d\t%s\n", NR, $0}' editor/plugins/editor_plugin.h | awk 'NR>=180 && NR<=210'
echo
echo "== EditorData.add_editor_plugin implementation =="
awk '{printf "%6d\t%s\n", NR, $0}' editor/editor_data.cpp | awk 'NR>=1265 && NR>=1290 {exit} 1' | awk 'NR>=1275 && NR<=1290 {exit} 1'
awk '{printf "%6d\t%s\n", NR, $0}' editor/editor_data.cpp | awk 'NR>=1275 && NR<=1295'
echo
echo "== EditorSelection.add_node ownership loop =="
awk '{printf "%6d\t%s\n", NR, $0}' editor/editor_data.cpp | awk 'NR>=1230 && NR<=1241'Repository: Redot-Engine/redot-engine
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== base handles implementation =="
awk '{printf "%6d: %s\n", NR, $0}' editor/plugins/editor_plugin.cpp | awk 'NR>=338 && NR<=354'
awk '{printf "%6d: %s\n", NR, $0}' editor/plugins/editor_plugin.h | awk 'NR>=190 && NR<=202'
echo
echo "== selection ownership loop =="
awk '{printf "%6d: %s\n", NR, $0}' editor/editor_data.cpp | awk 'NR>=1228 && NR<=1238'
echo
echo "== editor plugin registration/list =="
awk '{printf "%6d: %s\n", NR, $0}' editor/editor_data.h | awk 'NR>=168 && NR<=182'
awk '{printf "%6d: %s\n", NR, $0}' editor/editor_data.cpp | awk 'NR>=488 && NR>=512 {exit} 1'
echo
echo "== selection add/remove ownership helpers =="
rg -n -C4 "add_editor_plugin\\(" editor/editor_data.cpp editor/editor_data.h editor/editor_node.cpp --type=cppRepository: Redot-Engine/redot-engine
Length of output: 21658
Restrict ControlEditorPlugin::handles() to controls with offset transform support.
handles() returning true for every Control adds this plugin to selection handling and main-editor lookup for Controls it only draws an overlay for. If the intent is overlay-only, keep it out of handle(); otherwise narrow handles(), edit(), and inspection to Controls that support offset transform editing.
🤖 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 `@editor/scene/gui/control_editor_plugin.cpp` around lines 1216 - 1226,
Restrict ControlEditorPlugin::handles() to Controls that support offset
transform editing instead of every Control, and apply the same capability check
in edit() and related inspection paths. Preserve overlay drawing for unsupported
Controls while preventing this plugin from participating in selection handling
or main-editor lookup for them.
Comment on lines
+1385
to
+1391
| case PRESET_TOP_WIDE: | ||
| case PRESET_BOTTOM_WIDE: | ||
| case PRESET_HCENTER_WIDE: | ||
| case PRESET_FULL_RECT: | ||
| data.offset[2] = x * (1.0 - data.anchor[2]) - p_margin + parent_rect.position.x; | ||
| // Stretching axis: visual right edge at parent_right - margin. | ||
| // Divide by scale so visual_width = parent_width - 2*margin regardless of scale. | ||
| data.offset[2] = pivot_corr_x + p_preset_margin + (x - 2.0f * p_preset_margin) / data.scale.x - x + parent_rect.position.x; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Division by data.scale can blow up offsets for near-zero scale.
set_scale() clamps zeros to CMP_EPSILON, so (x - 2*margin) / data.scale.x yields values around 1e38 and can overflow to inf, which then propagates into data.offset[] and the size cache (set_size() has an isfinite guard, but the offsets themselves are already poisoned). Negative scale also inverts the stretched edge. Consider clamping the divisor to a sane minimum magnitude (or falling back to the unscaled formula when Math::abs(scale) < some epsilon).
Also applies to: 1421-1427
🤖 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 1385 - 1391, Update the stretched-preset
calculations in the
PRESET_TOP_WIDE/PRESET_BOTTOM_WIDE/PRESET_HCENTER_WIDE/PRESET_FULL_RECT branches
and the corresponding logic around the alternate highlighted lines to avoid
dividing by near-zero data.scale.x or data.scale.y. Clamp each divisor to a
finite, sensible minimum magnitude and preserve the intended positive stretching
direction so offsets cannot become infinite or invert from negative scale
values.
Comment on lines
+1578
to
1594
| // For corner/center anchor presets, re-align to the preset using the new size. | ||
| // Wide/full presets are driven by the parent rect, not by explicit size; skip them. | ||
| if (p_keep_offsets) { | ||
| _compute_anchors(Rect2(data.pos_cache, new_size), data.offset, data.anchor); | ||
| } else { | ||
| _compute_offsets(Rect2(data.pos_cache, new_size), data.anchor, data.offset); | ||
| bool preset_applied = false; | ||
| if (is_inside_tree() && data.anchor_preset_active) { | ||
| int current_preset = _get_anchors_layout_preset(); | ||
| if (current_preset != -1) { | ||
| _compute_preset_offsets((LayoutPreset)current_preset, new_size, data.anchor_preset_margin); | ||
| preset_applied = true; | ||
| } | ||
| } | ||
| if (!preset_applied) { | ||
| _compute_offsets(Rect2(data.pos_cache, new_size), data.anchor, data.offset); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment contradicts the code.
The comment says wide/full presets are skipped, but the branch applies _compute_preset_offsets() for every preset except -1, including PRESET_FULL_RECT and the *_WIDE presets. Either add the skip or fix the comment.
🤖 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 1578 - 1594, Update the preset handling
in the p_keep_offsets false branch around _get_anchors_layout_preset and
_compute_preset_offsets so PRESET_FULL_RECT and all *_WIDE presets bypass
_compute_preset_offsets and use the existing _compute_offsets path; keep
explicit corner/center presets applying _compute_preset_offsets.
Comment on lines
+2054
to
+2069
| void Control::set_offset_transform_scale(const Vector2 &p_scale) { | ||
| if (get_offset_transform_scale() == p_scale) { | ||
| return; | ||
| } | ||
|
|
||
| _ensure_allocated_offset_transform(); | ||
| data.offset_transform->scale = p_scale; | ||
|
|
||
| if (!data.offset_transform->enabled) { | ||
| return; | ||
| } | ||
|
|
||
| queue_redraw(); | ||
| _notify_transform(); | ||
| queue_accessibility_update(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unclamped offset_transform_scale makes the offset transform singular. Control::set_scale() guards against zero components, but the offset-transform equivalent does not, so a zero scale yields a non-invertible Transform2D that the editor preview then inverts.
scene/gui/control.cpp#L2054-L2069: clampp_scale.x/p_scale.ytoCMP_EPSILONinset_offset_transform_scale(), mirroringset_scale().editor/scene/gui/control_editor_plugin.cpp#L1183-L1197: as a defensive measure, skip drawing when the offset transform's determinant is zero instead of callingaffine_inverse()unconditionally.
📍 Affects 2 files
scene/gui/control.cpp#L2054-L2069(this comment)editor/scene/gui/control_editor_plugin.cpp#L1183-L1197
🤖 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 2054 - 2069, Clamp both components of
p_scale to CMP_EPSILON in Control::set_offset_transform_scale(), matching
set_scale(). Also update editor/scene/gui/control_editor_plugin.cpp lines
1183-1197 to skip drawing when the offset transform determinant is zero before
calling affine_inverse(); both changes must prevent singular offset transforms
from being inverted.
Comment on lines
+4605
to
+4606
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset", PROPERTY_HINT_NONE, "suffix:px", PROPERTY_USAGE_EDITOR), "set_pivot_offset", "get_pivot_offset"); | ||
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset_ratio"), "set_pivot_offset_ratio", "get_pivot_offset_ratio"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other Control transform properties declare usage, and whether pivot_offset was previously stored.
rg -n 'PNAME\("pivot_offset"\)|"pivot_offset"' scene/ editor/ -g '*.cpp' -g '*.h'
git log -1 --format=%H
git show HEAD~1:scene/gui/control.cpp 2>/dev/null | rg -n '"pivot_offset"'Repository: Redot-Engine/redot-engine
Length of output: 1284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## control.cpp related definitions"
sed -n '510,570p' scene/gui/control.cpp
sed -n '4330,4620p' scene/gui/control.cpp
echo
echo "## PropertyInfo defaults / usage semantics"
rg -n "class PropertyInfo|PropertyInfo::PropertyInfo|PROPERTY_USAGE_STORAGE|PROPERTY_USAGE_DEFAULT|PropertyHint" core -g '*.h' -g '*.cpp'Repository: Redot-Engine/redot-engine
Length of output: 34036
Restore storage usage for pivot_offset.
PropertyInfo defaults include PROPERTY_USAGE_STORAGE, but the explicit PROPERTY_USAGE_EDITOR for pivot_offset removes storage, so non-editor scenes save ignore this field even though the backing rect_pivot_offset has been renamed to pivot_offset and remains in upgrade mappings. Keep the suffix/format hint while dropping the explicit usage mask.
🐛 Proposed fix
- ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset", PROPERTY_HINT_NONE, "suffix:px", PROPERTY_USAGE_EDITOR), "set_pivot_offset", "get_pivot_offset");
+ ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset", PROPERTY_HINT_NONE, "suffix:px"), "set_pivot_offset", "get_pivot_offset");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset", PROPERTY_HINT_NONE, "suffix:px", PROPERTY_USAGE_EDITOR), "set_pivot_offset", "get_pivot_offset"); | |
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset_ratio"), "set_pivot_offset_ratio", "get_pivot_offset_ratio"); | |
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset", PROPERTY_HINT_NONE, "suffix:px"), "set_pivot_offset", "get_pivot_offset"); | |
| ADD_PROPERTY(PropertyInfo(Variant::VECTOR2, "pivot_offset_ratio"), "set_pivot_offset_ratio", "get_pivot_offset_ratio"); |
🤖 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 4605 - 4606, Update the pivot_offset
PropertyInfo registration in the control property declarations to remove the
explicit PROPERTY_USAGE_EDITOR argument while preserving the suffix:px hint,
allowing the default storage usage to remain active. Leave pivot_offset_ratio
and the existing setter/getter bindings unchanged.
GeneralProtectionFault
force-pushed
the
control_node_transform
branch
from
July 26, 2026 04:55
ebf7794 to
e6bdc09
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Essentially pulling in the functionality in this upstream PR:
godotengine/godot#87081
It's worth noting the animated properties and such in the video there were named differently by the time it was merged. All the properties are offset_[new_property_name].
I have tested animating a couple of them as in that example. Essentially, I believe these "offset" properties are transforming the control node independently of the UI constraints, bringing in a lot of new abilities in the process.
If this is accurate, I believe it would mitigate the pain of restrictions implicit in this PR:
#1317
For any users that may have hacked some desired behavior from the not-working anchor functionality, hopefully this would allow a much more staight-forward way to do those things and more.
Commits deliberately not squashed to show the history/needed commits. Merge conflicts were very light, some simply caused by doxygen comments.
Summary by CodeRabbit
Control, including enable/disable, position (absolute + ratio), pivot (absolute + ratio), rotation, scale, and visual-only mode.Controldocs to clarify pivot offset behavior and offset/anchor preset margin usage.