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

BLADERUNNER: Adding Subtitles support #1219

Merged
merged 16 commits into from Dec 25, 2018

Conversation

Projects
None yet
4 participants
@antoniou79
Copy link
Contributor

antoniou79 commented Jun 18, 2018

Attaching the external resource files for the subtitles and the optional font. These should be extracted and copied in the game's installation folder.
Edit (Sep 02 2018): the new code supports (and needs) a SUBTITLES.MIX file where all relevant resources are packed. Better subtitles font included in the update and updated transcript.
SUBTITLES-02-09-2018-B.zip

@sev-
Copy link
Member

sev- left a comment

I love the idea and kudos for the amount of work which went into it.

I have several questions and remarks, though:

  1. What is the advantage of using TRE format for subtitles?
  2. The disadvantage of using the FON instead of TTF is that there will be problems with supporting non-Latin subtitles. Also, it adds tons of extra code.
  3. What is the purpose of making the whole thing switchable at compile time, not just at run-time?
  4. Asytle was too aggressive and did the number of not good changes. Those need to be reviewed and reverted. I would altogether squash two commits, and remove non-funcitonal changes. Those could be applied separately, and currently, it makes it difficult to review.
  5. You are using some sophisticated algorithm for word wrapping. I suggest taking a look at our wordWrap.
  6. A shortcut. Instead of keeping the list of videos in code, you could just check for the file presence. The check is cheap, and simplifies code and makes it more universal.
Show resolved Hide resolved engines/bladerunner/actor.cpp Outdated
Show resolved Hide resolved engines/bladerunner/bladerunner.cpp Outdated
Show resolved Hide resolved engines/bladerunner/bladerunner.cpp Outdated
Show resolved Hide resolved engines/bladerunner/bladerunner.cpp Outdated
Show resolved Hide resolved engines/bladerunner/bladerunner.h Outdated
Show resolved Hide resolved engines/bladerunner/font.cpp Outdated
Show resolved Hide resolved engines/bladerunner/font.cpp Outdated

/**
* Calculate the position (X axis - horizontal) where the current active subtitle text should be displayed/drawn
* This also determines if more than one lines should be drawn and what text goes into each line; splitting into multiple lines is done here

This comment has been minimized.

Copy link
@sev-

sev- Jun 18, 2018

Member

Why not calculate the bounding box and then just use wordWrap() function plus aligned text rendering here? This could simplify things big time.

This comment has been minimized.

Copy link
@antoniou79

antoniou79 Jun 19, 2018

Author Contributor

I wasn't aware of the wordWrap method. Still, the purpose of this code here is to allow subtitle quotes to be split in multiple lines first by checking for new line characters, and then by doing the wrapping thing. However, the wrapping code also makes sure that the line segments are approximately even (or close to that) with regard to their width. Is this something I can do with wordWrap()?

This comment has been minimized.

Copy link
@sev-

sev- Jun 23, 2018

Member

This is precisely what the wordWrap method is doing: First, split by newlines, then split the rest so it fits the specified width. It takes into account proportional fonts.

If you're thinking about evenness feature, which is neat, I would recommend enhancing wordWrap method, so other engines could benefit from it. If you need help with that, please chime in.

Show resolved Hide resolved engines/bladerunner/ui/vk.cpp Outdated
Show resolved Hide resolved engines/bladerunner/ui/vk.cpp Outdated
Show resolved Hide resolved engines/bladerunner/bladerunner.h Outdated
Show resolved Hide resolved engines/bladerunner/font.cpp Outdated
Show resolved Hide resolved engines/bladerunner/ui/vk.cpp Outdated
Show resolved Hide resolved engines/bladerunner/actor.cpp
Show resolved Hide resolved engines/bladerunner/font.cpp Outdated
Show resolved Hide resolved engines/bladerunner/font.cpp Outdated
Show resolved Hide resolved engines/bladerunner/font.h Outdated
@antoniou79

This comment has been minimized.

Copy link
Contributor Author

antoniou79 commented Jun 19, 2018

@SEV.

  1. What is the advantage of using TRE format for subtitles?

TRE is the native format for text resources that the game uses. This meant I could mostly reuse the existing implemented mechanism to access and display the text in-game. Also, by implementing access to external TRE files, it possible to easily expand this and override internal TRE files, for purposes of localization in other languages.

  1. The disadvantage of using the FON instead of TTF is that there will be problems with supporting non-Latin subtitles. Also, it adds tons of extra code.

The FON format has similar advantages - it is native to the game, and external FON files can support localizations of the game. You can use the FON file for non-latin subtitles (I can do Greek subtitles for example) in a similar fashion that this is done for the LucasArts games (Monkey island, Full throttle).
The functions to access the font glyphs properties and show them were already implemented.
I'm not sure where it adds tons of extra code. Is it for the dev tools needed to create FON files? I have written a tool for this (and also for converting subtitles to TRE format) in python and I'll be sharing on github.

  1. What is the purpose of making the whole thing switchable at compile time, not just at run-time?

No real purpose. Just me trying to keep my code isolated and quickly locate changes for a specific feature. This can be removed.

  1. Asytle was too aggressive (...)

I agree and sorry about that. I'm not sure how I can go about squashing commits, but I'll ask for help in the IRC chat.

  1. You are using some sophisticated algorithm for word wrapping. I suggest taking a look at our wordWrap.

It's a custom algorithm. The purpose is to prioritize new line characters in the quote as split points, and if such don't exist then try to split the quote in as even parts as possible without overflowing out of the screen width. I could use wordWrap, but would it substitute this functionality?

  1. A shortcut. Instead of keeping the list of videos in code, you could just check for the file presence. The check is cheap, and simplifies code and makes it more universal.

The list of the videos is kept mainly so that I can map them to an uint ID which is then used to index their respective subtitles file. It also makes it easier to pre-load all the available subtitles at startup, and then access them from memory.

Show resolved Hide resolved engines/bladerunner/subtitles.cpp Outdated
@sev-

This comment has been minimized.

Copy link
Member

sev- commented Jun 23, 2018

@SEV.

| What is the advantage of using TRE format for subtitles?
TRE is the native format for text resources that the game uses. This meant I could mostly reuse the existing implemented mechanism to access and display the text in-game. Also, by implementing access to external TRE files, it possible to easily expand this and override internal TRE files, for purposes of localization in other languages.

What I see is that you're not using a oneliner to read the subtitles, but have a full blown parser in TextResource::openFromStream(). I see, that you could alter this code very slightly, but get support of the plain text subtitles, which will be easier to maintain, translate, etc.

E.g. I understand the engineering beauty of your solution "I am using the game formats", but then, you anyway have to write code around it, and making the maintenance harder, thus, voiding this beauty.

I would highly recommend to switch to plaintext.

| The disadvantage of using the FON instead of TTF is that there will be problems with supporting non-Latin subtitles. Also, it adds tons of extra code.
The FON format has similar advantages - it is native to the game, and external FON files can support localizations of the game. You can use the FON file for non-latin subtitles (I can do Greek subtitles for example) in a similar fashion that this is done for the LucasArts games (Monkey island, Full throttle).
The functions to access the font glyphs properties and show them were already implemented.
I'm not sure where it adds tons of extra code. Is it for the dev tools needed to create FON files? I have written a tool for this (and also for converting subtitles to TRE format) in python and I'll be sharing on github.

Right. Supposedly I dreamed this off. Part of my brain remembers glyph widths manipulation. This must be your word wrapping code.

Another complication which you have is that #ifdef'fed code about external font file. I recommend to simplify that, e.g. either make it run-time, or leave only one branch.

As of the tools, please, always have those as part of the distribution. The good place is devtools/bladerunner directory.

| What is the purpose of making the whole thing switchable at compile time, not just at run-time?
No real purpose. Just me trying to keep my code isolated and quickly locate changes for a specific feature. This can be removed.

Yes, please simplify that as denoted earlier.

| Asytle was too aggressive (...)
I agree and sorry about that. I'm not sure how I can go about squashing commits, but I'll ask for help in the IRC chat.

| You are using some sophisticated algorithm for word wrapping. I suggest taking a look at our wordWrap.
It's a custom algorithm. The purpose is to prioritize new line characters in the quote as split points, and if such don't exist then try to split the quote in as even parts as possible without overflowing out of the screen width. I could use wordWrap, but would it substitute this functionality?

Yes, wordWrap is doing this at the moment, however, you mentioned that you also have algo for making strings look even, and that would require enhancing the wordWrap code. I prefer to enhance the existing code (and make it as a parameter), thatn duplicating the functionality.

| A shortcut. Instead of keeping the list of videos in code, you could just check for the file presence. The check is cheap, and simplifies code and makes it more universal.
The list of the videos is kept mainly so that I can map them to an uint ID which is then used to index their respective subtitles file. It also makes it easier to pre-load all the available subtitles at startup, and then access them from memory.

I misread your code. I thought that you do have a string ID, but you have only integer one. Your code makes sense now.

@peterkohaut

This comment has been minimized.

Copy link
Contributor

peterkohaut commented Jun 23, 2018

I think you can remove lot of "external" code if you add all to a new mix file and load that file in same way how others are loaded. There are few tools on internet which can do that.

@antoniou79 antoniou79 force-pushed the antoniou79:BladeRunnerSubtitlesSupport branch from 413dd52 to 5dfc687 Jul 2, 2018

@antoniou79

This comment has been minimized.

Copy link
Contributor Author

antoniou79 commented Aug 1, 2018

I've packed all TRE resource files and the external optional font in a SUBTITLES.MIX file (now attached in my original comment above). Like peterkohaut suggested this allowed removal of all the extra code for accessing external files (text resources and font files) which also was duplicating some of the existing functionality of the engine methods.
I looked into how to merge with ScummVM's wordwrap algorithm, which is itself simple enough and straightforward, but so far I haven't come up with an efficient way to do what I'm doing using (or extending) the existing wordwrap. That's because my text wrap code in calculatePosition assumes access to the width (in pixels) of the font glyphs and takes that into consideration as well as the boundaries (in pixels) which is basically the screen width.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Aug 2, 2018

Font::getCharWidth() gives you width of a character, and second parameter to wordWrapText() is your bounding box width.

@antoniou79 antoniou79 force-pushed the antoniou79:BladeRunnerSubtitlesSupport branch from 4644104 to 16b9746 Aug 19, 2018

@peterkohaut

This comment has been minimized.

Copy link
Contributor

peterkohaut commented Nov 24, 2018

How is this PR going? I tried it out and the subtitles are awesome!

@antoniou79

This comment has been minimized.

Copy link
Contributor Author

antoniou79 commented Dec 3, 2018

@peterkohaut as far as the transcribing goes, I've completed one and a half passes of it. It's fairly ok, but I do want to complete the second pass which is done as I'm progressing with my let's play project (currently beginning of Act 4), double-checking and verifying the lines. Once in a while, I spot quotes where corrections or improvements are needed. There are also a few cases where I'm not entirely certain of what is actually said by the actors and those will probably need the help of someone for whom English is the native language.

The transcribed parts (in-game and movie cutscenes), as well as the fonts for the subtitles are now all packed in the SUBTITLES.MIX file, so that's all the external files anyone needs to enable subtitles.

The subtitles text is still in the game's TRE format and the fonts in the game's FON format using the BladeRunner engine's own existing classes and structures.

I do see the benefit of eventually supporting (somehow) the common .srt format and utf-8 encoding, as well as using ScummVM's own Font class (and thus the wordWrapText() method) , but I don't think I'm up for doing that in this pull request, because it seems like that would negate almost all the work I've done so far -- and moreover I failed to come up with an elegant way (that I would be happy with) to do it., whereas I'm quite happy with my implementation as is now.

If the above is a hard requirement, then probably someone else will have to do it. If it's not, then please could you review the recent state of the PR and point out the outstanding issues with it so that I could focus on resolving them?

@antoniou79 antoniou79 force-pushed the antoniou79:BladeRunnerSubtitlesSupport branch from a5305ba to 117a1d5 Dec 11, 2018

Show resolved Hide resolved engines/bladerunner/font.cpp Outdated
Show resolved Hide resolved engines/bladerunner/font.h Outdated
Show resolved Hide resolved engines/bladerunner/text_resource.h Outdated
Show resolved Hide resolved engines/bladerunner/ui/kia_section_settings.h Outdated
@@ -192,6 +233,18 @@ void Font::drawCharacter(const uint8 character, Graphics::Surface &surface, int

int endY = height + y - 1;
int currentY = y;

// FIXME/TODO

This comment has been minimized.

Copy link
@peterkohaut

peterkohaut Dec 13, 2018

Contributor

is it still needed with a custom font?

This comment has been minimized.

Copy link
@antoniou79

antoniou79 Dec 14, 2018

Author Contributor

Not really needed. But it's a safety check; it won't really affect the external (custom) font, and it will prevent crashes with the internal font (mostly when debugging stuff with that font I guess).

Show resolved Hide resolved engines/bladerunner/subtitles.h Outdated
@peterkohaut
Copy link
Contributor

peterkohaut left a comment

Looks good! There are some small things which need polishing, but for me it is good enough to merge

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Dec 21, 2018

As we discussed, could you please add the MIX and FNT file generators to devtools/. Also, this needs to be rebased to the current master, and all merge commits removed. Currently, it does not even merge cleanly.

@antoniou79 antoniou79 force-pushed the antoniou79:BladeRunnerSubtitlesSupport branch from 999e6b5 to 8574aa3 Dec 23, 2018

@@ -0,0 +1,2 @@

MODULE := devtools/blade_runner/subtitles

This comment has been minimized.

Copy link
@sev-

sev- Dec 25, 2018

Member

It looks that the building rules are missing here. E.g. we would need to have to demonstrate how to build it.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Dec 25, 2018

Great! Let's merge it. What I'd like to see eventually is the automated way to build the font and subtitle files.

@sev- sev- merged commit b6e9368 into scummvm:master Dec 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.