Skip to content

Optimize Group::select() with short-circuit checks before positionEdgeVariables() #622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
highperformancecoder merged 3 commits into from
Mar 11, 2026
+14 −0
Merged

Optimize Group::select() with short-circuit checks before positionEdgeVariables() #622

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25c675e
Initial plan
Copilot Mar 11, 2026
2a03dd2
Optimize Group::select() with short-circuit checks before positionEdg…
Copilot Mar 11, 2026
e71ed2c
Merge branch 'bugfix-610-unable-to-pull-wire' into copilot/sub-pr-621
highperformancecoder Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions model/group.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,6 +1235,20 @@ namespace minsky

ItemPtr Group::select(float x, float y) const
{
// Short-circuit: no edge variables to hit-test.
if (inVariables.empty() && outVariables.empty())
return nullptr;

// Short-circuit: avoid the expensive positionEdgeVariables() call when
// (x,y) is clearly outside the expanded group bounding box. Using
// w+h as the conservative AABB half-extent covers all rotation angles.
const float z = zoomFactor();
const float w = 0.5f * iWidth() * z;
const float h = 0.5f * iHeight() * z + topMargin * z;
const float maxExtent = w + h;
if (abs(x - this->x()) > maxExtent || abs(y - this->y()) > maxExtent)
return nullptr;

Comment on lines +1242 to +1251
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The new AABB short-circuit uses only the group icon dims (iWidth/iHeight + topMargin) to decide whether to skip positionEdgeVariables(). However edge variables can be laid out well outside those bounds (eg yOff grows with the number/height of in/out variables in layoutEdgeVariables), so this can return nullptr even when (x,y) is actually over a drawn edge variable/port, breaking hit-testing/wire dragging for groups with many edge vars (and potentially large label extents/margins). Consider widening the bound using a conservative upper bound that includes possible edge-variable extents (eg based on counts and cached bb heights/widths), caching margins/edge-extents from the last layout, or removing this short-circuit if correctness can’t be guaranteed.

Suggested change
// Short-circuit: avoid the expensive positionEdgeVariables() call when
// (x,y) is clearly outside the expanded group bounding box. Using
// w+h as the conservative AABB half-extent covers all rotation angles.
const float z = zoomFactor();
const float w = 0.5f * iWidth() * z;
const float h = 0.5f * iHeight() * z + topMargin * z;
const float maxExtent = w + h;
if (abs(x - this->x()) > maxExtent || abs(y - this->y()) > maxExtent)
return nullptr;

Copilot uses AI. Check for mistakes.
// Ensure edge variable positions are up-to-date before hit-testing.
// (Positions are otherwise only set during draw1edge() in a render pass.)
positionEdgeVariables();
Expand Down