f2e-spec
f2e-spec copied to clipboard
alibaba
Singing: Fix long lyric phrases going off-screen.
What does this PR do?
Fix a bug in my calculations that resulted in long lyrical phrases going off the screen. Now I'm accounting for their expected enlargement when being sung.
Closes Issue(s)
- Fixes #222
Motivation
Been meaning to fix this for a while but had forgotten.
More
- [ ] Added/updated documentation
Additional Notes
Related to #411; still might be worth checking in the spacing that was bothering @Tronic. To be clear, I don't really intend to change it at least for asian mode, but I haven't really tried the other mode in ages so I'm open to being convinced.
For the record, I omitted a line break from the song in order to test this.
Before:
After:
Looking good so far on 1920x1080 :)
However with VERY VERY VERY long sentences of garbage it's getting pretty small and hard to read. (Yes that's how it's currently working everywhere in the game except the screen_sing)
Instead of resizing the syllables, which would work 9/10 times, can't we just put a new line in there?
This way we keep the font readable.
With the song title too long it still blacks out at the playlist screen (while it can show the title in the song browser screen)
For the playlist screen, we'd have to call the draw function using the lyrics boolean set to true to trigger my fix.
I don't think adding a newline is feasible; at least not without rewriting the rendering from the ground up, because these calculations are done after the text is rendered.
What we might be able to do is, on top of this, implement something that breaks up phrases that are too long, in the song parser (maybe being able to configure the cut-off limit?). That might work. Still, would likely be complex owing to the timings but if we managed to make fusing duet tracks work...
For the playlist screen, we'd have to call the draw function using the lyrics boolean set to true to trigger my fix.
That's an easy fix :+1: ! The black bars also appear in other screens if the text is too long, might be worth checking all screen_XXX files and the menus
I don't think adding a newline is feasible; at least not without rewriting the rendering from the ground up, because this calculations are done after the text is rendered.
In the perfect world it'd be really nice to have this newline added (within the screen_sing) as for this pr i'm not gonna take it in account since none of the screens work that way.
Also, as you might deduce from the source-file, I arbitrarily decided to use a 2% of the screen for a margin, we could reduce that number if you want to have more usable screen.
Or, again, we also could make it configurable between certain ranges.
@Baklap4 are your examples real ones, or did you hack the file? Despite th fact that I have some song with too long lines, I doubt that a "correct" file can expect 300 words in a time laps of a few seconds. Idem for the line split. I think this already addresses 99.9% of song-files
I explicitely hacked the file yes to actually get the black bars shown (in playlist) which is the issue within #222 The song titles even if a little too long but still a 'normal' title also gives me this result.
As you say for the song-text itself this should fix 99% of the problem yeah
As for the menu items in the current master english language there are currently barely readable texts within the menu entries because it's scaled so tiny
I was playing a bit more with this just now. So, I believe I might have found one of the things that contributes most to the problem. This is, I think the text-rendering and compositing is being done ass-backwards. From what I could gather reading examples around the internet, usually what you do is:
- Set-up a cairo context with a given size.
- Set-up a pango layout and size it according to the cairo context.
- Set-up in pango all attributes and an appropriate word-wrapping method in pango, then shape the text and go back to cairo to render.
Where as, what we're doing is:
- Set-up a pango layout and a cairo context with no sizes.
- Set-up in pango all attributes and an appropriate word-wrapping method in pango, then shape the text.
- Only now we figure the size of the pango layout and make a cairo rectangle big enough for this text.
What my patch here does is essentially just shrink-to-fit the text past this stage, in order to make sure it didn't go off-screen. However, a possible side-effect of this approach is #222. What you're seeing is not a "random black bar" but rather that has been shrunk so much it's literally lost all detail.
The take-away from this? Performous was from the beginning designed with each sentence rendered fitting in a single line. Some important parts of the UI rely on this, in fact.
So, how did I approach this just now? Very simple, I set the pango layout to PANGO_WRAP_WORD and then set the pango_layout's width, as following: pango_layout_set_width(layout.get(), targetWidth * m * PANGO_SCALE * 0.96); which essentially is the entire screen minus a 2% screen-width margin on each side.
Now, some images to demonstrate because they will often speak volumes:
First, our current situation (without the above described modification but still using the patch from this PR):
Here's the song selection screen:
And the playlist screen (notice the issue described in #222):
Now, here's the same two screens after applying my initial naive approach to fixing this for good:
You can definitely see improvement, although the layout goes to hell. Worth mentioning also, some text disappeared with this (i.e. text at the bottom of the screen, including the song selection controls), the FPS text and some other things. My guess is this messed the positioning calculation but I haven't really looked into it.
You will also notice that, since the text is prevented from overflowing at the layouting stage, my earlier fix doesn't ever get triggered.
So, I think the answer lies somewhere in the middle. I still think what I did originally in this PR is worthwhile as a final safety measure, but we should be investing in fixing the errors in the layout stage. For the singing itself (one of the places where the fact it's just one line is important) I'd rather keep just my earlier fix, but we could maybe apply some version of the layout fix elsewhere? I do think there comes a point when, if people want to use broken song-files, it's not really on us to try and make them look good, anyway...
Please @performous/core-owners and @performous/contributors weigh-in on this (and hopefully help me get this on its way properly)?
I've given this a thought...
As you encountered yourself (in the second batch of screenshots) word-wrapping won't solve the case + makes it even harder to read.
if people want to use broken song-files
That's true, however most people wouldn't know if a songfile is broken or not. In fact there's plenty of txt files on usdb/ultrastar which can be considered 'faulty' yet this is a very popular source to get your text files from. Based on this i think we should tell the user their song is 'broken' with the reason 'too long lines' and log this aswell. The song however can be played. It's indeed up to them fixing the txt-file.
So, I think the answer lies somewhere in the middle
I agree, is this pango/cairo setup a onetimer? If not we might have to go through the app so we can say we want to word-wrap this screen but not the other. As you saw on the browse song screen it works over there. But within the playlist screen it doesn't work, an alternative would be to elipse if it's too long in the playlist screen (not sure if that's possible?)
different approach
Don't hate me for saying this, but would a complete overhaul be worth the effort? With an overhaul i mean using pango/cairo correctly as described in your 1.2.3 step-plan?
If we shape the text, check for overflow and reshape it if too long, then the disappearing text issue is corrected. I guess that happened because it was small text in a rectangle spanning the width of the entire screen. Not sure what the performance impacts of this would be, though.
(Obviously all that debug text will go, as that is almost certainly slowing this down considerably as well)
Still, it occurs to me that in certain places of the UI we could want text that wraps to something smaller than the entire screen.
Thoughts?
P.S. I converted this to a draft so it's more obvious that at this point I'm throwing ideas around.
I've given this a thought...
As you encountered yourself (in the second batch of screenshots) word-wrapping won't solve the case + makes it even harder to read.
if people want to use broken song-files
That's true, however most people wouldn't know if a songfile is broken or not. In fact there's plenty of txt files on usdb/ultrastar which can be considered 'faulty' yet this is a very popular source to get your text files from. Based on this i think we should tell the user their song is 'broken' with the reason 'too long lines' and log this aswell. The song however can be played. It's indeed up to them fixing the txt-file.
We should still keep my previous fix; what I mean is, if they have a line that is so long that shrinking the text to fit makes it unreadable... well damn, there's only so much we can do.
I haven't really looked into the code in layout_singer.cc (I think that's the correct file) but I do think I remember something relative to positioning being dependent on whether a line is currently being sung or not. Maybe that won't be as hard to fix as I'm making it out to be, though.
So, I think the answer lies somewhere in the middle
I agree, is this pango/cairo setup a onetimer? If not we might have to go through the app so we can say we want to word-wrap this screen but not the other. As you saw on the browse song screen it works over there. But within the playlist screen it doesn't work, an alternative would be to elipse if it's too long in the playlist screen (not sure if that's possible?)
No, this is done each time an OpenGLText instance is created; in my previous commit I've already fixed the issue of the disappearing text but as I say, not sure whether performance might take a noticeable hit from doing that?
I think we'll need to correct vertical spacing code for lists of items; I don't think that's going to be particularly hard, actually.
And yes, ellipsizing text is possible.
different approach
Don't hate me for saying this, but would a complete overhaul be worth the effort? With an overhaul i mean using pango/cairo correctly as described in your 1.2.3 step-plan?
As demonstrated by my previous commit, an overhaul might not be necessary but again, don't know what the performance cost is. Maybe doing it like I am now will impact performance noticeably but it can be done better if we overhaul the whole system?
I think we need some way to model text-containers, maybe? So we can say, "ok, you know what, do we want this wrapped? if yes, to what length?"
As this was asking for comments on the general approach, here's my .02€:
To display text on a single line, there's only so many things that can be done: trim it or scale it down.
An approach I've used in a long-forgotten database printing project was to have a small fixed number of sizes (2 or 3 for this purpose) that would be fallen back to in case of overflow, I think that this approach might be viable here as well as it'd prevent the symptom whole essays degenerating into a "black bar". The text could be rendered onto a 2%-less-than-screen single-line canvas. If that overflows (and IIRC pango would indicate that), the next smaller font size is tried. (The computational overhead of this should be rather small, especially as it can be memoized easily.)
Unpreventably there will be cases of "bad files" that have way more text than realistic on one line. The options I see for those cases are, in decreasing order of my personal preference (not because they're really getting worse but because they're needlessly complex workarounds for broken files):
- Hard-trimming (or even ellipsizing) the text on the smallest text size. The user will already from the barely-large-enough-to-read font size that's selected for the last fallback step see that there is way more text on a line than reasonable, and consider fixing the file.
- Arbitrarily splitting the line.
- Displaying the whole line but scrolling it along to always have the boundary betwen "already sung" and "next to sing" somewhere on the screen.
As this was asking for comments on the general approach, here's my .02€:
To display text on a single line, there's only so many things that can be done: trim it or scale it down.
An approach I've used in a long-forgotten database printing project was to have a small fixed number of sizes (2 or 3 for this purpose) that would be fallen back to in case of overflow, I think that this approach might be viable here as well as it'd prevent the symptom whole essays degenerating into a "black bar". The text could be rendered onto a 2%-less-than-screen single-line canvas. If that overflows (and IIRC pango would indicate that), the next smaller font size is tried. (The computational overhead of this should be rather small, especially as it can be memoized easily.)
I'm a bit loath to add too much more processing overhead to the singing screen. I was kinda thinking that since lyrics are pretty big to begin with anyway, the infinite stretch to fix would be adequate for most cases but I'll visit your idea anyway when I keep working on this. It seems to me now that actually adding extra lines of lyrics is probably not as big an issue as I originally thought. I think I'll definitely use a version of your idea for the playlist menu, though.
I noticed this for a couple of songs as I have been playing so far. What are the current plans with this feature?
I'm working on it but it's gonna take a while still go figure everything out properly.
Lot's of discussion here. The current status is that it has some conflicts. Any chance this can get rebased? And if ready undrafted so it can be (re-)reviewed
@Lord-Kamina My bet is you've already solved 90% of cases. There are a few that I can reliably reproduce with the current version. If you resolve the conflicts, packages will build and I will test it :-)
Lot's of discussion here. The current status is that it has some conflicts. Any chance this can get rebased? And if ready undrafted so it can be (re-)reviewed
Eventually but not yet, because essentially I'm doing an overhaul of the text-rendering code. Current status is a ridiculous stash that hasn't been even split into commits because I'm still trying some stuff out. And, I haven't touched it in a while so it will need lots of rebasing and conflict resolution.
I was kinda waiting for dust to settle on other PRs (such as the webserver which IMO was more important) but I guess that's never going to end so I should probably just get back to working on this.
I'm seeing this issue. Perfection is the enemy of done. I think the first fix would be better than what we have even if it's not the best possible way to do it. The rewrite can happen later.
