Path3d cherrypick#1302
Conversation
cherry picked commits 3cfcedd6d57e171b54a342842bf807479b0c903e (make changing disk size for path3d editor plugin tilt gizmo not require engine restart) and 365f8fd038ec47b8f22f4960e13c11264be0b515 (add option to snap handles to colliders in path3d editor plugin) from Godot.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPath3D editing adds optional collider snapping for point placement and primary-handle dragging through deferred physics-frame raycasts. Gizmo disk sizing now reads editor settings directly, and a menu checkbox controls snapping. ChangesPath3D collider snapping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Path3DEditorPlugin
participant Physics as PhysicsDirectSpaceState3D
participant Gizmo as Path3DGizmo
participant History as UndoRedo
Editor->>Editor: Store click or handle ray state
Editor->>Physics: intersect_ray on physics frame
Physics-->>Editor: Collider hit or plane fallback
Editor->>Gizmo: Apply snapped handle position
Editor->>History: Commit snapped point addition
🚥 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: 2
🤖 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 `@editor/scene/3d/path_3d_editor_plugin.cpp`:
- Around line 703-713: Update the deferred snap/edit flow around make_visible,
the waiting flags, and the processing paths near the ray setup and commit logic
to clear pending state whenever the plugin is hidden or the active Path3D
changes. Capture the target Path3D, curve, camera, and matching _edit.gizmo when
deferring the edit, then validate they remain valid and unchanged before
committing; otherwise cancel the deferred operation without dereferencing null
or stale objects.
- Around line 125-133: Update the deferred movement branch in the path gizmo
handling code to apply only when collider snapping is enabled, matching the
existing point-placement behavior near the collider-snap check. When snapping is
disabled, continue primary-handle movement immediately instead of setting the
physics wait state.
🪄 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: d7c8713e-e891-4695-a043-7e2284ed20b6
📒 Files selected for processing (3)
editor/scene/3d/path_3d_editor_plugin.cppeditor/scene/3d/path_3d_editor_plugin.heditor/settings/editor_settings.cpp
|
|
||
| Vector3 ray_from = viewport->get_ray_pos(mbpos); | ||
| Vector3 ray_dir = viewport->get_ray(mbpos); | ||
|
|
||
| if (snap_to_collider) { | ||
| _edit.click_ray_pos = ray_from; | ||
| _edit.click_ray_dir = ray_dir * p_camera->get_far(); | ||
| _edit.gizmo_camera = p_camera; | ||
| _edit.origin = origin; | ||
| _edit.waiting_point_physics = true; | ||
| return EditorPlugin::AFTER_GUI_INPUT_STOP; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cancel or validate deferred edits when the active Path3D changes.
make_visible(false) clears path without clearing the waiting flags. When processing resumes, this can dereference a null target, apply the old ray to a newly selected Path3D, or call through an invalid _edit.gizmo when no matching gizmo was found.
Clear deferred state on hide/edit changes and capture or validate the target Path3D, curve, camera, and gizmo before committing.
Also applies to: 815-816, 965-1010
🤖 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/3d/path_3d_editor_plugin.cpp` around lines 703 - 713, Update the
deferred snap/edit flow around make_visible, the waiting flags, and the
processing paths near the ray setup and commit logic to clear pending state
whenever the plugin is hidden or the active Path3D changes. Capture the target
Path3D, curve, camera, and matching _edit.gizmo when deferring the edit, then
validate they remain valid and unchanged before committing; otherwise cancel the
deferred operation without dereferencing null or stale objects.
There was a problem hiding this comment.
I am not sure if this is actually possible in practice?
I made a few paths in the editor and have tried switching between them with the tool active and I cant seem to reproduce any negative effects. Can you give me a reproduction example?
There was a problem hiding this comment.
Arctis-Fireblight, fair question — under default settings (60 physics ticks/sec) the race window is only a few milliseconds, so it's genuinely hard to hit by clicking around normally. Here's a way to reproduce it deterministically:
- Enable Snap to Colliders on the Path3D toolbar.
- In Project Settings → Physics → Common, temporarily set Physics Ticks per Second to something very low, e.g.
1. This widens the gap between the click and the nextNOTIFICATION_PHYSICS_PROCESStick to ~1 second, giving you time to act in between. - Select Path3D node A, activate the "Add Point" tool, and click in the viewport to place a point. This sets
_edit.waiting_point_physics = trueand returns immediately (Line 725) — the point isn't added yet, it's deferred to the next physics tick. - Before that tick fires (you now have ~1 second), deselect the node entirely (click empty space in the Scene panel, or select a non-Path3D node). This triggers
make_visible(false), which setspath = nullptr(Line 806) but leaves_edit.waiting_point_physicsset. - When the deferred physics tick runs,
_notification()'sNOTIFICATION_PHYSICS_PROCESSbranch callspath->get_global_transform()(Line 966) withpath == nullptr→ crash.
A milder variant of the same root cause (no crash, but wrong data) is switching directly from Path3D A to Path3D B within that window: edit() reassigns path to B without going through make_visible, so the pending click ray from A ends up adding a point to B's curve — since _notification() re-reads path->get_curve() at commit time rather than using a captured reference. This second one you may have partially exercised already, but it's likely too fast/subtle to notice as "wrong" without deliberately checking which curve got the new point.
Reset the physics tick rate back to its normal value afterward. Let me know if you can reproduce it with this approach — happy to reconsider if it still doesn't manifest.
There was a problem hiding this comment.
Even setting the physics tick rate to 1 does not seem to allow me to reproduce this crash. I've tried about a dozen times or so. That said I am not sure if the project settings are actually impacting the editor since even with the tick rate set to 1, moving the control points around with snapping enabled is quite smooth.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'physics_ticks_per_second' --type=cpp -C2 main/ 2>/dev/null | head -100
rg -n 'physics_ticks_per_second' --type=cpp -C2 core/ 2>/dev/null | head -60Length of output: 8817
🏁 Script executed:
#!/bin/bash
rg -n 'physics_ticks_per_second' --type=cpp editor/ 2>/dev/null
rg -n 'set_physics_ticks_per_second' --type=cpp . 2>/dev/null | grep -v main.cppLength of output: 1520
@Arctis-Fireblight, thanks for testing this — and you're right to be skeptical of my repro steps. I dug into why the tick-rate change had no visible effect:
physics/common/physics_ticks_per_second is only read once, in Main::setup() (main/main.cpp Line 2070), via Engine::get_singleton()->set_physics_ticks_per_second(GLOBAL_DEF_BASIC(...)). There is no listener anywhere in editor/ that re-applies this value when you change it in the Project Settings dialog at runtime — it only takes effect after restarting the editor. That's exactly consistent with what you saw: dragging remained smooth because the actual physics tick rate never changed. My suggested repro was flawed on that basis, and I apologize for the wasted testing time.
That said, I'd distinguish between "hard to trigger by hand" and "not a real bug": even at the default 60 Hz, there's still a real window (up to ~16ms) between the click/drag event and the next NOTIFICATION_PHYSICS_PROCESS tick where _edit.waiting_point_physics/waiting_handle_physics is true but path could be swapped out from under it via make_visible(false) or edit(). Reliably hitting a ~16ms window by manually clicking in the Scene panel is understandably very difficult (essentially guessing at frame timing), so I don't have a practical manual repro to offer you right now, given the setting doesn't actually let you widen that window without a rebuild/patch to force a lower tick rate live.
Given I can't currently produce an evidence-backed reproduction, I won't push further on this as a blocking concern for this PR. If you'd like, I'm happy to open a follow-up issue documenting the theoretical race (null path dereference in _notification()'s NOTIFICATION_PHYSICS_PROCESS branch, and stale-target risk when switching between two Path3D nodes) so it isn't lost, without holding up this cherry-pick. Let me know if you'd like me to file that.
(๑•́ ₃ •̀๑) still puzzling over that timing window
✏️ Learnings added
Learnt from: Arctis-Fireblight
Repo: Redot-Engine/redot-engine PR: 1302
File: editor/scene/3d/path_3d_editor_plugin.cpp:703-713
Timestamp: 2026-07-11T21:35:51.159Z
Learning: In Redot Engine (Godot-derived) codebase, the `physics/common/physics_ticks_per_second` project setting is only applied once via `Engine::set_physics_ticks_per_second(GLOBAL_DEF_BASIC(...))` in `Main::setup()` (main/main.cpp). There is no live listener in the editor that re-applies this setting when changed at runtime through the Project Settings dialog; the editor must be restarted for a changed tick rate to actually take effect.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Ok, so you are indeed correct that the tick rate is only read once. Restarting the editor after making the change does in fact drop it to 1 tick per second. However, even at that low tick rate, I cannot reproduce a crash, error message, or any other odd behaviors. As far as I can tell, deselecting the currently edited node seems to immediately deactivate the tool so no crash.
The reason I push back on this is more or less due to this feature being pulled directly from Godot upstream and was included in their 4.7 release, and I dont see any reports of a crash caused by this upstream.
I'd like to at least confirm the behavior before I go and make Tekk go fix a problem that looks like is being handled elsewhere from my testing.
There was a problem hiding this comment.
Okay so, I'm not 100% sure, but as far as i understand, edit() will always be called before make_visible(true) which calls set_physics_process(true), which means that path will not be null because physics processing isn't enabled before that. This pattern is also used in editor/animation/animation_tree_editor_plugin.cpp and modules/gridmap/editor/grid_map_editor_plugin.cpp. But if I'm wrong, the other plugins would have to also be fixed. :)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
…ss frames on unused feature
4 participants
cherry pick commits godotengine/godot@3cfcedd (make changing path3d editor plugin gizmo tilt disk size not require engine restart) and godotengine/godot@365f8fd (add option to snap path3d editor plugin path handles to colliders in the scene).
it works on my machine :)
Summary by CodeRabbit