Conversation
|
@microsoft-github-policy-service agree |
Implements DECSET 1016 (sgr-pixels), which uses the same CSI sequence format as SGR mode (1006) but reports pixel coordinates instead of character-cell coordinates. This enables sub-cell mouse precision for sixel-aware programs and smooth scrolling in TUI frameworks. - Add SgrPixelMouseEncoding mode, mutually exclusive with 1005/1006 - Thread pixel coordinates through the input pipeline - Add DECSET/DECRST/DECRPM support for mode 1016 - Add SgrPixelModeTests covering coords, buttons, modifiers, exclusivity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1845cbd to
b3bf440
Compare
|
I haven't reviewed the code in detail, but I think there are couple of high level issues that need to be addressed:
|
src/host/inputBuffer.cpp
Outdated
| // Approximate pixel position from cell coordinates for SGR-Pixel mode (1016). | ||
| const auto fontSize = gci.GetActiveOutputBuffer().GetCurrentFont().GetSize(); | ||
| const til::point pixelPosition{ position.x * fontSize.width, position.y * fontSize.height }; |
There was a problem hiding this comment.
I concur with what @j4james said (who conceived and guided development the majority of this code). To add: This VT sequence doesn't report actual display pixels. Instead it reports them using the pixel size of the original DEC which was (among others) 10*20 pixels. It's almost like a legacy "DPI-independent coordinate format".
Regarding the implementation specifically, I think we should pass the viewportPos as two float parameters and remove the pixelPosition parameter. Clicking in the middle of the cell at 10,23 will then pass 10.5, 23.5. The TerminalInput class can then either truncate the float to an integer (using e.g. lrint) for existing VT sequences or multiply by 10/20 and then truncate to an integer for your new code.
This requires minimal changes in the UI code since it already calculates viewport position in pixels / cell size in pixels. It does it using integers right now and we'd just need to turn that into floats as far as I can see.
Rework the coordinate pipeline per reviewer feedback: 1. Use virtual pixel resolution (10x20 per cell) instead of raw display pixels. The DEC VT340 convention defines character cells as 10x20 virtual pixels, so coordinates are computed as viewportX*10 and viewportY*20, then reported 1-based in SGR format. 2. Pass fractional cell coordinates (float) through the mouse event chain instead of a separate pixelPosition parameter. The UI layer already computes pixel/cellSize — switching from integer to float division preserves sub-cell precision. TerminalInput truncates to int for cell-based modes or multiplies by 10/20 for SGR-Pixels. 3. Scroll offset is now applied to the float Y coordinate in _sendMouseEventHelper, preserving sub-cell precision through the adjustment. 4. Float-based clamping in Terminal::SendMouseEvent ensures coordinates stay within the VT display area [0, viewport dimensions). 5. Disable SGR-Pixel mode in direct conhost (no ConPTY) since the Win32 console input API only provides cell coordinates. Add ITerminalApi::IsConhost() to detect this case. DECRQM reports PermanentlyDisabled for mode 1016 in direct conhost. Remove the fake pixel coordinate approximation from inputBuffer.cpp. 6. Update tests to use the float API and verify virtual pixel math (e.g., center of cell (0,0) at float pos (0.5, 0.5) produces virtual pixel output (6, 11) in 1-based SGR format). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hey thanks for the valuable feedback @lhecker and @j4james. All these coordinate transformations do my head in, so I did had to ask my buddy Claude for help again. There is also still the problem that the only build/dev box I can build this on is still broken. Is it possible to have those CI builds enabled?. Also one thing I was not sure about is the clamping to (0, width-1) etc, seems to be a bit hairy with floats, maybe should it use IEEERemainder instead? |
| // SGR-Pixel mode requires real sub-cell pixel coordinates. It's not | ||
| // supported in direct conhost because the Win32 console input API only | ||
| // provides character-cell coordinates. In ConPTY mode (IsVtInputEnabled), | ||
| // the request is passed through to the Terminal frontend which has pixel data. | ||
| if (!_api.IsConhost() || _api.IsVtInputEnabled()) |
There was a problem hiding this comment.
It should be relatively easy to add conhost support in this PR actually, even if we have to refactor some code to achieve this. conhost is not limited by what the console API supports, after all - those are separate concerns (internal VS external API).
| return _getTerminalInput().HandleMouse(viewportPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state); | ||
| const auto viewport = _GetMutableViewport().ToOrigin(); | ||
| viewportX = std::clamp(viewportX, 0.0f, static_cast<float>(viewport.Width() - 1)); | ||
| viewportY = std::clamp(viewportY, 0.0f, static_cast<float>(viewport.Height() - 1)); |
There was a problem hiding this comment.
I believe this is wrong. Let's say the viewport is 1*1 cell large. This code would clamp the float value to between 0 and 0, permitting no fractional positions within that one cell.
Additionally, the clamping isn't quite identical to what it was before. The ToOrigin().Clamp() would essentially intersect the viewport-relative mouse coordinates with the scrollbar-relative buffer viewport. Thus, the coordinates would get correctly clamped.
When I'm writing this, I realize, however, that the old code seemingly doesn't shift the cursor-coordinates according to the scroll offset (= when the user has scrolled slightly upward, clicking into the partially visible VT viewport should translate the mouse coordinates back down the scroll offset). I think I'll have to check out this PR locally.
There was a problem hiding this comment.
w.r.t to scrolling and just to clarify the coordinate system: What this whole change really means in terms of VT command output:
Cell size: 10x20
SGR 1006 reports mouse at 7,11 (row, col)
SGR 1016 would report something like 75,230 (pixels), in case when the mouse pointer is exactly in the centre, correct?
This is what my small test program spits out at least: https://gist.github.com/sebgod/9b9a38a7d934907584978c9bafdd0489
Summary of the Pull Request
Add support for SGR-Pixels mouse mode (DECSET 1016), which reports mouse
events using pixel coordinates instead of character-cell coordinates. This
uses the same CSI sequence format as SGR mode (1006) but with pixel
positions, enabling sub-cell mouse precision for applications that need it
(e.g., sixel-aware programs, smooth scrolling in TUI frameworks like
Textual).
References and Relevant Issues
Detailed Description of the Pull Request / Additional comments
What is SGR-Pixels (mode 1016)?
SGR-Pixels is an extension of the SGR extended mouse mode (1006). It uses
the identical sequence format:
The only difference is that
xandyare pixel coordinates (1-based,relative to the top-left of the text area) instead of character-cell
coordinates. This allows applications to detect mouse positions within
individual cells, which is useful for:
Implementation details
New mode constant:
SGR_PIXEL_MODE = DECPrivateMode(1016)added toDispatchTypes.hpp.New encoding mode:
SgrPixelMouseEncodingadded toTerminalInput::Modeenum. It is mutually exclusive withUtf8MouseEncoding(1005) and
SgrMouseEncoding(1006) — enabling one disables the others.Encoding logic: When SGR-Pixel mode is active,
HandleMousepasses theraw pixel position (instead of the cell position) to
_GenerateSGRSequence.The sequence generator is reused as-is since the format is identical; only
the coordinate source differs.
Pixel coordinate pipeline: A
pixelPositionparameter was added toHandleMouseand propagated through the calling chain:ControlInteractivity(WinUI)HwndTerminal(Win32)lParamInputBuffer(conhost)The conhost path approximates pixel coordinates from cell positions since the
Win32 console input API only provides character-cell coordinates. This gives
cell-boundary-aligned pixel values, which is the best available without
refactoring the conhost input pipeline.
DECRPM support: Mode 1016 is queryable via
DECRPM(CSI ? 1016 $ p),reporting enabled/disabled status.
Validation Steps Performed
SgrPixelModeTeststoMouseInputTest.cppcovering:implementations
PR Checklist