Skip to content

Fix CSG operations not respecting the top_level flag on child nodes#117657

Open
ryevdokimov wants to merge 1 commit intogodotengine:masterfrom
Open-Industry-Project:csg-top-level
Open

Fix CSG operations not respecting the top_level flag on child nodes#117657
ryevdokimov wants to merge 1 commit intogodotengine:masterfrom
Open-Industry-Project:csg-top-level

Conversation

@ryevdokimov
Copy link
Copy Markdown
Contributor

@ryevdokimov ryevdokimov commented Mar 19, 2026

Fixes: #117651

2026-03-19.17-56-13.mp4

@ryevdokimov ryevdokimov added this to the 4.x milestone Mar 19, 2026
@ryevdokimov ryevdokimov requested a review from a team as a code owner March 19, 2026 22:05
Copy link
Copy Markdown
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some issue with the gizmos. In the MRP from #117651 I've added another top level child (grandchild of root CSGShape3D). See it staying it the correct place, but the gizmo is at wrong spot until updated:

godot.windows.editor.x86_64_RlzXukvAxg.mp4

Also when having both top level shapes selected, the gizmo moves only the parent one shape. Shouldn't it move both, separately?

godot.windows.editor.x86_64_oVMFX1iTjo.mp4

Comment thread modules/csg/csg_shape.cpp
has_top_level_child = true;
break;
}
}
Copy link
Copy Markdown
Member

@kleonc kleonc Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it a comment for whole this loop)
Shouldn't this be cached? Usually there are probably no top level children, in which case this would needlessly iterate over all children. Seems like unnecessary overhead. On the other hand, trigerring NOTIFICATION_LOCAL_TRANSFORM_CHANGED on CSGShape3D is also probably an unusual case? 🤔

But I think adding virtual void _top_level_changed() to Node3D and calling it in Node3D::set_as_top_level should be fine (as changing top level is usually a one time thing so minimal overhead is not an issue there; also note a similar callback exists in CanvasItem (actually more complex as it's propagating to the children there)). Then CSGShape3D could have int top_level_child_shape_count = 0 member which would be incremented/decremented for the parent_shape on NOTIFICATION_PARENTED/NOTIFICATION_UNPARENTED, and in CSGShape3D::_top_level_changed override.
Then here you'd have if (top_level_child_shape_count > 0).

Comment thread modules/csg/csg_shape.cpp
transformed_brush.copy_from(*child_brush, child->get_transform());
Transform3D child_xform;
if (child->is_set_as_top_level()) {
child_xform = get_global_transform().affine_inverse() * child->get_global_transform();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That affine_inverse() could be potentially calculated only once. If going with the virtual void _top_level_changed() suggestion from the other comment it could be:

Transform3D global_transform_inverse;
if (top_level_child_shape_count > 0) {
	global_transform_inverse = get_global_transform().affine_inverse();
}

outside the loop, and here:

Suggested change
child_xform = get_global_transform().affine_inverse() * child->get_global_transform();
child_xform = global_transform_inverse * child->get_global_transform();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CSG operations do not respect the "top_level" flag of the node's transform

3 participants