fix: Correct picking formulas#999
Open
afonsobspinto wants to merge 5 commits into
Open
Conversation
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
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.
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:
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:
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 pickDiameterrow-major buffer. Each entry should be addressed with coordinates relative to that pick window:The previous clearing code computed absolute window coordinates
xandy, then used those as though they were local buffer coordinates: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:
That keeps the two coordinate systems separate:
x/yare viewport coordinates used for bounds checks, whilerelativeX/relativeYare 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 insidepickOffsetSequence.pickOffsetSequencestores buffer offsets sorted by distance from the pick-window center. The old 2D code usedoffsetto compute the selected pixel position, but read the pick value usingi: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:The helper returns both the sampled value and the corresponding
relativeX/relativeYderived from that sameoffset. 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.
setPointMarkerSizeupdatesng_markerDiameter, but notng_markerBorderWidth:The default point marker border width remains
1.0unless explicitly overridden:The 2D point draw path initializes the line shader with a
1pxfeather too. Therefore we get:Here:
lineWidthInPixelsis the point marker interior size,1.uLineParams.zis the feather width,1.borderWidthis the marker border width,1.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
5pxwide, the pick buffer can contain the annotation pick ID in these cells:pick_radius = 1means the click pixel plus one pixel on each horizontal side are checked. Along the x-axis, if the click is atX, Neuroglancer checks:With
check_pick(webdriver, 3), the click is atC+3.There is overlap at
C+2:Therefore: