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.

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

Char measurement #6083

Closed
wants to merge 67 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@as-cii
Member

as-cii commented Mar 24, 2015

Fixes #6055

This is still a work in progress: code is messy and specs are not passing yet. With this PR we basically wrap every character with a span so that we're able to measure it. This helps to calculate the real character's width on screen: whenever this is not possible, however, the current solution falls back to @defaultCharWidth.

We have split the TextEditorPresenter and the TextEditorComponent updates in two phases: pre-measure and post-measure. The former should calculate anything related to lines, whereas the latter performs updates which need to be based on lines measurement.

  • Write new specs to consolidate this behavior
  • Make existing specs pass
  • Remove references to scoped char width
  • Scrolling into an unmeasured line with decorations (or cursors) has issues (since we don't measure such lines on scroll anymore and updating them doesn't require any user action)
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 24, 2015

Contributor

I'm wondering if rather than storing the widths of each character at each position if it wouldn't be easier to just store a running total. So if every character were 10px wide on a line with 4 characters, it would be [10, 20, 30, 40]. Basically, rather than an array of widths it is now an array of the horizontal position of the right side of that character in absolute terms.

Contributor

nathansobo commented Mar 24, 2015

I'm wondering if rather than storing the widths of each character at each position if it wouldn't be easier to just store a running total. So if every character were 10px wide on a line with 4 characters, it would be [10, 20, 30, 40]. Basically, rather than an array of widths it is now an array of the horizontal position of the right side of that character in absolute terms.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 24, 2015

Contributor

🚀 Looking forward to collaborating on this problem with you.

Contributor

nathansobo commented Mar 24, 2015

🚀 Looking forward to collaborating on this problem with you.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 25, 2015

Member

I'm wondering if rather than storing the widths of each character at each position if it wouldn't be easier to just store a running total. So if every character were 10px wide on a line with 4 characters, it would be [10, 20, 30, 40]. Basically, rather than an array of widths it is now an array of the horizontal position of the right side of that character in absolute terms.

I was just thinking about this and it's definitely something we could do. I wonder, however, if we could make it even faster 😮 What if, while measuring lines, we stored:

  • A matrix of rows and columns, with the absolute left position of each column
  • An interval skip list (or a similar data structure) of absolute left positions (this is needed for clipping when in the middle)

If we manage to do this, then screenPositionForPixelPosition and pixelPositionForScreenPosition would basically avoid tokens iteration altogether, making finally use of the existing computation performed by measureCharactersInVisibleLines. What do you think @nathansobo? This would be huge in terms of speed! 🐎

By the way, specs are 💚 now (exception made for those referring to scoped char widths) and it would be great if you could have a second look at this. Thank you 🙇

In the meantime I'll try to put a bit of thought into a reflow-less solution, even though I feel like this PR performs quite well.

Member

as-cii commented Mar 25, 2015

I'm wondering if rather than storing the widths of each character at each position if it wouldn't be easier to just store a running total. So if every character were 10px wide on a line with 4 characters, it would be [10, 20, 30, 40]. Basically, rather than an array of widths it is now an array of the horizontal position of the right side of that character in absolute terms.

I was just thinking about this and it's definitely something we could do. I wonder, however, if we could make it even faster 😮 What if, while measuring lines, we stored:

  • A matrix of rows and columns, with the absolute left position of each column
  • An interval skip list (or a similar data structure) of absolute left positions (this is needed for clipping when in the middle)

If we manage to do this, then screenPositionForPixelPosition and pixelPositionForScreenPosition would basically avoid tokens iteration altogether, making finally use of the existing computation performed by measureCharactersInVisibleLines. What do you think @nathansobo? This would be huge in terms of speed! 🐎

By the way, specs are 💚 now (exception made for those referring to scoped char widths) and it would be great if you could have a second look at this. Thank you 🙇

In the meantime I'll try to put a bit of thought into a reflow-less solution, even though I feel like this PR performs quite well.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 25, 2015

Member

@nathansobo: tomorrow I'll write a bunch of specs for this and collect some useful metrics in order to make a more informed decision 🚀

Member

as-cii commented Mar 25, 2015

@nathansobo: tomorrow I'll write a bunch of specs for this and collect some useful metrics in order to make a more informed decision 🚀

as-cii added some commits Mar 26, 2015

Use only batched updateCursorsState
This way we prevent cursors from calculating a wrong position because of
out-of-sync lines measurement: we need to perform such updates inside
`getPostMeasureState`, where we can guarantee correct char widths.

@as-cii as-cii changed the title from WIP: Char measurement to Char measurement Mar 26, 2015

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Apr 24, 2015

Member

@nathansobo: what do you think about this change? After adding <span id="token-{id}"> as the top level HTML element for tokens, reflows have become more expensive and therefore I needed an alternative to access a token in a performant way.

Removing tokens grouping helped bringing fast reflows back, but I am not quite sure it's actually a good thing, as I assume you had some good reasons to put group tokens in the first place. Styling-wise this doesn't seem to break anything, but it would be great if you could share some insights on that 🙏

Member

as-cii commented on 092c55b Apr 24, 2015

@nathansobo: what do you think about this change? After adding <span id="token-{id}"> as the top level HTML element for tokens, reflows have become more expensive and therefore I needed an alternative to access a token in a performant way.

Removing tokens grouping helped bringing fast reflows back, but I am not quite sure it's actually a good thing, as I assume you had some good reasons to put group tokens in the first place. Styling-wise this doesn't seem to break anything, but it would be great if you could share some insights on that 🙏

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Apr 24, 2015

Contributor

We need to nest scopes in order for nested selectors to work correctly. This allows a theme to do things like .function .variable. If we don't nest, we lose this hierarchical order. Why does the addition of the id cause reflows to be slower?

Contributor

nathansobo replied Apr 24, 2015

We need to nest scopes in order for nested selectors to work correctly. This allows a theme to do things like .function .variable. If we don't nest, we lose this hierarchical order. Why does the addition of the id cause reflows to be slower?

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Apr 24, 2015

Member

@nathansobo: as discussed yesterday, I am still working to make this mergeable and this will hopefully happen by the end of this week 🔥

Right now there are still a few things I need to address. Specifically:

  1. 💚 Look into two tests that are currently failing for no apparent good reason (it should be quite straightforward, though);
  2. 🐎 Avoid computing scroll width in DisplayBuffer every single time setCharLeftForPoint is called;
  3. 🐛 Scrolling from model (i.e. editor.setScrollTop) should trigger a character measurement on newly built lines;
  4. Resolve conflicts with master

In the meantime, however, it would be great if you could start having a look at this and leave some comments wherever you feel we should change something. Code-wise this is not perfect yet and, since it has been up for a while and I experimented a lot, there may be a bunch of things we want to refine and do better.

Thanks! 🙇

Member

as-cii commented Apr 24, 2015

@nathansobo: as discussed yesterday, I am still working to make this mergeable and this will hopefully happen by the end of this week 🔥

Right now there are still a few things I need to address. Specifically:

  1. 💚 Look into two tests that are currently failing for no apparent good reason (it should be quite straightforward, though);
  2. 🐎 Avoid computing scroll width in DisplayBuffer every single time setCharLeftForPoint is called;
  3. 🐛 Scrolling from model (i.e. editor.setScrollTop) should trigger a character measurement on newly built lines;
  4. Resolve conflicts with master

In the meantime, however, it would be great if you could start having a look at this and leave some comments wherever you feel we should change something. Code-wise this is not perfect yet and, since it has been up for a while and I experimented a lot, there may be a bunch of things we want to refine and do better.

Thanks! 🙇

@getScopedCharWidths(scopeNames)[char] = width
@scopedCharacterWidthsChangeCount++
@characterWidthsChanged() unless @batchingCharacterMeasurement
setCharLeftPositionForPoint: (row, column, charWidth) ->

This comment has been minimized.

@as-cii

as-cii Apr 24, 2015

Member

I am not super happy with the name of this method. I couldn't come up with something clever and everything always felt like a mouthful. Any ideas how could we improve it? 💭

@as-cii

as-cii Apr 24, 2015

Member

I am not super happy with the name of this method. I couldn't come up with something clever and everything always felt like a mouthful. Any ideas how could we improve it? 💭

This comment has been minimized.

@nathansobo

nathansobo Apr 24, 2015

Contributor

Only a slight improvement: setLeftPixelPositionForPoint. I think emphasizing pixels is a good idea.

@nathansobo

nathansobo Apr 24, 2015

Contributor

Only a slight improvement: setLeftPixelPositionForPoint. I think emphasizing pixels is a good idea.

Merge branch 'master' into as-char-measurement
# Conflicts:
#	src/lines-component.coffee
#	src/text-editor-presenter.coffee
@asamiam

This comment has been minimized.

Show comment
Hide comment
@asamiam

asamiam Jun 3, 2015

Will this address #3821? I switched from Sublime originally b/c ligatures worked in Atom... they broke a few versions back and it would be great to have it for fonts like Hasklig and Fire Code.

asamiam commented Jun 3, 2015

Will this address #3821? I switched from Sublime originally b/c ligatures worked in Atom... they broke a few versions back and it would be great to have it for fonts like Hasklig and Fire Code.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jun 3, 2015

Contributor

Yeah, it should fix ligatures. We need to try something more efficient than the current state of this PR because it has a bit too much overhead.

Contributor

nathansobo commented Jun 3, 2015

Yeah, it should fix ligatures. We need to try something more efficient than the current state of this PR because it has a bit too much overhead.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jun 3, 2015

Contributor

Wanted to provide a status update on this because implementing correct character measurement is becoming a bottleneck for other efforts, such as CJK-aware softwrap. @as-cii feel free to chime in and fill in any details I may be missing.

@as-cii and I had a screen sharing session about this PR a few weeks back, and concluded that the measurement overheads for the current state of this PR are probably too high. Our overall goal is to be able to compute the horizontal pixel position of a column at any time without reading from the document (which causes reflows).

  • The existing solution on master caches the width of each character in each scope whenever the DOM is updated, allowing us to compute the horizontal location of a column by summing up characters and reading out of this cache. Unfortunately, this approach is broken with ligatures and doesn't work when subpixel font scaling is enabled.
  • The solution on this PR is to use the canvas API to measure text. The advantage to measuring via canvas is that we don't interact with the document at all, but unfortunately, the canvas text measurement API seems to be too slow for our purposes, requiring every prefix of the line to be measured in a separate call.
  • The idea @as-cii and I explored on our call is to instead perform measurement in a separate document, via an iframe. We would need to sample styling information for every unique scope in the document via dummy DOM nodes, similar to @abe33's approach for the minimap. Of course all we're interested in is font styling, since that's the only thing that affects measurement. Then, to measure the position of all columns on a line, we would construct a simpler version of that line in a different document with the same styling applied inline, then use the DOM range API to iterate over the characters and measure. The advantage of this is that we only need to perform layout once.

Will this new approach work? Not sure. But it's the next thing we'd like to try. Other ideas are welcome, but remember that it can't rely on reading from the document.

Contributor

nathansobo commented Jun 3, 2015

Wanted to provide a status update on this because implementing correct character measurement is becoming a bottleneck for other efforts, such as CJK-aware softwrap. @as-cii feel free to chime in and fill in any details I may be missing.

@as-cii and I had a screen sharing session about this PR a few weeks back, and concluded that the measurement overheads for the current state of this PR are probably too high. Our overall goal is to be able to compute the horizontal pixel position of a column at any time without reading from the document (which causes reflows).

  • The existing solution on master caches the width of each character in each scope whenever the DOM is updated, allowing us to compute the horizontal location of a column by summing up characters and reading out of this cache. Unfortunately, this approach is broken with ligatures and doesn't work when subpixel font scaling is enabled.
  • The solution on this PR is to use the canvas API to measure text. The advantage to measuring via canvas is that we don't interact with the document at all, but unfortunately, the canvas text measurement API seems to be too slow for our purposes, requiring every prefix of the line to be measured in a separate call.
  • The idea @as-cii and I explored on our call is to instead perform measurement in a separate document, via an iframe. We would need to sample styling information for every unique scope in the document via dummy DOM nodes, similar to @abe33's approach for the minimap. Of course all we're interested in is font styling, since that's the only thing that affects measurement. Then, to measure the position of all columns on a line, we would construct a simpler version of that line in a different document with the same styling applied inline, then use the DOM range API to iterate over the characters and measure. The advantage of this is that we only need to perform layout once.

Will this new approach work? Not sure. But it's the next thing we'd like to try. Other ideas are welcome, but remember that it can't rely on reading from the document.

@nathansobo nathansobo referenced this pull request Jun 3, 2015

Closed

Soft wrap for CJK #14

@superlou

This comment has been minimized.

Show comment
Hide comment
@superlou

superlou Jun 4, 2015

@nathansobo: Will this approach allow positioning on lines that have margins or padding, or allow some other means of changing softwrap boundaries throughout the text editor?

superlou commented Jun 4, 2015

@nathansobo: Will this approach allow positioning on lines that have margins or padding, or allow some other means of changing softwrap boundaries throughout the text editor?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jun 5, 2015

Contributor

@superlou I'm not sure how much CSS applied to lines it's going to be realistic to support. Any supported styling will need to be sampled for measurement in the other document, and covering every possible thing people may want to style positioning wise could end up being pretty complicated. So we can try, but we may need to stick to CSS for styling fonts and colors and add other editor-specific mechanisms for controlling soft wrap behavior etc. We'll see how it goes.

Contributor

nathansobo commented Jun 5, 2015

@superlou I'm not sure how much CSS applied to lines it's going to be realistic to support. Any supported styling will need to be sampled for measurement in the other document, and covering every possible thing people may want to style positioning wise could end up being pretty complicated. So we can try, but we may need to stick to CSS for styling fonts and colors and add other editor-specific mechanisms for controlling soft wrap behavior etc. We'll see how it goes.

@larsenwork

This comment has been minimized.

Show comment
Hide comment
@larsenwork

larsenwork Jun 8, 2015

Not sure if it's related to this but I'm having issues with opentype calt stopped working

larsenwork commented Jun 8, 2015

Not sure if it's related to this but I'm having issues with opentype calt stopped working

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jun 8, 2015

Contributor

@larsenwork Nothing has really changed in this area for a while, so I'd say it's unlikely. You can still open another issue however.

Contributor

nathansobo commented Jun 8, 2015

@larsenwork Nothing has really changed in this area for a while, so I'd say it's unlikely. You can still open another issue however.

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Sep 10, 2015

Is this still a thing?

alexandernst commented Sep 10, 2015

Is this still a thing?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Sep 11, 2015

Contributor

@alexandernst Yep. It's still happening. @as-cii has been churning through a zillion alternatives looking for the best approach. It's looking increasingly like just eating a small forced reflow might be the best approach. We're getting there.

Contributor

nathansobo commented Sep 11, 2015

@alexandernst Yep. It's still happening. @as-cii has been churning through a zillion alternatives looking for the best approach. It's looking increasingly like just eating a small forced reflow might be the best approach. We're getting there.

@as-cii as-cii referenced this pull request Sep 16, 2015

Merged

DOM-based measurements #8811

11 of 11 tasks complete

@as-cii as-cii deleted the as-char-measurement branch Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment