uosc icon indicating copy to clipboard operation
uosc copied to clipboard

Add chapter title to hovered time

Open christoph-heinrich opened this issue 1 year ago • 9 comments

Closes #85

christoph-heinrich avatar Jul 12 '22 20:07 christoph-heinrich

Chapter ranges now work too. Took me a while to wrap my head around those.

christoph-heinrich avatar Jul 14 '22 00:07 christoph-heinrich

At first I wanted to make the change as unintrusive as possible to how to original code worked, but I think the second commit is much better. If you agree I'll squash it.

christoph-heinrich avatar Jul 14 '22 11:07 christoph-heinrich

Misaligned with CJK characters.

image image

That's unavoidable with the character width estimation, happens with latin characters too. Unfortunately we don't know how wide the text really is, so the best we can do is estimate it.

christoph-heinrich avatar Aug 09 '22 15:08 christoph-heinrich

Although I do wonder... since it is possible to use this to get the exact bounds, maybe we could call that once for each string and store the result for future use. This wouldn't just be useful for hovered time and chapters, but also the width estimation in menus and I believe the title at the top also uses width estimation, so a more general solution that could be used in all those places would be useful.

I'll look into it once all my PRs that touch anything to do with width estimation have been merged, otherwise it will just lead to a bunch of conflicts.

christoph-heinrich avatar Aug 09 '22 18:08 christoph-heinrich

Although I do wonder... since it is possible to use this to get the exact bounds, maybe we could call that once for each string and store the result for future use. This wouldn't just be useful for hovered time and chapters, but also the width estimation in menus and I believe the title at the top also uses width estimation, so a more general solution that could be used in all those places would be useful.

I'll look into it once all my PRs that touch anything to do with width estimation have been merged, otherwise it will just lead to a bunch of conflicts.

That's sound diffcult. maybe better to make things simple, for now. https://github.com/Natural-Harmonia-Gropius/uosc/commit/eb26f9786cc287064fbcad94f3653b3d0b69aeeb works for me. byte ranges got from google, coverd most cases. image image

The line local char = string.sub(text, i, byte_end) is left over form debugging I assume? Otherwise LGTM, please create a PR.

Edit: Would you mind if I add that code to console.lua (and maybe other places in mpv) at some point?

christoph-heinrich avatar Aug 09 '22 19:08 christoph-heinrich

Otherwise LGTM, please create a PR.

https://github.com/christoph-heinrich/uosc/pull/1

Would you mind if I add that code to console.lua (and maybe other places in mpv) at some point?

Sure, no problem.

christoph-heinrich#1

I meant a PR for this repo, because it's not specific to this change (although it does currently depend on it, so @darsain needs to be careful when merging).

christoph-heinrich avatar Aug 10 '22 19:08 christoph-heinrich

How about do this purely in ass tags, completely agnostic to the width of the text?

Just do a bottom center alignment with \an2 so that it grows upwards from the center, smart wrapping (each line approximately equally long) with \q0, and then limit the position to be at least font_size * 10 from the edge.

There is no reason we should render chapter titles on a single line.

And if there is a chapter title rendered, the time tooltip should also inherit the same edge limits, so that it is directly below the title.

tomasklaen avatar Aug 14 '22 12:08 tomasklaen

I don't know how I can get it to wrap. I've added the \q0 but it still went outside the screen instead of wrapping. Adding clipping also didn't help. Sorry but I'm not all that good with ass tags :upside_down_face:.

Edit: If i give it a really long line it ends up wrapping eventually, but it seems like it wraps based on the display width. How can i reduce the line width?

christoph-heinrich avatar Aug 14 '22 14:08 christoph-heinrich

Damn. Was hoping \q0 would wrap when it touches the edges. I guess too much to ask for.

So now we need to wrap ourselves. Best place for that to happen is during chapters serialization. Add an additional title_wrapped and title_wrapped_length properties, where the length one is the length of the longest line in the wrapped title.

Wrapping should happen at every space closest to the 25th character in the current line.

Then in rendering we use title_wrapped_length to decide the center point limits, and use that for both chapter title, and hovered time so they stick together.

Though I'm not 100% sure how to do wrapping for asian text without breaking it 😶.

I'm going to merge the improved text width estimation PR so you can use it here.

tomasklaen avatar Aug 15 '22 07:08 tomasklaen

\q0 only works for automatically positioned subtitles, i.e. those without an explicit \pos.

FichteFoll avatar Aug 15 '22 12:08 FichteFoll

I've made an iterator out of the code from @Natural-Harmonia-Gropius and used that for the text wrapping thing. The code of the text wrapping function is kinda horrible, but at this point I'm just glad that it works reliably. Maybe I'll try making it better tomorrow. Any suggestions are welcome. text_wrap_example text_wrap_example_jap text_wrap_example_rus

I made the byte count for double width > 2 , because otherwise the russian text would look like this: text_wrap_example_rus_1

The ass_escape function is from console.lua, no Idea if I'm allowed to just take this. It's really only needed for escaping the new lines anyway, but who knows what weird chapter titles might come along sometimes.

christoph-heinrich avatar Aug 15 '22 21:08 christoph-heinrich

Looks good. The wrapping function does look a bit over-complicated, but I'm too lazy to come up with something myself :)

The only nitpick I have, I'd like an extra space between hovered time and chapter title, otherwise it looks like it's just another line of the title.

tomasklaen avatar Aug 16 '22 07:08 tomasklaen

I prefer to render time and chapters separately,The time on two sides jumpping when width of chapter in timeline smaller than hover warpper.

Hm... that's true, there will be an annoying jumping for chapters at the edges. Makes sense to decouple them then.

tomasklaen avatar Aug 16 '22 10:08 tomasklaen

Time is once again separate from the chapter title and there is some extra spacing between them.

Edit: I've looked into the word wrapping thing a bit and I don't really see a way of making it better. Everything I can think of that would make the code easier to read, would also make the algorithm less efficient (currently it passes over the string once, without any additional look ahead). Everything I find on the internet behaves sub optimally in some way. For one they all wrap based on a max length, and not based on "should be roughly this length, wrap on the closest opportunity". Also almost all of them assume constant character width (and ASCII characters usually). The algorithm from TeX is supposedly one of the best, if not the best, word wrapping algorithm, but I haven't read the paper yet. But I'm sure it will also wrap based on a max width like the others. It does avoid greedy wrapping though.

christoph-heinrich avatar Aug 16 '22 13:08 christoph-heinrich

Tried it out, and apart of hopefully the last two nitpicks below it works great!

1. It looks better when chapter title is forced bold, so just replace the optional ..bold_tag.. on it with \\b1.

2. I found a title that wraps correctly, but the shorter line is returned as text width and used to calculate limits:

Abaurin's punishment and pound of flesh

... or at least that's what I'm guessing is going on, since the upper line Abaurin's punishment and clips outside of the screen (on the right side), while the lower line pound of flesh is correctly limited and stops moving when it touches the edge.

tomasklaen avatar Aug 16 '22 16:08 tomasklaen

You're right, that string doesn't behave correctly and I already found the mistake. Was a pretty obvious one tbh, I'm surprised it never came up during testing.

I'll squash the previous attempt because since nobody said anything I think we all agree that the second approach is better.

christoph-heinrich avatar Aug 16 '22 16:08 christoph-heinrich

👍

tomasklaen avatar Aug 16 '22 16:08 tomasklaen

Thanks for the change, very cool.

FichteFoll avatar Aug 17 '22 00:08 FichteFoll

Chapter title doesn't wrap properly. pr93

test script:

mp.register_event("file-loaded", function ()
    local chapters = {
        data = { {
            -- !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~
            title = [[!"#$%&'()*+,./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~-123456-]],
            time = 0
            }
        }
    }
    mp.set_property_native("chapter-list", chapters.data)
end)

debugzxcv avatar Jan 15 '24 16:01 debugzxcv