Skip to content
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

mark plane transitions with hpa #2199

Closed
dankamongmen opened this issue Sep 22, 2021 · 14 comments · Fixed by #2305
Closed

mark plane transitions with hpa #2199

dankamongmen opened this issue Sep 22, 2021 · 14 comments · Fixed by #2305
Assignees
Labels
bug Something isn't working
Milestone

Comments

@dankamongmen
Copy link
Owner

running whiteout just now, i had an idea...it really doesn't matter if within a plane we have an incorrect idea of glyph widths (outside of pathological cases like on the last line, when we could cause accidental scrolling). the problem is on plane transitions within a row, like the flyer in whiteout's top left. but if we threw an hpa in on any plane transition, that can't happen. it's a good chunk of extra output, though -- maybe we could only use it following certain character classes?

this could largely overcome the problem of unpredictable character widths, though, so it's worth thinking about. taken to the extreme, this would involve a cup before every glyph. obviously we're not doing that.

@dankamongmen dankamongmen added documentation Improvements or additions to documentation enhancement New feature or request question/research Further information is requested labels Sep 22, 2021
@dankamongmen dankamongmen self-assigned this Sep 22, 2021
@dankamongmen
Copy link
Owner Author

mintty, it ought be noted, has a mode where it alerts us whenever a glyph wider than a single column is emitted. unfortunately, i don't see how it can be effectively used, as we'd essentially need to emit a character and then wait (how long?) to see whether it was wide or not.

@dankamongmen
Copy link
Owner Author

let's finally try this out and see what happens.

@dankamongmen dankamongmen added this to the 3.0.0 milestone Oct 26, 2021
@dankamongmen
Copy link
Owner Author

we can get a quick preview by simply commenting out the return 0; in goto_location(), let's see...

@dankamongmen
Copy link
Owner Author

in kitty, always dropping an hpa:

  • resolves all mess in uniblock
  • resolves the overeat in mojibake
  • completely fixes whiteout
  • has negligible effect on total notcurses-demo runtime
    succeeding rather beyond my wildest dreams. like, this at first glance fixes all outstanding width problems, with no significant cost. and that's the naive version, that issues hpa every move--we could cut it down to plane transitions, as speced here.

Alacritty has some small boundary problems in uniblock resolved.

XTerm gets much crappier whenever we're clearing sixel. here's another place where we'd really like DECERA to apply to sixel =[. but with the plane-transition change mentioned above, this goes away.

@dankamongmen dankamongmen removed documentation Improvements or additions to documentation question/research Further information is requested labels Oct 27, 2021
@dankamongmen
Copy link
Owner Author

so i think this solves a very real problem, and solves it elegantly and completely. i'm honestly surprised by just how much this improves the situation -- literally every alignment problem i was aware of has been resolved in one fell swoop.

i think it's sensible to add a stat for this, so we can track the number of "unnecessary" hpas being inserted. it might furthermore be reasonable to only insert the hpa when we've seen a possibly-wide character, though then we're back to guessing what terminals are going to do.

XTerm's [xray] does get substantially crappier, despite the change to only perform on a plane transition -- perhaps we fucked this up? hence the value of the proposed stat:

before:

             runtimeframesoutput(B)│    FPS│%r│%a│%wTheoFPS║                
══╤════════╤════════╪═══════╪═════════╪═══════╪══╪══╪══╪═══════╣                
 1intro5.95s2983.72Mi50.10046105.792xray28.97s48623.93Mi16.8006724.603eagle13.77s3687.03Mi26.7008132.62║                
══╧════════╧════════╪═══════╪═════════╪═══════╧══╧══╧══╧═══════╝                
              48.69s115234.68Mi1155 renders, 228.28ms (69.72µs min, 197.65µs avg, 5.40ms max)
1155 rasters, 47.51ms (20.53µs min, 41.14µs avg, 104.79µs max)
1155 writes, 33.58s (20.55µs min, 29.07ms avg, 160.14ms max)
34.69MiB (96B min, 30.75KiB avg, 503.88KiB max) 0 inputs
0 failed renders, 0 failed rasters, 0 refreshes, 0 input errors
RGB emits:elides: def 1180:810404 fg 208921:318784 bg 218348:1120940
Cell emits:elides: 1339288:6592488 (83.11%) 99.85% 60.41% 83.70%
Bitmap emits:elides: 14:761 (98.19%) 25.45MiB (73.37%) SuM: 0 (0.00%)

after:

             runtimeframesoutput(B)│    FPS│%r│%a│%wTheoFPS║                
══╤════════╤════════╪═══════╪═════════╪═══════╪══╪══╪══╪═══════╣                
 1intro6.72s2803.56Mi41.7005376.742xray84.63s48628.30Mi5.700995.763eagle14.70s3687.93Mi25.0008329.93║                
══╧════════╧════════╪═══════╪═════════╪═══════╧══╧══╧══╧═══════╝                
             106.05s113439.79Mi1137 renders, 239.77ms (91.40µs min, 210.88µs avg, 3.08ms max)
1137 rasters, 48.44ms (21.35µs min, 42.60µs avg, 132.83µs max)
1137 writes, 100.10s (20.68µs min, 88.03ms avg, 293.69ms max)
39.83MiB (96B min, 35.87KiB avg, 506.29KiB max) 0 inputs
0 failed renders, 0 failed rasters, 0 refreshes, 0 input errors
RGB emits:elides: def 1180:810404 fg 197140:303059 bg 211390:1100392
Cell emits:elides: 1311782:6496162 (83.20%) 99.85% 60.59% 83.89%
Bitmap emits:elides: 14:748 (98.16%) 25.45MiB (63.90%) SuM: 0 (0.00%)

so let's add the stat, and see what we can do about [xray] with sixel, but this is happening, and it's happening for 2.4.9.

@dankamongmen dankamongmen added bug Something isn't working and removed enhancement New feature or request labels Oct 27, 2021
@dankamongmen
Copy link
Owner Author

maaaaaaaaaaaaaaaaaaaaaaaaaaaybe we even make this a per-terminal thing, since some terminals have a single width for all characters?

@dankamongmen
Copy link
Owner Author

looks like we're definitely releasing too many, which would explain continued sixel slowdowns:

49G🚱^[[51G✊🏿69^[[55G🔬^[[57G🧬^[[59G🏴

@dankamongmen
Copy link
Owner Author

looks like we're definitely releasing too many, which would explain continued sixel slowdowns:

49G🚱^[[51G✊🏿69^[[55G🔬^[[57G🧬^[[59G🏴

this of course means we must verify continued effectiveness once we eliminate the unnecessary hpas.

@dankamongmen
Copy link
Owner Author

i'm so fuckin' excited about this! a major, major improvement.

@dankamongmen dankamongmen changed the title consider marking plane transitions with hpa mark plane transitions with hpa Oct 27, 2021
@dankamongmen
Copy link
Owner Author

just marking plane transitions turns out to be much less effective. in that case, it seems like the problem here must be bidi-related, and looking at the retained failures, that would indeed seem to be the case. perhaps rather than an LTR we ought emit an HPA following RTL glyphs?

@dankamongmen
Copy link
Owner Author

Dropping egc_rtl() transformations (see #2128) rather surprisingly seems to fix up the remaining issues...?

@dankamongmen
Copy link
Owner Author

As noted in #2199, it turns out that this reduced Ghpa doesn't fix the mojibake problem on Kitty. if we're not going to resolve all these issues, i'd like to cut the number of Ghpas down drastically...hrmm.

@dankamongmen
Copy link
Owner Author

for now, let's at least limit the Ghpa to terminals where it solves problems. then we maybe move to only unleashing it when we have a glyph of uncertain width.

@dankamongmen
Copy link
Owner Author

It's now restricted to kitty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant