Skip to content

fix: Correct picking formulas#999

Open
afonsobspinto wants to merge 5 commits into
google:masterfrom
MetaCell:feature/picking
Open

fix: Correct picking formulas#999
afonsobspinto wants to merge 5 commits into
google:masterfrom
MetaCell:feature/picking

Conversation

@afonsobspinto
Copy link
Copy Markdown
Contributor

@afonsobspinto afonsobspinto commented May 20, 2026

Summary

Fixes some picking bugs caused by inconsistent mouse-coordinate conversion and incorrect pick-buffer indexing.

Problems

1. Panel Border Was Applied Incorrectly

The previous code handled the panel borders inconsistently:

mouseX = ... - element.clientLeft;
mouseY = ... + element.clientTop;

getBoundingClientRect() gives coordinates relative to the panel border box. To get the mouse position relative to the panel content, both borders need to be subtracted. The X path already subtracted the left border, but the Y path added the top border instead.

With the current border size, that makes picking about 4 pixels off in Y. If the border size is expanded later, the picking error grows with it.

This is fixed by making the Y path match the X path:

mouseX = ... - element.clientLeft;
mouseY = ... - element.clientTop;

That gives the picking code the mouse position relative to the actual rendered panel content instead of relative to the outer border box.

2. Out-Of-Bounds Clearing Used Window Coordinates As Buffer Indices

The pick readback buffer is a small pickDiameter x pickDiameter row-major buffer. Each entry should be addressed with coordinates relative to that pick window:

relativeY * pickDiameter + relativeX

The previous clearing code computed absolute window coordinates x and y, then used those as though they were local buffer coordinates:

buffer[baseOffset + (y * pickDiameter + x) * stride] = 0;

That could clear the wrong buffer entry when the pick window overlapped the viewport edge. This probably did not show up often because it only affects picking close to panel boundaries, where picking is comparatively rare.

This is fixed by still using absolute window coordinates only to decide whether a sample is outside the viewport, but using the relative pick-window coordinates to address the readback buffer:

const x = startX + relativeX;
const y = startY + relativeY;
if (x < 0 || y < 0 || x >= viewportWidth || y >= viewportHeight) {
  buffer[baseOffset + (relativeY * pickDiameter + relativeX) * stride] = 0;
}

That keeps the two coordinate systems separate: x/y are viewport coordinates used for bounds checks, while relativeX/relativeY are buffer coordinates used for indexing.

3. 2D Picking Mixed Up Search Order And Buffer Offset

The previous 2D picking code mixed up two different indices:

  • offset: the actual row-major pixel location inside the readback buffer.
  • i: the search-order index inside pickOffsetSequence.

pickOffsetSequence stores buffer offsets sorted by distance from the pick-window center. The old 2D code used offset to compute the selected pixel position, but read the pick value using i:

const offset = pickOffsetSequence[i];
const pickId = data[4 * i];

That means the code could use the nearest sample's position while reading the pick ID from a different buffer pixel. The 3D path was already doing this correctly by reading from data[4 * offset].

This is fixed by adding a shared helper, resolveNearestPanelPickSample, that always reads the pick data from the sampled row-major buffer offset:

const offset = pickOffsetSequence[i];
const pickValue = data[pickBaseOffset + stride * offset] ?? 0;

The helper returns both the sampled value and the corresponding relativeX/relativeY derived from that same offset. Both 2D and 3D picking now use this helper, so the selected position and selected object ID always come from the same pick-buffer sample.

(This explains why picking nodes in 2D polylines was unreliable while 3D picking worked.)


Updated Test explanation in detail

The test_pick_radius was updated as I believe it was passing before incorrectly due to the 2D Picking Mixed Up Search Order And Buffer Offset problem.

setPointMarkerSize updates ng_markerDiameter, but not ng_markerBorderWidth:

The default point marker border width remains 1.0 unless explicitly overridden:

// src/annotation/point.ts
ng_markerDiameter = 5.0;
ng_markerBorderWidth = 1.0;

The 2D point draw path initializes the line shader with a 1px feather too. Therefore we get:

// src/webgl/lines.ts
float totalLineWidth =
  lineWidthInPixels +
  2.0 * uLineParams.z +
  2.0 * borderWidth;

Here:

  • lineWidthInPixels is the point marker interior size, 1.
  • uLineParams.z is the feather width, 1.
  • borderWidth is the marker border width, 1.
totalLineWidth = 1 + 2 * 1 + 2 * 1 = 5px

The pick buffer receives the annotation pick ID wherever the rendered annotation fragment is emitted:
That means the pick ID follows the rendered 2D marker footprint. It is not limited to the single center pixel implied by setPointMarkerSize(1.0).

In the test we have a scenario similar to:

Assume the annotation center is C.

Because the effective pickable footprint is about 5px wide, the pick buffer can contain the annotation pick ID in these cells:

C-2   C-1    C    C+1   C+2
[ID]  [ID]  [ID]  [ID]  [ID]

pick_radius = 1 means the click pixel plus one pixel on each horizontal side are checked. Along the x-axis, if the click is at X, Neuroglancer checks:

X-1   X   X+1

With check_pick(webdriver, 3), the click is at C+3.

Annotation pick-ID cells:

C-2   C-1    C    C+1   C+2   C+3   C+4
[ID]  [ID]  [ID]  [ID]  [ID]  [ ]   [ ]

Click at offset 3:

                              X
                             C+3

pick_radius = 1 checks:

                       P      P      P
                      C+2    C+3    C+4

There is overlap at C+2:

C+2 has [ID] and is checked by pick_radius = 1

Therefore:

check_pick(webdriver, 3) is True (test was expecting it to be False)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants