WIP: introduce unscaled measurements for consistent gaps in a scaled editor #73

Closed
AmadeusWM wants to merge 2 commits from fix-scaled-gaps into main
AmadeusWM commented 2025-07-11 20:14:46 +02:00 (Migrated from github.com)

This PR hopes to resolve the issue mentioned in this discussion, where the editorview gaps are inconsistent as the editor is being scaled (only noticeable if scrolled down far enough).
I found that the problem was in the implementation of the DocView.computeBlockGapDeco where the "screen"-space coordinates divided by the scaleY were used to calculate the height of the gap.
To solve this issue, I introduced originalLineHeight, originalCharWidth, originalTop, originalHeight, etc. in the HeightOracle and HeightMaps, which are the unscaled sizes and coordinates of the lines (instead of the screen coordinates). These are then used to calculate the height of the gaps. This approach avoids floating-point errors that could occur when adding up the heights of the lines (which was causing the visual bug before), as it now sums the unscaled heights of each line instead.

There might be some bugs as this is still a work in progress.

This PR hopes to resolve the issue mentioned in [this discussion](https://discuss.codemirror.net/t/synchronization-issue-between-codemirror-viewport-and-a-canvas-viewport/9318), where the editorview gaps are inconsistent as the editor is being scaled (only noticeable if scrolled down far enough). I found that the problem was in the implementation of the `DocView.computeBlockGapDeco` where the "screen"-space coordinates divided by the scaleY [were used to calculate the height of the gap](https://github.com/codemirror/view/blob/014076ae6e47ab04e6cf990d0561f4c4b6b53949/src/docview.ts#L502C9-L502C95). To solve this issue, I introduced originalLineHeight, originalCharWidth, originalTop, originalHeight, etc. in the HeightOracle and HeightMaps, which are the unscaled sizes and coordinates of the lines (instead of the screen coordinates). These are then used to calculate the height of the gaps. This approach avoids floating-point errors that could occur when adding up the heights of the lines (which was causing the visual bug before), as it now sums the unscaled heights of each line instead. - [This is how scaling the editor looked before the change](https://imgur.com/a/3STKwcu) - [And here is how it looks after the change](https://imgur.com/a/AqHEUdo) There might be some bugs as this is still a work in progress.
marijnh commented 2025-07-13 12:59:43 +02:00 (Migrated from github.com)

I'm not convinced the tracking of original sizes is a good idea. These values are used for estimating, and the tiny errors produced by multiplying then re-dividing seem unlikely to be much of an issue. And the extra, noisy code needed to track them is a serious cost.

I'm not convinced the tracking of original sizes is a good idea. These values are used for estimating, and the tiny errors produced by multiplying then re-dividing seem unlikely to be much of an issue. And the extra, noisy code needed to track them *is* a serious cost.
AmadeusWM commented 2025-07-13 13:13:01 +02:00 (Migrated from github.com)

I agree that it does introduce a bunch of noisy code. However, it did resolve the issue I was having. I couldn't find another source for the scaling issue, if you have any other ideas or hints about where the issue might be coming from, I’d love to investigate further! But I'm not really sure where else I should be looking in the codebase.

I agree that it does introduce a bunch of noisy code. However, it did resolve the issue I was having. I couldn't find another source for the scaling issue, if you have any other ideas or hints about where the issue might be coming from, I’d love to investigate further! But I'm not really sure where else I should be looking in the codebase.
AmadeusWM commented 2025-07-14 02:36:56 +02:00 (Migrated from github.com)

Upon further investigation I've found that the values returned by .getBoundingClientRect().height are slightly inaccurate, which causes an increasing error when we sum a bunch of these heights together to get the measured gap height. Here are some logs that show the errors:

scaleY: 2,                  lineHeight / scaleY: 29.800003051757812, originalLineHeight: 29.8, difference: -0.0000030517578117894573
scaleY: 1.8999999301570627, lineHeight / scaleY: 29.798247780194224, originalLineHeight: 29.8, difference: 0.001752219805776889
scaleY: 1.8000000349214687, lineHeight / scaleY: 29.79630306504807,  originalLineHeight: 29.8, difference: 0.0036969349519324624
scaleY: 1.6999999650785313, lineHeight / scaleY: 29.803916197021852, originalLineHeight: 29.8, difference: -0.003916197021851531
scaleY: 1.6000000698429373, lineHeight / scaleY: 29.80208266820059,  originalLineHeight: 29.8, difference: -0.0020826682005896657

As you can see it is often off by ~0.004 pixels, which will be noticeable in scenarios with hundreds or thousands of lines of code.

So, instead of using the previous approach of mine where we keep track of all the original coordinates, it might be more interesting to calculate an accurate lineHeight using the originalHeight:

const computedStyle = getComputedStyle(this.dom!)
const computedHeight = parseFloat(computedStyle.height)
const computedPaddingTop = parseFloat(computedStyle.paddingTop)
const computedPaddingBottom = parseFloat(computedStyle.paddingBottom)
const computedBorderTop = parseFloat(computedStyle.borderTopWidth)
const computedBorderBottom = parseFloat(computedStyle.borderBottomWidth)
const originalLineHeight = computedHeight + computedPaddingTop + computedPaddingBottom + computedBorderTop + computedBorderBottom

const lineHeight = originalLineHeight * scaleY

This would replace all the noisy code I've previously written (the originalLineHeight etc. can be removed everywhere as we now have an accurate lineHeight to calculate the gap height).

Do you think this would be a better solution? If so, I can make a new pull request.

Upon further investigation I've found that the values returned by `.getBoundingClientRect().height` are slightly inaccurate, which causes an increasing error when we sum a bunch of these heights together to get the measured gap height. Here are some logs that show the errors: ``` scaleY: 2, lineHeight / scaleY: 29.800003051757812, originalLineHeight: 29.8, difference: -0.0000030517578117894573 scaleY: 1.8999999301570627, lineHeight / scaleY: 29.798247780194224, originalLineHeight: 29.8, difference: 0.001752219805776889 scaleY: 1.8000000349214687, lineHeight / scaleY: 29.79630306504807, originalLineHeight: 29.8, difference: 0.0036969349519324624 scaleY: 1.6999999650785313, lineHeight / scaleY: 29.803916197021852, originalLineHeight: 29.8, difference: -0.003916197021851531 scaleY: 1.6000000698429373, lineHeight / scaleY: 29.80208266820059, originalLineHeight: 29.8, difference: -0.0020826682005896657 ``` As you can see it is often off by ~0.004 pixels, which will be noticeable in scenarios with hundreds or thousands of lines of code. So, instead of using the previous approach of mine where we keep track of all the original coordinates, it might be more interesting to calculate an accurate lineHeight using the originalHeight: ```ts const computedStyle = getComputedStyle(this.dom!) const computedHeight = parseFloat(computedStyle.height) const computedPaddingTop = parseFloat(computedStyle.paddingTop) const computedPaddingBottom = parseFloat(computedStyle.paddingBottom) const computedBorderTop = parseFloat(computedStyle.borderTopWidth) const computedBorderBottom = parseFloat(computedStyle.borderBottomWidth) const originalLineHeight = computedHeight + computedPaddingTop + computedPaddingBottom + computedBorderTop + computedBorderBottom const lineHeight = originalLineHeight * scaleY ``` This would replace all the noisy code I've previously written (the originalLineHeight etc. can be removed everywhere as we now have an accurate lineHeight to calculate the gap height). Do you think this would be a better solution? If so, I can make a new pull request.
marijnh commented 2025-07-14 11:46:38 +02:00 (Migrated from github.com)

Again, there is no expectation for the estimated height of unrendered lines to be 100% correct. 0.004 pixels per line would be 40 pixels per 10 000 lines, which is not huge, and will also definitely occur in non-scaled scenarios when a couple of wrapped lines are estimated incorrectly or there's some styling going on.

The text staying in the precise same place when you scale the editor is not a level of accuracy I'm aiming for with this implementation. You may be able to implement something like that with careful custom code, but viewporting is complicated enough with estimated heights.

Again, there is no expectation for the estimated height of unrendered lines to be 100% correct. 0.004 pixels per line would be 40 pixels per 10 000 lines, which is not huge, and will also definitely occur in non-scaled scenarios when a couple of wrapped lines are estimated incorrectly or there's some styling going on. The text staying in the precise same place when you scale the editor is not a level of accuracy I'm aiming for with this implementation. You may be able to implement something like that with careful custom code, but viewporting is complicated enough with estimated heights.
AmadeusWM commented 2025-07-14 12:40:04 +02:00 (Migrated from github.com)

Alright, fair enough, if this is not something codemirror is aiming for I will close this PR. Thanks for your time!

Alright, fair enough, if this is not something codemirror is aiming for I will close this PR. Thanks for your time!

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
codemirror/view!73
No description provided.