Coordinate flip in Picking.js causes out-of-bounds access when y=0

[gilgilad]

What happened?

Hello ,
In the pickPositionWorldCoordinates function at line 632:

cesium/packages/engine/Source/Scene/Picking.js

Line 632 in 7355e1e

drawingBufferPosition.y = scene.drawingBufferHeight - drawingBufferPosition.y;

There is a Y-coordinate flip:
drawingBufferPosition.y = scene.drawingBufferHeight - drawingBufferPosition.y;

However, this creates an off-by-one error. When drawingBufferPosition.y = 0, the result becomes scene.drawingBufferHeight, which is out of bounds since valid pixel coordinates should range from [0, scene.drawingBufferHeight - 1].
For example, with a drawing buffer height of 270:

Input: y = 0 → Output: y = 270 (invalid - should be 269)
Input: y = 269 → Output: y = 1 (valid)

Should this be corrected to:
drawingBufferPosition.y = scene.drawingBufferHeight - drawingBufferPosition.y - 1

This would ensure the coordinate stays within valid bounds [0, height-1] after the flip.
Is this intentional behavior, or should it be fixed?

[ggetz]

Hi @gilgilad, this sounds like an unintentional oversight to me.

Would you happen to have a test case which reproduces the issue? That would help to confirm a potential fix.

[javagl]

Reproducing it is possible with this sandcastle:

https://sandcastle.cesium.com/#c=fVJRa+MwDP4rIk8ZFOfudUvLjlLYwUoH3W0vgeHaambOkYPtZHRH//vkpB3tbtub9Un69H2ylKMQoTf4gh6mQPgCcwyma8TDgOVVpoZ47ihKQ+irbAL/KgKIpkHLyCVspQ04SZgk08hoHJ2CQSHh0mm8M+ov+tPURga8lTv0H1P7i6uKRlniaVTwaHSNkSOP2sR3PSLEnUWhTWit3LGHKiNHWGVMoAZ3z5K0/WhvzTRI61YqXPRI8WYsyg8zB81CSeplSFIOHCJg/E1tF3+p5DLfdjQ8IG9cjw3zXIzLSZMdy7Kufk8JJH3ngkkNiRPAbOHTrGAf0yn8OJCd01XZDXr2N1IAFIXGTVfX6AdgfxwfwWPobGTfZ6ZaXvVx0KPzVs+d89qQjBi+0XoqYSRO+H7y1ULvdy2K5erPevG0XD0sUnE2ycrht2aJ8No0rfMROm9zIYqIDX8gayg2Hd9CFCoMmy+LY0upTQ9GTz85SVBWhsCZbWft2rzyemZlwfVnbdZJ9lmvevR8Kqnk+efsdgSFEGXB4f9d0Tm7kf6EMW39DQ

But this invalid coordinate is just passed to readPixels, which does not seem to give a care about whether the coordinates are valid or not (it simply returns "empty pixels" then). So this does not break anything, it is just wrong...