From a2c8674252743c451600f53c77df3c7fac6e7786 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Fri, 6 Oct 2017 15:06:34 -0500 Subject: SCI32: Clean up GfxText32 * Rewrap comments to 80 columns * Clarify comments where possible --- engines/sci/graphics/text32.cpp | 147 ++++++++++++++++++++++------------------ engines/sci/graphics/text32.h | 98 ++++++++++++--------------- 2 files changed, 124 insertions(+), 121 deletions(-) (limited to 'engines/sci') diff --git a/engines/sci/graphics/text32.cpp b/engines/sci/graphics/text32.cpp index 7399fe0268..1fcf2920cc 100644 --- a/engines/sci/graphics/text32.cpp +++ b/engines/sci/graphics/text32.cpp @@ -44,7 +44,7 @@ int16 GfxText32::_yResolution = 0; GfxText32::GfxText32(SegManager *segMan, GfxCache *fonts) : _segMan(segMan), _cache(fonts), - // Not a typo, the original engine did not initialise height, only width + // SSCI did not initialise height, so we intentionally do not do so also _width(0), _text(""), _bitmap(NULL_REG) { @@ -84,8 +84,8 @@ reg_t GfxText32::createFontBitmap(int16 width, int16 height, const Common::Rect mulinc(_textRect, scaleX, scaleY); } - // _textRect represents where text is drawn inside the - // bitmap; clipRect is the entire bitmap + // `_textRect` represents where text is drawn inside the bitmap; `clipRect` + // is the entire bitmap Common::Rect bitmapRect(_width, _height); if (_textRect.intersects(bitmapRect)) { @@ -134,9 +134,9 @@ reg_t GfxText32::createFontBitmap(const CelInfo32 &celInfo, const Common::Rect & SciBitmap &bitmap = *_segMan->allocateBitmap(&_bitmap, _width, _height, _skipColor, 0, 0, _xResolution, _yResolution, 0, false, gc); - // NOTE: The engine filled the bitmap pixels with 11 here, which is silly - // because then it just erased the bitmap using the skip color. So we don't - // fill the bitmap redundantly here. + // SSCI filled the bitmap pixels with 11 here, which is silly because then + // it just erased the bitmap using the skip color. So we don't fill the + // bitmap redundantly here. _backColor = _skipColor; erase(bitmapRect, false); @@ -164,9 +164,10 @@ reg_t GfxText32::createFontBitmap(const CelInfo32 &celInfo, const Common::Rect & } void GfxText32::setFont(const GuiResourceId fontId) { - // NOTE: In SCI engine this calls FontMgr::BuildFontTable and then a font - // table is built on the FontMgr directly; instead, because we already have - // font resources, this code just grabs a font out of GfxCache. + // In SSCI, this calls FontMgr::BuildFontTable, and then a font table is + // built on the FontMgr directly; instead, because we already have GfxFont + // resources from SCI16 and those resources did not change in SCI32, this + // code just grabs those out of GfxCache if (fontId != _fontId) { _fontId = fontId; _font = _cache->getFont(_fontId); @@ -179,7 +180,7 @@ void GfxText32::drawFrame(const Common::Rect &rect, const int16 size, const uint SciBitmap &bitmap = *_segMan->lookupBitmap(_bitmap); byte *pixels = bitmap.getPixels() + rect.top * _width + rect.left; - // NOTE: Not fully disassembled, but this should be right + // Not fully disassembled, but appears to be correct in all cases int16 rectWidth = targetRect.width(); int16 heightRemaining = targetRect.height(); int16 sidesHeight = heightRemaining - size * 2; @@ -275,10 +276,9 @@ void GfxText32::drawTextBox(const Common::String &text) { void GfxText32::drawText(const uint index, uint length) { assert(index + length <= _text.size()); - // NOTE: This draw loop implementation is somewhat different than the - // implementation in the actual engine, but should be accurate. Primarily - // the changes revolve around eliminating some extra temporaries and - // fixing the logic to match. + // This draw loop implementation is somewhat different than the + // implementation in SSCI, but is accurate. Primarily the changes revolve + // around eliminating some extra temporaries and fixing the logic to match. const char *text = _text.c_str() + index; while (length-- > 0) { char currentChar = *text++; @@ -341,10 +341,10 @@ void GfxText32::invertRect(const reg_t bitmapId, int16 bitmapStride, const Commo SciBitmap &bitmap = *_segMan->lookupBitmap(bitmapId); - // NOTE: SCI code is super weird here; it seems to be trying to look at the - // entire size of the bitmap including the header, instead of just the pixel - // data size. We just look at the pixel size. This function generally is an - // odd duck since the stride dimension for a bitmap is built in to the bitmap + // SSCI is super weird here; it seems to be trying to look at the entire + // size of the bitmap including the header, instead of just the pixel data + // size. We just look at the pixel size. This function generally is an odd + // duck since the stride dimension for a bitmap is built in to the bitmap // header, so perhaps it was once an unheadered bitmap format and this // function was never updated to match? Or maybe they exploit the // configurable stride length somewhere else to do stair stepping inverts... @@ -355,7 +355,7 @@ void GfxText32::invertRect(const reg_t bitmapId, int16 bitmapStride, const Commo error("InvertRect too big: %u >= %u", invertSize, bitmapSize); } - // NOTE: Actual engine just added the bitmap header size hardcoded here + // SSCI just added a hardcoded bitmap header size here byte *pixel = bitmap.getPixels() + bitmapStride * targetRect.top + targetRect.left; int16 stride = bitmapStride - targetRect.width(); @@ -392,16 +392,17 @@ uint GfxText32::getLongest(uint *charIndex, const int16 width) { char currentChar; while ((currentChar = *text++) != '\0') { - // NOTE: In the original engine, the font, color, and alignment were - // reset here to their initial values + // In SSCI, the font, color, and alignment were reset here to their + // initial values; this does not seem to be necessary and really + // complicates the font system, so we do not do it // The text to render contains a line break; stop at the line break if (currentChar == '\r' || currentChar == '\n') { - // Skip the rest of the line break if it is a Windows-style - // \r\n or non-standard \n\r - // NOTE: In the original engine, the `text` pointer had not been - // advanced yet so the indexes used to access characters were - // one higher + // Skip the rest of the line break if it is a Windows-style \r\n (or + // non-standard \n\r) + + // In SSCI, the `text` pointer had not been advanced yet here, so + // the indexes used to access characters were one higher there if ( (currentChar == '\r' && text[0] == '\n') || (currentChar == '\n' && text[0] == '\r' && text[1] != '\n') @@ -418,16 +419,20 @@ uint GfxText32::getLongest(uint *charIndex, const int16 width) { } // Skip the line break and return all text seen up to now - // NOTE: In original engine, the font, color, and alignment were - // reset, then getTextWidth was called to use its side-effects to - // set font, color, and alignment according to the text from - // `initialCharIndex` to `testLength` + // In SSCI, the font, color, and alignment were reset, then + // getTextWidth was called to use its side-effects to set font, + // color, and alignment according to the text from + // `initialCharIndex` to `testLength`. This is complicated and + // apparently not necessary for correct operation, so we do not do + // it + ++*charIndex; return testLength; } else if (currentChar == ' ') { - // The last word in the line made it too wide to fit in the text area; - // return up to the previous word, then collapse the whitespace - // between that word and its next sibling word into the line break + // The last word in the line made it too wide to fit in the text + // area; return up to the previous word, then collapse the + // whitespace between that word and its next sibling word into the + // line break if (getTextWidth(initialCharIndex, testLength) > width) { *charIndex = lastWordBreakIndex; const char *nextChar = _text.c_str() + lastWordBreakIndex; @@ -435,14 +440,17 @@ uint GfxText32::getLongest(uint *charIndex, const int16 width) { ++*charIndex; } - // NOTE: In original engine, the font, color, and alignment were - // set here to the values that were seen at the last space character + // In SSCI, the font, color, and alignment were set here to the + // values that were seen at the last space character, but this + // is complicated and unnecessary so we do not do it + return length; } - // NOTE: In the original engine, the values of _fontId, _foreColor, - // and _alignment were stored for use in the return path mentioned - // just above here + // In SSCI, the values of `_fontId`, `_foreColor`, and `_alignment` + // were stored for use in the return path mentioned just above here, + // but we do not need to do this because we do not cause + // side-effects when calculating text dimensions // We found a word break that was within the text area, memorise it // and continue processing. +1 on the character index because it has @@ -456,8 +464,9 @@ uint GfxText32::getLongest(uint *charIndex, const int16 width) { ++*charIndex; ++testLength; - // NOTE: In the original engine, the font, color, and alignment were - // reset here to their initial values + // In SSCI, the font, color, and alignment were reset here to their + // initial values, but we do not need to do this because we do not cause + // side-effects when calculating text dimensions // The text to render contained no word breaks yet but is already too // wide for the text area; just split the word in half at the point @@ -471,10 +480,11 @@ uint GfxText32::getLongest(uint *charIndex, const int16 width) { // The complete text to render was a single word, or was narrower than // the text area, so return the entire line if (length == 0 || getTextWidth(initialCharIndex, testLength) <= width) { - // NOTE: In original engine, the font, color, and alignment were - // reset, then getTextWidth was called to use its side-effects to - // set font, color, and alignment according to the text from - // `initialCharIndex` to `testLength` + // In SSCI, the font, color, and alignment were reset, then getTextWidth + // was called to use its side-effects to set font, color, and alignment + // according to the text from `initialCharIndex` to `testLength`. This + // is not necessary because we do not cause side-effects when + // calculating text dimensions return testLength; } @@ -495,14 +505,13 @@ int16 GfxText32::getTextWidth(const uint index, uint length) const { while (length > 0 && currentChar != '\0') { // Control codes are in the format `||` if (currentChar == '|') { - // NOTE: Original engine code changed the global state of the - // FontMgr here upon encountering any color, alignment, or - // font control code. - // To avoid requiring all callers to manually restore these - // values on every call, we ignore control codes other than - // font change (since alignment and color do not change the - // width of characters), and simply update the font pointer - // on stack instead of the member property font. + // SSCI changed the global state of the FontMgr here upon + // encountering any color, alignment, or font control code. To avoid + // requiring all callers to manually restore these values on every + // call, we ignore control codes other than font change (since + // alignment and color do not change the width of characters), and + // simply update the font pointer on stack instead of the member + // property font to avoid these unnecessary side-effects. currentChar = *text++; --length; @@ -548,12 +557,12 @@ int16 GfxText32::getTextWidth(const Common::String &text, const uint index, cons } Common::Rect GfxText32::getTextSize(const Common::String &text, int16 maxWidth, bool doScaling) { - // NOTE: Like most of the text rendering code, this function was pretty - // weird in the original engine. The initial result rectangle was actually - // a 1x1 rectangle (0, 0, 0, 0), which was then "fixed" after the main - // text size loop finished running by subtracting 1 from the right and - // bottom edges. Like other functions in SCI32, this has been converted - // to use exclusive rects with inclusive rounding. + // Like most of the text rendering code, this function was pretty weird in + // SSCI. The initial result rectangle was actually a 1x1 rectangle + // (0, 0, 0, 0), which was then "fixed" after the main text size loop + // finished running by subtracting 1 from the right and bottom edges. Like + // other functions in SCI32, this has been converted to use exclusive rects + // with inclusive rounding. Common::Rect result; @@ -598,17 +607,18 @@ Common::Rect GfxText32::getTextSize(const Common::String &text, int16 maxWidth, if (getSciVersion() < SCI_VERSION_2_1_MIDDLE) { result.bottom = 0; } else { - // NOTE: In the original engine code, the bottom was not decremented - // by 1, which means that the rect was actually a pixel taller than - // the height of the font. This was not the case in the other branch, - // which decremented the bottom by 1 at the end of the loop. + // In SSCI, the bottom was not decremented by 1, which means that + // the rect was actually a pixel taller than the height of the font. + // This was not the case in the other branch, which decremented the + // bottom by 1 at the end of the loop. For accuracy, we do what SSCI + // did, even though this means the result is a pixel off result.bottom = _font->getHeight() + 1; } } if (doScaling) { - // NOTE: The original engine code also scaled top/left but these are - // always zero so there is no reason to do that. + // SSCI also scaled top/left but these are always zero so there is no + // reason to do that result.right = ((result.right - 1) * scriptWidth + _xResolution - 1) / _xResolution + 1; result.bottom = ((result.bottom - 1) * scriptHeight + _yResolution - 1) / _yResolution + 1; } @@ -700,7 +710,10 @@ void GfxText32::scrollLine(const Common::String &lineText, int numLines, uint8 c _foreColor = color; _alignment = align; - //int fc = _foreColor; + + // As with other font functions, SSCI saved _foreColor here so it could be + // restored after the getTextWidth call, but this call is side-effect-free + // in our implementation so this is not necessary setFont(fontId); @@ -713,8 +726,8 @@ void GfxText32::scrollLine(const Common::String &lineText, int numLines, uint8 c _drawPosition.x += _textRect.width() - textWidth; } - //_foreColor = fc; - //setFont(fontId); + // _foreColor and font were restored here in SSCI due to side-effects of + // getTextWidth which do not exist in our implementation drawText(0, lineText.size()); } diff --git a/engines/sci/graphics/text32.h b/engines/sci/graphics/text32.h index c8fafb287f..945cc81130 100644 --- a/engines/sci/graphics/text32.h +++ b/engines/sci/graphics/text32.h @@ -45,11 +45,11 @@ enum ScrollDirection { class GfxFont; /** - * This class handles text calculation and rendering for - * SCI32 games. The text calculation system in SCI32 is - * nearly the same as SCI16, which means this class behaves - * similarly. Notably, GfxText32 maintains drawing - * parameters across multiple calls. + * This class handles text calculation and rendering for SCI32 games. The text + * calculation system in SCI32 is nearly the same as SCI16, which means this + * class behaves similarly. Notably, GfxText32 maintains drawing parameters + * across multiple calls, instead of requiring all text parameters to be + * provided on every draw call. */ class GfxText32 { private: @@ -57,10 +57,10 @@ private: GfxCache *_cache; /** - * The width and height of the currently active text - * bitmap, in text-system coordinates. + * The width and height of the currently active text bitmap, in text-system + * coordinates. * - * @note These are unsigned in the actual engine. + * @note These are unsigned in SSCI. */ int16 _width, _height; @@ -75,20 +75,19 @@ private: uint8 _backColor; /** - * The transparent color of the text box. Used when - * compositing the bitmap onto the screen. + * The transparent color of the text box. Used when compositing the bitmap + * onto the screen. */ uint8 _skipColor; /** - * The rect where the text is drawn within the bitmap. - * This rect is clipped to the dimensions of the bitmap. + * The rect where the text is drawn within the bitmap. This rect is clipped + * to the dimensions of the bitmap. */ Common::Rect _textRect; /** - * The text being drawn to the currently active text - * bitmap. + * The text being drawn to the currently active text bitmap. */ Common::String _text; @@ -103,7 +102,8 @@ private: int16 _borderColor; /** - * TODO: Document + * If true, text will be drawn using a dither that draws only every other + * pixel of the text. */ bool _dimmed; @@ -123,19 +123,17 @@ private: void drawText(const uint index, uint length); /** - * Gets the length of the longest run of text available - * within the currently loaded text, starting from the - * given `charIndex` and running for up to `maxWidth` - * pixels. Returns the number of characters that can be - * written, and mutates the value pointed to by - * `charIndex` to point to the index of the next - * character to render. + * Gets the length of the longest run of text available within the currently + * loaded text, starting from the given `charIndex` and running for up to + * `maxWidth` pixels. Returns the number of characters that can be written, + * and mutates the value pointed to by `charIndex` to point to the index of + * the next character to render. */ uint getLongest(uint *charIndex, const int16 maxWidth); /** - * Gets the pixel width of a substring of the currently - * loaded text, without scaling. + * Gets the pixel width of a substring of the currently loaded text, without + * scaling. */ int16 getTextWidth(const uint index, uint length) const; @@ -163,29 +161,27 @@ public: reg_t _bitmap; /** - * The size of the x-dimension of the coordinate system - * used by the text renderer. Static since it was global in SSCI. + * The size of the x-dimension of the coordinate system used by the text + * renderer. Static since it was global in SSCI. */ static int16 _xResolution; /** - * The size of the y-dimension of the coordinate system - * used by the text renderer. Static since it was global in SSCI. + * The size of the y-dimension of the coordinate system used by the text + * renderer. Static since it was global in SSCI. */ static int16 _yResolution; /** - * The currently active font resource used to write text - * into the bitmap. + * The currently active font resource used to write text into the bitmap. * - * @note SCI engine builds the font table directly - * inside of FontMgr; we use GfxFont instead. + * @note SSCI builds the font table directly inside of FontMgr; we use + * GfxFont instead. */ GfxFont *_font; /** - * Creates a plain font bitmap with a flat color - * background. + * Creates a plain font bitmap with a flat color background. */ reg_t createFontBitmap(int16 width, int16 height, const Common::Rect &rect, const Common::String &text, const uint8 foreColor, const uint8 backColor, const uint8 skipColor, const GuiResourceId fontId, TextAlign alignment, const int16 borderColor, bool dimmed, const bool doScaling, const bool gc); @@ -212,26 +208,22 @@ public: /** * Draws the given text to the bitmap. * - * @note The original engine holds a reference to a - * shared string which lets the text be updated from - * outside of the font manager. Instead, we give this - * extra signature to send the text to draw. - * - * TODO: Use shared string instead? + * @note SSCI holds a reference to a shared string which lets the text be + * updated from outside of the font manager. Instead, we use this extra + * signature to send the text to draw. */ void drawTextBox(const Common::String &text); /** - * Erases the given rect by filling with the background - * color. + * Erases the given rect by filling with the background color. */ void erase(const Common::Rect &rect, const bool doScaling); void invertRect(const reg_t bitmap, const int16 bitmapStride, const Common::Rect &rect, const uint8 foreColor, const uint8 backColor, const bool doScaling); /** - * Sets the font to be used for rendering and - * calculation of text dimensions. + * Sets the font to be used for rendering and calculation of text + * dimensions. */ void setFont(const GuiResourceId fontId); @@ -251,8 +243,8 @@ public: Common::Rect getTextSize(const Common::String &text, const int16 maxWidth, bool doScaling); /** - * Gets the pixel width of a substring of the currently - * loaded text, with scaling. + * Gets the pixel width of a substring of the currently loaded text, with + * scaling. */ int16 getTextWidth(const Common::String &text, const uint index, const uint length); @@ -262,23 +254,21 @@ public: int16 getStringWidth(const Common::String &text); /** - * Gets the number of characters of `text`, starting - * from `index`, that can be safely rendered into - * `textRect`. + * Gets the number of characters of `text`, starting from `index`, that can + * be safely rendered into `textRect`. */ int16 getTextCount(const Common::String &text, const uint index, const Common::Rect &textRect, const bool doScaling); /** - * Gets the number of characters of `text`, starting - * from `index`, that can be safely rendered into - * `textRect` using the given font. + * Gets the number of characters of `text`, starting from `index`, that can + * be safely rendered into `textRect` using the given font. */ int16 getTextCount(const Common::String &text, const uint index, const GuiResourceId fontId, const Common::Rect &textRect, const bool doScaling); /** * Scroll up/down one line. `numLines` is the number of the lines in the - * textarea, and `textLine` contains the text to draw as the newly - * visible line. Originally FontMgr::DrawOneLine and FontMgr::UpOneLine. + * textarea, and `textLine` contains the text to draw as the newly visible + * line. Originally FontMgr::DrawOneLine and FontMgr::UpOneLine. */ void scrollLine(const Common::String &textLine, int numLines, uint8 color, TextAlign align, GuiResourceId fontId, ScrollDirection dir); }; -- cgit v1.2.3