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

Add chapter title to hovered time #93

Merged

Conversation

christoph-heinrich
Copy link
Contributor

Closes #85

@christoph-heinrich
Copy link
Contributor Author

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

@christoph-heinrich
Copy link
Contributor Author

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.

uosc.lua Outdated Show resolved Hide resolved
@natural-harmonia-gropius
Copy link
Contributor

natural-harmonia-gropius commented Aug 9, 2022

Misaligned with CJK characters.

image
image

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 9, 2022

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
Copy link
Contributor Author

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.

@natural-harmonia-gropius
Copy link
Contributor

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.
natural-harmonia-gropius@eb26f97 works for me. byte ranges got from google, coverd most cases.
image
image

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 9, 2022

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?

@natural-harmonia-gropius
Copy link
Contributor

Otherwise LGTM, please create a PR.

christoph-heinrich#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
Copy link
Contributor Author

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).

@tomasklaen
Copy link
Owner

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.

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 14, 2022

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 🙃.

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?

@tomasklaen
Copy link
Owner

tomasklaen commented Aug 15, 2022

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.

@FichteFoll
Copy link

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

@christoph-heinrich christoph-heinrich force-pushed the timeline_hover_chapter branch 2 times, most recently from c7be234 to e7a94b6 Compare August 15, 2022 21:40
@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 15, 2022

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.

@tomasklaen
Copy link
Owner

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.

@natural-harmonia-gropius
Copy link
Contributor

natural-harmonia-gropius commented Aug 16, 2022

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

@tomasklaen
Copy link
Owner

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

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 16, 2022

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.

@tomasklaen
Copy link
Owner

tomasklaen commented Aug 16, 2022

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.

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Aug 16, 2022

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.

Long titles are wrapped at a width of roughly 25 characters and
the titles are always shown in bold with a small additional spacing
between time and title.

Closes tomasklaen#85
@tomasklaen
Copy link
Owner

👍

@tomasklaen tomasklaen merged commit 81b5baa into tomasklaen:master Aug 16, 2022
@FichteFoll
Copy link

Thanks for the change, very cool.

@debugzxcv
Copy link

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)

@christoph-heinrich christoph-heinrich deleted the timeline_hover_chapter branch January 15, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show chapter name together with timestamp when hovering over seek bar
5 participants