Skip to content

fix: Correct picking formulas #142

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

Open
afonsobspinto wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Correct picking formulas #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
107 changes: 60 additions & 47 deletions src/perspective_view/panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ import type {
RenderedDataViewerState,
} from "#src/rendered_data_panel.js";
import {
getCenteredPickWindowCoordinate,
getPickDiameter,
getPickOffsetSequence,
RenderedDataPanel,
resolveNearestPanelPickSample,
} from "#src/rendered_data_panel.js";
import {
DerivedProjectionParameters,
Expand Down Expand Up @@ -717,55 +719,66 @@ export class PerspectivePanel extends RenderedDataPanel {
mouseState.pickedRenderLayer = null;
const pickDiameter = getPickDiameter(pickRadius);
const pickOffsetSequence = getPickOffsetSequence(pickRadius);
const numOffsets = pickOffsetSequence.length;
for (let i = 0; i < numOffsets; ++i) {
const offset = pickOffsetSequence[i];
const zValue = data[4 * offset];
if (zValue === 0) continue;
const relativeX = offset % pickDiameter;
const relativeY = (offset - relativeX) / pickDiameter;
const glWindowZ = 1.0 - zValue;
tempVec3[0] =
(2.0 * (glWindowX + relativeX - pickRadius)) /
pickingData.viewportWidth -
1.0;
tempVec3[1] =
(2.0 * (glWindowY + relativeY - pickRadius)) /
pickingData.viewportHeight -
1.0;
tempVec3[2] = 2.0 * glWindowZ - 1.0;
vec3.transformMat4(tempVec3, tempVec3, pickingData.invTransform);
let { position: mousePosition, unsnappedPosition } = mouseState;
const { value: voxelCoordinates } = this.navigationState.position;
const rank = voxelCoordinates.length;
if (mousePosition.length !== rank) {
mousePosition = mouseState.position = new Float32Array(rank);
}
if (unsnappedPosition.length !== rank) {
unsnappedPosition = mouseState.unsnappedPosition = new Float32Array(
rank,
);
}
mousePosition.set(voxelCoordinates);
mouseState.coordinateSpace = this.navigationState.coordinateSpace.value;
const displayDimensions =
this.navigationState.pose.displayDimensions.value;
const { displayDimensionIndices } = displayDimensions;
for (
let i = 0, spatialRank = displayDimensionIndices.length;
i < spatialRank;
++i
) {
mousePosition[displayDimensionIndices[i]] = tempVec3[i];
}
unsnappedPosition.set(mousePosition);
const pickValue = data[4 * pickDiameter * pickDiameter + 4 * offset];
pickingData.pickIDs.setMouseState(mouseState, pickValue);
mouseState.displayDimensions = displayDimensions;
mouseState.setActive(true);
const resolvedPick = resolveNearestPanelPickSample(
data,
pickOffsetSequence,
pickRadius,
{
depthBaseOffset: 0,
pickBaseOffset: 4 * pickDiameter * pickDiameter,
},
);
if (resolvedPick === undefined || resolvedPick.depthValue === undefined) {
mouseState.setActive(false);
return;
}
mouseState.setActive(false);
const glWindowZ = 1.0 - resolvedPick.depthValue;
tempVec3[0] =
(2.0 *
getCenteredPickWindowCoordinate(
glWindowX,
resolvedPick.relativeX,
pickRadius,
)) /
pickingData.viewportWidth -
1.0;
tempVec3[1] =
(2.0 *
getCenteredPickWindowCoordinate(
glWindowY,
resolvedPick.relativeY,
pickRadius,
)) /
pickingData.viewportHeight -
1.0;
tempVec3[2] = 2.0 * glWindowZ - 1.0;
vec3.transformMat4(tempVec3, tempVec3, pickingData.invTransform);
let { position: mousePosition, unsnappedPosition } = mouseState;
const { value: voxelCoordinates } = this.navigationState.position;
const rank = voxelCoordinates.length;
if (mousePosition.length !== rank) {
mousePosition = mouseState.position = new Float32Array(rank);
}
if (unsnappedPosition.length !== rank) {
unsnappedPosition = mouseState.unsnappedPosition = new Float32Array(
rank,
);
}
mousePosition.set(voxelCoordinates);
mouseState.coordinateSpace = this.navigationState.coordinateSpace.value;
const displayDimensions = this.navigationState.pose.displayDimensions.value;
const { displayDimensionIndices } = displayDimensions;
for (
let i = 0, spatialRank = displayDimensionIndices.length;
i < spatialRank;
++i
) {
mousePosition[displayDimensionIndices[i]] = tempVec3[i];
}
unsnappedPosition.set(mousePosition);
pickingData.pickIDs.setMouseState(mouseState, resolvedPick.pickValue);
mouseState.displayDimensions = displayDimensions;
mouseState.setActive(true);
}

translateDataPointByViewportPixels(
Expand Down
108 changes: 20 additions & 88 deletions src/rendered_data_panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ import type { DisplayContext } from "#src/display_context.js";
import { RenderedPanel } from "#src/display_context.js";
import type { NavigationState } from "#src/navigation_state.js";
import { PickIDManager } from "#src/object_picking.js";
import {
clearOutOfBoundsPickData,
getPickDiameter,
} from "#src/rendered_data_panel_picking.js";
import {
displayToLayerCoordinates,
layerToDisplayCoordinates,
Expand Down Expand Up @@ -77,88 +81,14 @@ export class PickRequest {

const pickRequestInterval = 30;

export function getPickDiameter(pickRadius: number): number {
return 1 + pickRadius * 2;
}

let _cachedPickRadius = -1;
let _cachedPickOffsetSequence: Uint32Array | undefined;

/**
* Sequence of offsets into C order (pickDiamater, pickDiamater) array in order of increasing
* distance from center.
*/
export function getPickOffsetSequence(pickRadius: number) {
if (pickRadius === _cachedPickRadius) {
return _cachedPickOffsetSequence!;
}
_cachedPickRadius = pickRadius;
const pickDiameter = 1 + pickRadius * 2;
const maxDist2 = pickRadius ** 2;
const getDist2 = (x: number, y: number) =>
(x - pickRadius) ** 2 + (y - pickRadius) ** 2;

let offsets = new Uint32Array(pickDiameter * pickDiameter);
let count = 0;
for (let x = 0; x < pickDiameter; ++x) {
for (let y = 0; y < pickDiameter; ++y) {
if (getDist2(x, y) > maxDist2) continue;
offsets[count++] = y * pickDiameter + x;
}
}
offsets = offsets.subarray(0, count);
offsets.sort((a, b) => {
const x1 = a % pickDiameter;
const y1 = (a - x1) / pickDiameter;
const x2 = b % pickDiameter;
const y2 = (b - x2) / pickDiameter;
return getDist2(x1, y1) - getDist2(x2, y2);
});
return (_cachedPickOffsetSequence = offsets);
}

/**
* Sets array elements to 0 that would be outside the viewport.
*
* @param buffer Array view, which contains a C order (pickDiameter, pickDiameter) array.
* @param baseOffset Offset into `buffer` corresponding to (0, 0).
* @param stride Stride between consecutive elements of the array.
* @param glWindowX Center x position, must be integer.
* @param glWindowY Center y position, must be integer.
* @param viewportWidth Width of viewport in pixels.
* @param viewportHeight Width of viewport in pixels.
*/
export function clearOutOfBoundsPickData(
buffer: Float32Array,
baseOffset: number,
stride: number,
glWindowX: number,
glWindowY: number,
viewportWidth: number,
viewportHeight: number,
pickRadius: number,
) {
const pickDiameter = 1 + pickRadius * 2;
const startX = glWindowX - pickRadius;
const startY = glWindowY - pickRadius;
if (
startX >= 0 &&
startY >= 0 &&
startX + pickDiameter <= viewportWidth &&
startY + pickDiameter <= viewportHeight
) {
return;
}
for (let relativeY = 0; relativeY < pickDiameter; ++relativeY) {
for (let relativeX = 0; relativeX < pickDiameter; ++relativeX) {
const x = startX + relativeX;
const y = startY + relativeY;
if (x < 0 || y < 0 || x >= viewportWidth || y >= viewportHeight) {
buffer[baseOffset + (y * pickDiameter + x) * stride] = 0;
}
}
}
}
export {
Copy link
Copy Markdown

@seankmartin seankmartin Apr 30, 2026

Choose a reason for hiding this comment

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

To change - We can avoid the re-export, neuroglancer doesn't guarantee a stable API

clearOutOfBoundsPickData,
getCenteredPickWindowCoordinate,
getPickDiameter,
getPickOffsetSequence,
resolveNearestPanelPickSample,
type ResolvedPanelPickSample,
} from "#src/rendered_data_panel_picking.js";

export abstract class RenderedDataPanel extends RenderedPanel {
/**
Expand Down Expand Up @@ -234,13 +164,15 @@ export abstract class RenderedDataPanel extends RenderedPanel {
gl.bindBuffer(WebGL2RenderingContext.PIXEL_PACK_BUFFER, buffer);
}
const { renderViewport } = this;
const glWindowX =
const glWindowX = Math.floor(
Copy link
Copy Markdown

@seankmartin seankmartin Apr 30, 2026

Choose a reason for hiding this comment

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

Note for when we make PR - Problem, without rounding to int (floor makes sense) it is possible we could end up with a floating point value here. This immediately goes into readPixels which is expecting an int, and how our float is handled is likely implementation dependent. It is also used in clearOutOfBoundsPickData, but with our other fix to use relative X and Y that could handle fractional data so it's less of a problem there.

This change is related to the change to add 0.5 to the pick because now we're flooring and using the 0.5 to use the center. But I think we can remove the 0.5 add and land at the pixel edge instead of the pixel centre. In neuroglancer, the origin of a voxel is the lower corner anyway, not the centre. I've noted this bit elsewhere

this.mouseX -
renderViewport.visibleLeftFraction * renderViewport.logicalWidth;
const glWindowY =
renderViewport.visibleLeftFraction * renderViewport.logicalWidth,
);
const glWindowY = Math.floor(
renderViewport.height -
(this.mouseY -
renderViewport.visibleTopFraction * renderViewport.logicalHeight);
(this.mouseY -
renderViewport.visibleTopFraction * renderViewport.logicalHeight),
);
this.issuePickRequest(glWindowX, glWindowY, pickRadius);
pickRequest.sync = gl.fenceSync(
WebGL2RenderingContext.SYNC_GPU_COMMANDS_COMPLETE,
Expand Down Expand Up @@ -882,7 +814,7 @@ export abstract class RenderedDataPanel extends RenderedPanel {
((clientX - bounds.left) / bounds.width) * element.offsetWidth -
element.clientLeft;
const mouseY =
((clientY - bounds.top) / bounds.height) * element.offsetHeight +
((clientY - bounds.top) / bounds.height) * element.offsetHeight -
Copy link
Copy Markdown

@seankmartin seankmartin Apr 30, 2026

Choose a reason for hiding this comment

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

Note for PR - definite problem - the border around the panels is accidentally getting added to mouse position. Result would be a 4 pixel off selection in Y, and moreso if the border size is expanded.

element.clientTop;
const { mouseState } = this.viewer;
mouseState.pageX = clientX + window.scrollX;
Expand Down
93 changes: 93 additions & 0 deletions src/rendered_data_panel_picking.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/**
* @license
* Copyright 2026 Google Inc.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { describe, expect, it } from "vitest";

import {
clearOutOfBoundsPickData,
getPickDiameter,
getPickOffsetSequence,
resolveNearestPanelPickSample,
} from "#src/rendered_data_panel_picking.js";

describe("resolveNearestPanelPickSample", () => {
it("dereferences slice pick data using the sampled offset", () => {
const pickRadius = 5;
const pickOffsetSequence = getPickOffsetSequence(pickRadius);
const targetOffset = pickOffsetSequence[1];
const data = new Float32Array(4 * getPickDiameter(pickRadius) ** 2);
data[4 * targetOffset] = 17;

expect(
resolveNearestPanelPickSample(data, pickOffsetSequence, pickRadius),
).toEqual({
offset: targetOffset,
relativeX: targetOffset % getPickDiameter(pickRadius),
relativeY:
(targetOffset - (targetOffset % getPickDiameter(pickRadius))) /
getPickDiameter(pickRadius),
pickValue: 17,
depthValue: undefined,
});
});

it("returns depth and pick payload from the same sampled pixel", () => {
const pickRadius = 2;
const pickDiameter = getPickDiameter(pickRadius);
const pickOffsetSequence = getPickOffsetSequence(pickRadius);
const targetOffset = pickOffsetSequence[3];
const data = new Float32Array(2 * 4 * pickDiameter * pickDiameter);
data[4 * targetOffset] = 0.25;
data[4 * pickDiameter * pickDiameter + 4 * targetOffset] = 23;

expect(
resolveNearestPanelPickSample(data, pickOffsetSequence, pickRadius, {
depthBaseOffset: 0,
pickBaseOffset: 4 * pickDiameter * pickDiameter,
}),
).toEqual({
offset: targetOffset,
relativeX: targetOffset % pickDiameter,
relativeY: (targetOffset - (targetOffset % pickDiameter)) / pickDiameter,
pickValue: 23,
depthValue: 0.25,
});
});
});

describe("clearOutOfBoundsPickData", () => {
it("zeros the relative pick-window indices that fall outside the viewport", () => {
const pickRadius = 1;
const pickDiameter = getPickDiameter(pickRadius);
const buffer = new Float32Array(4 * pickDiameter * pickDiameter).fill(1);

clearOutOfBoundsPickData(
buffer,
0,
4,
0,
0,
3,
3,
pickRadius,
);

expect(buffer[0]).toBe(0);
expect(buffer[4]).toBe(0);
expect(buffer[4 * pickDiameter]).toBe(0);
expect(buffer[4 * (pickDiameter + 1)]).toBe(1);
});
});
Loading
Loading