Hard Light Productions Forums
Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: pedro on November 08, 2009, 06:59:00 am
-
Hello everybody.
karajorma told me to fix this bug http://scp.indiegames.us/mantis/view.php?id=1992 (http://scp.indiegames.us/mantis/view.php?id=1992). I still don't have a working mantis account :((BTW: can someone assign a random password to pedro mantis account ?), that's why I'm posting here a note in order to share my correction proposal.
The color bug has two causes:
1 - brief_color_text_init terminates every lines by a {letter=0 color=0 } colored_char. It is useless to add a strign termination character because the SCP_vector stores the string length. In addition it makes SCP_vector.size() = string length +1 which is not intuitive.
2 - brief_render_line draws briefing lines made of character sequences separated by spaces with the color of the last character of the sequence. FUBAR-BDHR notes shows every line are terminated by a {letter=0 color=0 } colored_char. This char is part of the character sequence because it is not a whitespace (whitespace is char 32, not 0), as a consequence the color 0 is used to draw the last character sequence of a line. This is surprising that this method tries to detect words because the coloring information have already been built by brief_color_text_init function. It would be more appropriate to focus on character colors.
I propose a double fix where the two fix are sufficient to solve mantis issue (http://scp.indiegames.us/mantis/view.php?id=1992):
1 - Modify brief_color_text_init to remove the extra terminal character
2 - Rewrite brief_render_line to work with character sequences delimited by color change.
Note that the current implementation of brief_color_text_init does not recognize ',.:;()' as word separators which causes the dot of "$g DELTA." string to be green whereas It could be considered to be white. I can fix it too, but it's a behavior modification, and I'm not able for now to be sure it's not going to add bugs.
Corrections are tested. I have a svn patch waiting for approval. I'm still not aware of the code commit procedure. Can someone show me the way ?
Thanks.
-
Just post the patch here and someone (probably me) will check it over and commit it to SVN crediting you for it.
Changing the behaviour to recognise ,.:; etc would probably be a good idea but I'd like to hear if anyone is going to be screwed over by such a change before I suggest that you do it.
-
Well the one problem I can think of are some of those missions where :v: forgot you couldn't use ; so there are extra lines of text in the missions.
-
Well the one problem I can think of are some of those missions where :v: forgot you couldn't use ; so there are extra lines of text in the missions.
Sorry, I didn't get you. Do you mean the ";" character has a special meaning when used in briefing text ? Have you an example of what should work and what shouldn't ?
-
And any mediavp missions having issue with it I'll be more than happy to correct or report on.
-
Well the one problem I can think of are some of those missions where :v: forgot you couldn't use ; so there are extra lines of text in the missions.
Sorry, I didn't get you. Do you mean the ";" character has a special meaning when used in briefing text ? Have you an example of what should work and what shouldn't ?
";" signifies a comment in mission files, everything past that will IIRC be ignored by the parser.
-
E is right. There are places where :v: forgot that in creating their own missions so you have things like this: (note not an actual example)
"You will be flying ship type x, however, weapons will be limited to y."
; however, weapons will be limited to y."
; however, weapons will be limited to y."
; however, weapons will be limited to y."
So if you made the ; parse correctly your briefing would then be:
"You will be flying ship type x, however, weapons will be limited to y. ; however, weapons will be limited to y.; however, weapons will be limited to y.; however, weapons will be limited to y."
While they can be fixed in the MediaVP versions running with retail data would result in messed up text in some cases.
-
Isn't there a "$Semicolon" for inserting those into text in mission?
-
Either that or something close to it. It was added awhile back (between 3.6.9 and 3.6.10 I believe) since people kept using them and occasionally it messed up missions.
-
Well, I can also place fixed versions of the retail where necessary in to MV_Core which would still allow for full Retail experience.
I know of several of the missions in question.
-
To be honest what you do with ; depends on the order these things work in. I suspect the game replaces any instance of $semicolon with a ; long before it gets to colouring the briefing text. In which case it shouldn't matter if Pedro makes the game treat ; differently from the way it does now.
-
Karajorma is true. I wandered the code, and here is what I found.
- First the mission text file is read and strip_comments function recognize several comment (including";") and then removes them.
- Then lcl_replace_stuff recognize and replace the following tokens: $callsign, $rank, $quote and $semicolon.
- And finally, brief_text_colorize scan the a string where comment have been removed, and where ";" exists because of $semicolon. In the given string there can be $f, $h.
All in all, I'm not going to break the mission parsing algorithm by changing the character colors.
I'm going to change the coloring algorithm (if you are ok) and consider the following character to be word separators:
[0;47] for a lot of chars including !"#$%&'()*+,-./
[58;64] for :;<=>?@
[91;94] for [\]^
{96} for `
[123;127] for {|}~
And any mediavp missions having issue with it I'll be more than happy to correct or report on.
Feel free to report issues in mantis and assign them to me.
-
Here are the behavior modification introduced.
I detected a minor memory access out of allocated memory, that can be fixed.
- "$g Alpha_one, and $g Delta." gives Alpha_one, and Delta." whereas it gave "Alpha_one, and Delta.".
- "Something $g" gives "Something" whereas it gave "Something $g".
- "Something $" gives "Something $" whereas it gave an unpredictable result because of the coloring depend on the char after the \0 of the given C string.
I tested it on several mission, the best would be to be able to process the complete mission list of freespace2 (any idea on how to do it ?).
A patch has been posted to the mantis.
-
:welcome:
Greetings. Apparently you've already got contacts inside the SCP... nice to see more coders here, I guess.
-
Thanks. Nice to meet you Aardwolf.
-
A patch has been posted to the mantis.
Naturally, I read this just after getting to work. I'll test this out on the MediaVP missions in a test build as soon as I get home. Hopefully, we can combine this with the ability to have coloured text in De-Briefings as well as expanding on the currently available colours.
-
Either that or something close to it. It was added awhile back (between 3.6.9 and 3.6.10 I believe) since people kept using them and occasionally it messed up missions.
It was added on 5 September 2003, actually, which would make it just before 3.5.5.
-
http://scp.indiegames.us/mantis/view.php?id=1753
Might have been added then but the work around wasn't added until March 29, 2009
-
...no, that's simply the briefing automatically replacing a ; token with a $semicolon token before it saves the file. Related, but not the same feature.
-
I really would like to parse every mission file I have extracted from the original game, in order to extract the mission briefing and compare the resulting coloring of the new code to the previous ones. I hope to detect improvements and regressions. I have several questions:
Are colored texts currently used for something else than briefing ?
I succeeded to build a new exe from "code" VC Project adding fredstubs.cpp bus mission parsing function makes model lookups that fails because I don't run enough engine startup code. It looks to be easier to parse missions files by myself.
What is the fastest way to extract colored strings?
-
IIRC, the Fiction Viewer uses the same coloring code as briefing texts.
-
It definitely does. I had to change things a while back cause the codes were borking the fiction viewer.
-
So far, this patch does as advertised on the FS2 Campaign Missions just fine.
Have there been any changes or other needs for it?
And how well can it be applied to working with debriefing texts?
-
Does the patch work on "Dr. Mina Hargrove"? Abbreviations such as Dr., Mr., Mrs., Ms., Rev., Jr., etc. should have the period in colored text.
-
The "." in "Dr. Mina Hargrove" does not color any more than the "," at the end does. And while it might be nice to colorize a significant name like that, it's not really necessary.
Additionally, the brief text could just be made to read "Doctor" instead of "Dr." Or, some sort of an escape sequence could be placed in front of "special" characters (namely, periods) that allows them to be colored. So it would then be:
"$g Dr\. $g Mina $g Hargrove" to result in Dr. Mina Hargrove instead of the current Dr. Mina Hargrove
As well, coloration on pluralized names is still needing some work. "$g Beta's" for example results in Beta's.
-
Escape character (or quote character?) is probably a better idea. Some people may not want the 's to be colorized.
-
If I colorize "Aquitaine's" I'd expect the 's to be colored.
As well, colorizing "Terran-Vasudan" gives Terran-Vasudan instead of Terran-Vasudan
Anything in () also is not colored. Example, if I were to $g the (SOC) in "As Lightning Fall".
In related news, I have an expansion for the briefing text colors. It seems the case statements are case-sensitive, so $g and $G can be two separate colors.
Here is what I have so far, comments on additional colors would be great: (white ($w) is default text color, $b, $g, $r are default colors available already)
Bright White: $W
Standard (default) White: $w
Black: $k (Black)
Standard Blue: $b
Bright Blue: $B
Standard Green: $g
Bright Green: $G
Standard Yellow: $y
Bright Yellow: $Y
Gray: $e
Silver: $E
Violet Gray: $v
Violet: $V
Standard Red: $r
Bright Red: $R
Pink: $p
Bright Pink: $P
Additive (non pre-existent colors) are: Silver, Violet, Pink and Bright Pink. The other colors were already defined in alphacolors.cpp, with the exception of Yellow/Bright Yellow which got redefined as they looked more like Lime Green. And I realize that on a black background Grey looks really dark. In game, it's very legible and distinctive from the standard text.
-
Okay, I made a bit of a testing Command Brief to fiddle with that contains a lot of the common usage variations that can be found in a briefing.
I performed testing on the following in windowed mode running at 1024x768 and requires either Testing Build based on 5755 (http://www.hard-light.net/forums/index.php?topic=67255.0) or Nightly 5758 (or later).
(additionally, changing $V, $p and $y to something else will work just as well)
$Stage Text:
XSTR("Briefing Color Test
This is a series of example $V strings, for the purpose of testing briefing colorization. This test includes $b hyphenated-words, words in $g $quotequotation$quote, as well as prefixes like $y Mr. and $y Dr. as well as pluralized forms such as $b portal's and $f Aquitane's, to name a few. Additional $quote$g quoted$quote test. Some $p (TLA) and ($p TLA) tests. A $g GTVA. Test.
Another test on $b hyphenated-words, as a secondary example.
$y Additionally: $b hyphenated-$g words as a third example.
We are also going to test $r exclusions, with $g Alpha's and $g Beta's help.
We will also be testing ending $r words.
We also need to test, when as ending $y words: quoted $g $quotewords$quote.
$b Hyphenated-Words.
And pluralized $r word's.", -1)
$end_multi_text
$Ani Filename: <NONE>
+Wave Filename: <NONE>
Here is the current form of output by the engine:
Briefing Color Test
This is a series of example strings, for the purpose of testing briefing colorization. This test includes hyphenated-words,
words in "quotation", as well as prefixes like Mr. and Dr. as well as pluralized forms such as portal's and Aquitane's, to
name a few. Additional "quoted" test. Some (TLA) and (TLA) tests. A GTVA. Test.
Another test on hyphenated-words, as a secondary example.
Additionally: hyphenated-words as a third example.
We are also going to test exclusions, with Alpha's and Beta's help.
We will also be testing ending words.
We also need to test, when as ending words: quoted "words".
Hyphenated-Words.
And pluralized word's.
Note how the first case of "hyphenated-words," is not colored? That's because it's at the end of the display line in the viewer, so it doesn't get colored like the second one does.
Interestingly enough, the third case ignores the $b and colorizes fully in green and the Additional "quoted" test is also fully in green and both of the (TLA) including the ()'s are colored.
Here is the out-put from the current proposed patch:
Briefing Color Test
This is a series of example strings, for the purpose of testing briefing colorization. This test includes hyphenated-words,
words in "quotation", as well as prefixes like Mr. and Dr. as well as pluralized forms such as portal's and Aquitane's, to
name a few. Additional "quoted" test. Some (TLA) and (TLA) tests. A GTVA. Test.
Another test on hyphenated-words, as a secondary example.
Additionally: hyphenated-words as a third example.
We are also going to test exclusions, with Alpha's and Beta's help.
We will also be testing ending words.
We also need to test, when as ending words: quoted "words".
Hyphenated-Words.
And pluralized word's.
This comes very close to almost doing it. I like that it clips the , that breaks up sentences, but it totally destroys hyphenated and quoted words and doesn't react well to Dr. or Mr. at all, even though it get's the very last word as well as words at the line wrap. Do note however that it handles the third case of hyphenated words example correctly, as well as the additional "quoted" test
Here is an example of what we are going to be aiming for:
Briefing Color Test
This is a series of example strings, for the purpose of testing briefing colorization. This test includes hyphenated-words,
words in "quotation", as well as prefixes like Mr. and Dr. as well as pluralized forms such as portal's and Aquitane's, to
name a few. Additional "quoted" test. Some (TLA) and (TLA) tests. A GTVA. Test.
Another test on hyphenated-words, as a secondary example.
Additionally: hyphenated-words as a third example.
We are also going to test exclusions, with Alpha's and Beta's help.
We will also be testing ending words.
We also need to test, when as ending words: quoted "words".
Hyphenated-Words.
And pluralized word's.
Note that while "Additionally:" is all yellow, "words:" only has "words" in yellow. And we appear to be getting schizoid with our colorization of 's.
How are we going to achieve when it does or does not manage to color a . (period) ,(comma) '(single quote)? Well, the , character could and should probably be ignored, which the current patch does. And if the .(period) is the very last item before a return it should probably be ignored, but we don't want it ignoring all other period cases that are not newlines unless we tell it to do so.
Introducing the "Non-Breaking Space" character, "$|" idea.
With which we can turn the "($p TLA)" into "($p TLA$|)" without worrying about breaking the first "$p (TLA)" case by having it exclude ( or ).
We can keep "$ Dr." and "$y Mr." coloring the period without coloring it in "$g GTVA$|. Test."
We can also use it on "$y words$|:" without breaking being able to have "$y Additionally:" colored.
It will also allow us to keep "$b portal's" and "$g Aquitaine's" but show "$Alpha$|'s" and "$g Beta$|'s" without the 's being colored.
Watch this space for either an upcoming patch, or a call for assistance.
-
$Stage Text:
XSTR("Briefing Color Test
This is a series of example $V strings, for the purpose of testing briefing colorization. This test includes $b hyphenated-words, words in $g $quotequotation$quote, as well as prefixes like $y Mr. and $y Dr. as well as pluralized forms such as $b portal's and $f Aquitane's, to name a few. Additional $quote$g quoted$|$quote test. Some $p (TLA) and ($p TLA$|) tests. A $g GTVA$|. Test.
Another test on $b hyphenated-words, as a secondary example.
$y Additionally: $b hyphenated$|-$g words as a third example.
We are also going to test $r exclusions, with $g Alpha$|'s and $g Beta$|'s help.
We will also be testing ending $r words$|.
We also need to test, when as ending $y words$|: quoted $g $quotewords$quote$|.
$b Hyphenated-Words$|.
And pluralized $r word's$|.
$end_multi_text
$Ani Filename: <NONE>
+Wave Filename: <NONE>
So, with the above Command Briefing, we have success. Sort of. Here are the actual results:
Briefing Color Test
This is a series of example strings, for the purpose of testing briefing colorization. This test includes hyphenated-words,
words in "quotation", as well as prefixes like Mr. and Dr. as well as pluralized forms such as portal's and Aquitane's, to
name a few. Additional "quoted" test. Some (TLA) and (TLA) tests. A GTVA. Test.
Another test on hyphenated-words, as a secondary example.
Additionally: hyphenated-words as a third example.
We are also going to test exclusions, with Alpha's and Beta's help.
We will also be testing ending words.
We also need to test, when as ending words: quoted "words".
Hyphenated-Words.
And pluralized word's.
Note how it chewed up the newline/returns, but only on "$|." where it was before a newline/return? See that "$g GTVA$|. Test." rendered just fine?
Due to the newline/return issue (and banging my head against it for 5 hours with Goober and #SCP IRC folks), it's determined that any case where you wish to non-colorize a period that occurs before a newline/return, you must place a space on either side (but NOT both) of the period.
In all other cases, "$|" does not require (but can have if you so wish) a space prior to the character being "escaped". Again, this ONLY affects when the next item after the "escaped" is a newline/return, which is most common with multi-line Briefings where a colorized word occurs before a period that you wish to not have colorized.
Here is a working Command Brief that shows the previous "Desired" result (additionally linked to as a mission file):
#Command Briefing
$Stage Text:
XSTR("Briefing Color Test
This is a series of example $V strings, for the purpose of testing briefing colorization. This test includes $b hyphenated-words, words in $g $quotequotation$quote, as well as prefixes like $y Mr. and $y Dr. as well as pluralized forms such as $b portal's and $f Aquitane's, to name a few. Additional $quote$g quoted$|$quote test. Some $p (TLA) and ($p TLA$|) tests. A $g GTVA$|. Test.
Another test on $b hyphenated-words, as a secondary example.
$y Additionally: $b hyphenated$|-$g words as a third example.
We are also going to test $r exclusions, with $g Alpha$|'s and $g Beta$|'s help.
We will also be testing ending $r words$| .
We also need to test, when as ending $y words$|: quoted $g $quotewords$quote$| .
$b Hyphenated-Words$| .
And pluralized $r word's$|.", -1)
$end_multi_text
$Ani Filename: <NONE>
+Wave Filename: <NONE>
I also ran this patch against Retail Missions. It incurred no breaking of the presentation aside from 2 factors:
Where a "word," was colorized and the ',' was also colored, the ',' is no longer colored.
Where a word was at the end of the XSTR or edge of briefing display and did not show that it was colored, it is now colored.
I detected a minor memory access out of allocated memory, that can be fixed.
Can you check the current implementation and tell me if that is still the case or provide that fix so we can implement this patch into Trunk then?
M1992_01022010.patch (http://zacam.ueuo.com/scp/Patches/M1992_01022010.patch)
Void.fs2 (http://zacam.ueuo.com/temp/Void.fs2)
Requires building against Trunk 5758 or later.
-
Zacam,
I checked your patch. Thanks a lot for your test case. I have a few questions.
Note how it chewed up the newline/returns, but only on "$|." where it was before a newline/return? See that "$g GTVA$|. Test." rendered just fine?
Due to the newline/return issue (and banging my head against it for 5 hours with Goober and #SCP IRC folks), it's determined that any case where you wish to non-colorize a period that occurs before a newline/return, you must place a space on either side (but NOT both) of the period.
The newline/return issue does not occur any more with the patch I submitted, does it ?
I noticed a mission briefing like STR(" something $") will cause a warning message because the final '\0' character is sent to brief_return_color_index function. Maybe could we check the remaining string size before to identify "$" as a text coloring character.
The out of memory read does no longer exists in your patch. It was part of a check that has been removed. By the way, it is no longer necessary to add a space after the '$'+code sequence, which is a minor behavior change. Is it intended ?
Finally I wonder why $| cannot be replaced by $w in briefing definition. Does $| introduce an new behavior ?
-
PM quote here:
I merged your patch with my local tree, and traced through the functions. I figured out that the problem isn't in the coloration function, but rather in split_str in parselo. Specifically, parselo will set an ignore_until_whitespace flag when it finds an ignore character (in this case, the $), so it thinks the end-of-splitted-string marker is "$|.\n" instead of "$|". I'm not sure what to do there, so I'm going to think about it. You ought to do the same.
This should explain a bit better what the newline issue is. As far as I can determine from reading that, the problem is not with the briefing colorization code at all, as it also affects the original code. As for '\0', I suppose the question is, is it really necessary?
Ideally, a malformed XSTR should be fixed and never be in a final product, so trying to make an exception handling case for that would be a lot of unnecessary effort IMO. If anybody wants or needs to use '$' as a standard character in a briefing (or debrief or Fiction Viewer) we could probably come up with something that would prevent it from being an issue in terms of colorization. But in terms of checking string size, that could work for colorization, but we don't want an accidental "$Something" turning into a case of "$S omething" and we don't want it breaking "$quote" or "$semicolon" so doing case and literal matches would be best.
And yes, the space for handling '$c' was made optional for two reasons: Reduction of casual typo's causing errors and to more explicitly handle the new case "$|" and have it more easily inline with making it a more usable "Color Escape" code. And I introduced "$|" rather than re-using (or directing folks into being able to use) "$w" for the ability of future implementation: to to allow that the default Briefing Text may not actually be white. The idea is (especially for the Fiction Viewer, but also for Brief/DeBrief) is to allow for "Block Quote" colorization (probably through use of '$<' and '$>' as an example) that will still allow for '$c' to make the text in the block a different color and '$|' to indicate to return to the block quote color.
However, it is still by and large recommended that folks continue using the by-now standard practice of colorization by using "$g Alpha" but at least now if there is a stuck spacebar out there, it won't break things. And at the current moment, '$|' only requires a space when dealing with a newline/return which is currently being looked at, but frankly, while it is annoying and I'd rather not inconvenience people with too many "methods" for how to use this, I am happy with the usability of the feature.
Thanks for the testing and the questions, if I haven't answered to your satisfaction or if anything is still not clear, let me know. It's still under going review at the moment as I guess the idea is to try and resolve the newline handling issue before committing it and I'll admit that I'm at a bit of a loss on that one.
-
But in terms of checking string size, that could work for colorization, but we don't want an accidental "$Something" turning into a case of "$S omething" and we don't want it breaking "$quote" or "$semicolon" so doing case and literal matches would be best.
I think my post was not so clear about the warning. It only occurs only for a string terminated by "$" character. (To avoid the case just check the current position against the string length) . Given that you consider the string to be invalid, it is not a bug.
By the way, $quote and $semicolon are replaced far before the briefing coloring functions, so it is not possible to beak it.
It's still under going review at the moment as I guess the idea is to try and resolve the newline handling issue before committing it and I'll admit that I'm at a bit of a loss on that one.
Can you tell me more about the remaining newline issues ? I supposed it to be resolved, and I would be glad to help.
-
See my PM quote in the above from Goober.
Essentially, the behavior is replicated in the message "Reply 28" above, where if "$|." is at the end of a line (there is a newline/return) it essentially eats the newline/return.
Reply #28 has two code snippets. The top one will example in the quote below it the truncation/ignoring/deletion of the newline/return while the code section below that shows what is to be done to prevent it. But it is not an entirely optimal solution, as it may incur some (modder) frustration of mixing how '$|' is used (and the desired method being that no spaces are necessary).
-
There's also an off-by-one error, which I forgot to mention in the PM. It manifests in what pedro said in the case of a $ being at the end of the string.
Here is my local code. It's slightly different from Zacam's patch; in addition to the off-by-one fix, it also drops the is_word_separator function:
int brief_text_colorize(char *src, int instance)
{
Assert(src);
Assert( 0<=instance && instance < (sizeof(Colored_stream)/sizeof(*Colored_stream)) );
briefing_line dest_line; //the resulting vector of colored character
ubyte active_color_index = BRIEF_TEXT_WHITE; //the current drawing color
int src_len = strlen(src);
for (int i=0; i<src_len; i++) {
// Is the character a color markup?
// text markup consists of a '$' plus a character plus an optional space
if ( (i < src_len - 1) && (src[i] == BRIEF_META_CHAR) ) {
i++; // Consume the $ character
active_color_index = brief_return_color_index(src[i]);
i++; // Consume the color identifier and focus on the white character (if any)
// Skip every whitespace until the next word is reached
while ( (i < src_len) && is_white_space(src[i]) )
i++;
// The next character is not a whitespace, let's process it as usual
// (subtract 1 because the for loop will add it again)
i--;
continue;
}
// When the word is terminated reset color to white
if (is_white_space(src[i])) {
active_color_index = BRIEF_TEXT_WHITE;
}
// Append the character to the result structure
colored_char dest;
dest.letter = src[i];
dest.color = active_color_index;
dest_line.push_back(dest);
}
Colored_stream[instance].push_back(dest_line);
return dest_line.size();
}
-
Would it not be possible to have it turn on a color until it gets to something indicating it should go back to the normal color?
I don't know what notation something like that might use, but... the current system seems kind of like you're trying to predict every possible 'correct' context for colorization, instead of just letting the user specify "start color here" / "end color here" stuff.
-
...
Hence the concept for '$<' and '$>' (or some other general equiv that can be made to work in that system). But as it currently stands, the '$c' code says "I'll color until I hit a whitespace". Then I added '$|' that says "You can stop before hitting the next character, even though it's one you would normally color, because I don't want you to color it.".
-
Coooooool.