From 0ac5d84062e430dff0f102788015dbb609fcdaa0 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Thu, 5 Oct 2017 21:31:29 -0500 Subject: SCI32: Clean up ScreenItem * Rewrap comments to 80 columns * Clarify comments where possible * Use smart pointers where appropriate --- engines/sci/engine/object.h | 1 - engines/sci/graphics/plane32.cpp | 5 +- engines/sci/graphics/screen_item32.cpp | 82 ++++++---------- engines/sci/graphics/screen_item32.h | 173 +++++++++++++++------------------ 4 files changed, 109 insertions(+), 152 deletions(-) (limited to 'engines/sci') diff --git a/engines/sci/engine/object.h b/engines/sci/engine/object.h index ffc4ac2f3d..9ab6ca34da 100644 --- a/engines/sci/engine/object.h +++ b/engines/sci/engine/object.h @@ -169,7 +169,6 @@ public: } } - // NOTE: In real engine, -info- is treated as byte size void clearInfoSelectorFlag(infoSelectorFlags flag) { if (getSciVersion() == SCI_VERSION_3) { _infoSelectorSci3 &= ~flag; diff --git a/engines/sci/graphics/plane32.cpp b/engines/sci/graphics/plane32.cpp index d9e0d2d9ff..08708f2c45 100644 --- a/engines/sci/graphics/plane32.cpp +++ b/engines/sci/graphics/plane32.cpp @@ -201,10 +201,9 @@ void Plane::addPicInternal(const GuiResourceId pictureId, const Common::Point *p } else { screenItem->_position = celObj->_relativePosition; } - _screenItemList.add(screenItem); + screenItem->_celObj.reset(celObj); - delete screenItem->_celObj; - screenItem->_celObj = celObj; + _screenItemList.add(screenItem); } _type = (g_sci->_features->hasTransparentPicturePlanes() && transparent) ? kPlaneTypeTransparentPicture : kPlaneTypePicture; } diff --git a/engines/sci/graphics/screen_item32.cpp b/engines/sci/graphics/screen_item32.cpp index 14e71735fb..4e1ed8399b 100644 --- a/engines/sci/graphics/screen_item32.cpp +++ b/engines/sci/graphics/screen_item32.cpp @@ -39,7 +39,6 @@ uint32 ScreenItem::_nextCreationId = 0; ScreenItem::ScreenItem(const reg_t object) : _creationId(_nextCreationId++), -_celObj(nullptr), _object(object), _pictureId(-1), _created(g_sci->_gfxFrameout->getScreenCount()), @@ -59,7 +58,6 @@ _plane(plane), _useInsetRect(false), _z(0), _celInfo(celInfo), -_celObj(nullptr), _fixedPriority(false), _position(0, 0), _object(make_reg(0, _nextObjectId++)), @@ -76,7 +74,6 @@ _plane(plane), _useInsetRect(false), _z(0), _celInfo(celInfo), -_celObj(nullptr), _fixedPriority(false), _position(rect.left, rect.top), _object(make_reg(0, _nextObjectId++)), @@ -98,7 +95,6 @@ _scale(scaleInfo), _useInsetRect(false), _z(0), _celInfo(celInfo), -_celObj(nullptr), _fixedPriority(false), _position(position), _object(make_reg(0, _nextObjectId++)), @@ -115,7 +111,6 @@ _plane(other._plane), _scale(other._scale), _useInsetRect(other._useInsetRect), _celInfo(other._celInfo), -_celObj(nullptr), _object(other._object), _mirrorX(other._mirrorX), _scaledPosition(other._scaledPosition), @@ -127,18 +122,18 @@ _drawBlackLines(other._drawBlackLines) { } void ScreenItem::operator=(const ScreenItem &other) { - // NOTE: The original engine did not check for differences in `_celInfo` - // to clear `_celObj` here; instead, it unconditionally set `_celInfo`, - // didn't clear `_celObj`, and did hacky stuff in `kIsOnMe` to avoid - // testing a mismatched `_celObj`. See `GfxFrameout::kernelIsOnMe` for - // more detail. kCelTypeMem types are unconditionally invalidated because - // the properties of a CelObjMem can "change" when a game deletes a bitmap - // and then creates a new one that reuses the old bitmap's offset in - // BitmapTable (as happens in the LSL7 About screen when hovering names). + // SSCI did not check for differences in `_celInfo` to clear `_celObj` here; + // instead, it unconditionally set `_celInfo`, didn't clear `_celObj`, and + // did hacky stuff in `kIsOnMe` to avoid testing a mismatched `_celObj`. See + // `GfxFrameout::kernelIsOnMe` for more detail. + // + // kCelTypeMem types are unconditionally invalidated because the properties + // of a CelObjMem can "change" when a game deletes a bitmap and then creates + // a new one that reuses the old bitmap's offset in BitmapTable (as happens + // in the LSL7 About screen when hovering names). if (_celInfo.type == kCelTypeMem || _celInfo != other._celInfo) { _celInfo = other._celInfo; - delete _celObj; - _celObj = nullptr; + _celObj.reset(); } _creationId = other._creationId; @@ -153,10 +148,6 @@ void ScreenItem::operator=(const ScreenItem &other) { _drawBlackLines = other._drawBlackLines; } -ScreenItem::~ScreenItem() { - delete _celObj; -} - void ScreenItem::init() { _nextObjectId = 20000; _nextCreationId = 0; @@ -176,17 +167,12 @@ void ScreenItem::setFromObject(SegManager *segMan, const reg_t object, const boo _celInfo.celNo = readSelectorValue(segMan, object, SELECTOR(cel)); if (_celInfo.resourceId <= kPlanePic) { - // TODO: Enhance GfxView or ResourceManager to allow - // metadata for resources to be retrieved once, from a - // single location Resource *view = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, _celInfo.resourceId), false); if (!view) { error("Failed to load %s", _celInfo.toString().c_str()); } - // NOTE: +2 because the header size field itself is excluded from - // the header size in the data - const uint16 headerSize = view->getUint16SEAt(0) + 2; + const uint16 headerSize = view->getUint16SEAt(0) + /* header size field */ sizeof(uint16); const uint8 loopCount = view->getUint8At(2); const uint8 loopSize = view->getUint8At(12); @@ -225,8 +211,7 @@ void ScreenItem::setFromObject(SegManager *segMan, const reg_t object, const boo } if (updateCel || updateBitmap) { - delete _celObj; - _celObj = nullptr; + _celObj.reset(); } if (readSelectorValue(segMan, object, SELECTOR(fixPriority))) { @@ -305,8 +290,8 @@ void ScreenItem::calcRects(const Plane &plane) { } // Cel may use a coordinate system that is not the same size as the - // script coordinate system (usually this means high-resolution - // pictures with low-resolution scripts) + // script coordinate system (usually this means high-resolution pictures + // with low-resolution scripts) if (celObj._xResolution != kLowResX || celObj._yResolution != kLowResY) { // high resolution coordinates @@ -386,7 +371,7 @@ void ScreenItem::calcRects(const Plane &plane) { mulinc(temp, celToScreenX, Ratio()); - CelObjPic *celObjPic = dynamic_cast(_celObj); + CelObjPic *celObjPic = dynamic_cast(_celObj.get()); if (celObjPic == nullptr) { error("Expected a CelObjPic"); } @@ -415,9 +400,8 @@ void ScreenItem::calcRects(const Plane &plane) { if (!scaleX.isOne() || !scaleY.isOne()) { mulinc(_screenItemRect, scaleX, scaleY); - // TODO: This was in the original code, baked into the - // multiplication though it is not immediately clear - // why this is the only one that reduces the BR corner + // TODO: This was in SSCI, baked into the multiplication. It is + // not clear why this is the only one that reduces the BR corner _screenItemRect.right -= 1; _screenItemRect.bottom -= 1; } @@ -434,7 +418,7 @@ void ScreenItem::calcRects(const Plane &plane) { temp.right -= 1; } - CelObjPic *celObjPic = dynamic_cast(_celObj); + CelObjPic *celObjPic = dynamic_cast(_celObj.get()); if (celObjPic == nullptr) { error("Expected a CelObjPic"); } @@ -487,19 +471,19 @@ void ScreenItem::calcRects(const Plane &plane) { } CelObj &ScreenItem::getCelObj() const { - if (_celObj == nullptr) { + if (!_celObj) { switch (_celInfo.type) { case kCelTypeView: - _celObj = new CelObjView(_celInfo.resourceId, _celInfo.loopNo, _celInfo.celNo); + _celObj.reset(new CelObjView(_celInfo.resourceId, _celInfo.loopNo, _celInfo.celNo)); break; case kCelTypePic: error("Internal error, pic screen item with no cel."); break; case kCelTypeMem: - _celObj = new CelObjMem(_celInfo.bitmap); + _celObj.reset(new CelObjMem(_celInfo.bitmap)); break; case kCelTypeColor: - _celObj = new CelObjColor(_celInfo.color, _insetRect.width(), _insetRect.height()); + _celObj.reset(new CelObjColor(_celInfo.color, _insetRect.width(), _insetRect.height())); break; } } @@ -534,7 +518,7 @@ void ScreenItem::printDebugInfo(Console *con) const { con->debugPrintf(" %s\n", _celInfo.toString().c_str()); - if (_celObj != nullptr) { + if (_celObj) { con->debugPrintf(" width %d, height %d, x-resolution %d, y-resolution %d\n", _celObj->_width, _celObj->_height, @@ -583,8 +567,7 @@ void ScreenItem::update() { } _deleted = 0; - delete _celObj; - _celObj = nullptr; + _celObj.reset(); } Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const { @@ -647,15 +630,13 @@ Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const { if (!scaleX.isOne() || !scaleY.isOne()) { // Different games use a different cel scaling mode, but the - // difference isn't consistent across SCI versions; instead, - // it seems to be related to an update that happened during - // SCI2.1mid where games started using hi-resolution game - // scripts + // difference isn't consistent across SCI versions; instead, it + // seems to be related to an update that happened during SCI2.1mid + // where games started using high-resolution game scripts if (scriptWidth == kLowResX) { mulinc(nsRect, scaleX, scaleY); - // TODO: This was in the original code, baked into the - // multiplication though it is not immediately clear - // why this is the only one that reduces the BR corner + // TODO: This was in SSCI, baked into the multiplication. It is + // not clear why this is the only one that reduces the BR corner nsRect.right -= 1; nsRect.bottom -= 1; } else { @@ -693,9 +674,8 @@ Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const { if (!scaleX.isOne() || !scaleY.isOne()) { mulinc(nsRect, scaleX, scaleY); - // TODO: This was in the original code, baked into the - // multiplication though it is not immediately clear - // why this is the only one that reduces the BR corner + // TODO: This was in SSCI, baked into the multiplication. It is not + // clear why this is the only one that reduces the BR corner nsRect.right -= 1; nsRect.bottom -= 1; } diff --git a/engines/sci/graphics/screen_item32.h b/engines/sci/graphics/screen_item32.h index cdde2d3965..5acf759c32 100644 --- a/engines/sci/graphics/screen_item32.h +++ b/engines/sci/graphics/screen_item32.h @@ -49,22 +49,20 @@ class SegManager; #pragma mark ScreenItem /** - * A ScreenItem is the engine-side representation of a - * game script View. + * A ScreenItem is the engine-side representation of a game script View. */ class ScreenItem { private: /** - * A serial used for screen items that are generated - * inside the graphics engine, rather than the - * interpreter. + * A serial used for screen items that are generated inside the graphics + * engine, rather than the interpreter. */ static uint16 _nextObjectId; /** - * A serial used to identify the creation order of - * screen items, to ensure a stable sort order for - * screen items with identical priorities and z-indexes. + * A serial used to identify the creation order of screen items, to ensure a + * stable sort order for screen items with identical priorities and + * z-indexes. */ static uint32 _nextCreationId; @@ -75,160 +73,145 @@ public: reg_t _plane; /** - * Scaling data used to calculate the final screen - * dimensions of the screen item as well as the scaling - * ratios used when drawing the item to screen. + * Scaling data used to calculate the final screen dimensions of the screen + * item as well as the scaling ratios used when drawing the item to screen. */ ScaleInfo _scale; private: /** - * The position & dimensions of the screen item in - * screen coordinates. This rect includes the offset - * of the parent plane, but is not clipped to the - * screen, so may include coordinates that are - * offscreen. + * The position & dimensions of the screen item in screen coordinates. This + * rect includes the offset of the parent plane, but is not clipped to the + * screen, so may include coordinates that are offscreen. */ Common::Rect _screenItemRect; /** - * If true, the `_insetRect` rectangle will be used - * when calculating the dimensions of the screen item - * instead of the cel's intrinsic width and height. + * If true, the `_insetRect` rectangle will be used when calculating the + * dimensions of the screen item instead of the cel's intrinsic width and + * height. * - * In other words, using an inset rect means that - * the cel is cropped to the dimensions given in - * `_insetRect`. + * In other words, using an inset rect means that the cel is cropped to the + * dimensions given in `_insetRect`. */ bool _useInsetRect; /** - * The cropping rectangle used when `_useInsetRect` - * is true. + * The cropping rectangle used when `_useInsetRect` is true. * - * `_insetRect` is also used to describe the fill - * rectangle of a screen item with a CelObjColor - * cel. + * `_insetRect` is also used to describe the fill rectangle of a screen item + * with a CelObjColor cel. */ Common::Rect _insetRect; /** - * The z-index of the screen item in pseudo-3D space. - * Higher values are drawn on top of lower values. + * The z-index of the screen item in pseudo-3D space. Higher values are + * drawn on top of lower values. */ int _z; /** - * Sets the common properties of a screen item that must - * be set both during creation and update of a screen - * item. + * Sets the common properties of a screen item that must be set both during + * creation and update of a screen item. */ void setFromObject(SegManager *segMan, const reg_t object, const bool updateCel, const bool updateBitmap); public: /** - * The creation order number, which ensures a stable - * sort when screen items with identical priorities and - * z-indexes are added to the screen item list. + * The creation order number, which ensures a stable sort when screen items + * with identical priorities and z-indexes are added to the screen item + * list. */ uint32 _creationId; /** - * A descriptor for the cel object represented by the - * screen item. + * A descriptor for the cel object represented by the screen item. */ CelInfo32 _celInfo; /** - * The cel object used to actually render the screen - * item. This member is populated by calling - * `getCelObj`. + * The cel object used to actually render the screen item. This member is + * populated by calling `getCelObj`. */ - mutable CelObj *_celObj; + mutable Common::ScopedPtr _celObj; /** - * If set, the priority for this screen item is fixed - * in place. Otherwise, the priority of the screen item - * is calculated from its y-position + z-index. + * If set, the priority for this screen item is fixed in place. Otherwise, + * the priority of the screen item is calculated from its y-position + + * z-index. */ bool _fixedPriority; /** - * The rendering priority of the screen item, relative - * only to the other screen items within the same plane. - * Higher priorities are drawn above lower priorities. + * The rendering priority of the screen item, relative only to the other + * screen items within the same plane. Higher priorities are drawn above + * lower priorities. */ int16 _priority; /** - * The top-left corner of the screen item, in game - * script coordinates, relative to the parent plane. + * The top-left corner of the screen item, in game script coordinates, + * relative to the parent plane. */ Common::Point _position; /** - * The associated View script object that was - * used to create the ScreenItem, or a numeric - * value in the case of a ScreenItem that was - * generated outside of the VM. + * The associated View script object that was used to create the ScreenItem, + * or a numeric value in the case of a ScreenItem that was generated by the + * kernel. */ reg_t _object; /** - * For screen items representing picture resources, - * the resource ID of the picture. + * For screen items representing picture resources, the resource ID of the + * picture. */ GuiResourceId _pictureId; /** * Flags indicating the state of the screen item. - * - `created` is set when the screen item is first - * created, either from a VM object or from within the - * engine itself - * - `updated` is set when `created` is not already set - * and the screen item is updated from a VM object - * - `deleted` is set by the parent plane, if the parent - * plane is a pic type and its picture resource ID has - * changed + * - `created` is set when the screen item is first created, either from a + * VM object or from within the kernel + * - `updated` is set when `created` is not already set and the screen item + * is updated from a VM object + * - `deleted` is set by the parent plane, if the parent plane is a pic type + * and its picture resource ID has changed */ int _created, _updated, _deleted; /** - * For screen items that represent picture cels, this - * value is set to match the `_mirrorX` property of the - * parent plane and indicates that the cel should be - * drawn horizontally mirrored. For final drawing, it is - * XORed with the `_mirrorX` property of the cel object. - * The cel object's `_mirrorX` property comes from the - * resource data itself. + * For screen items that represent picture cels, this value is set to match + * the `_mirrorX` property of the parent plane and indicates that the cel + * should be + * drawn horizontally mirrored. For final drawing, it is XORed with the + * `_mirrorX` property of the cel object. The cel object's `_mirrorX` + * property comes from the resource data. */ bool _mirrorX; /** - * The scaling ratios to use when drawing this screen - * item. These values are calculated according to the - * scale info whenever the screen item is updated. + * The scaling ratios to use when drawing this screen item. These values are + * calculated according to the scale info whenever the screen item is + * updated. */ Ratio _ratioX, _ratioY; /** - * The top-left corner of the screen item, in screen - * coordinates. + * The top-left corner of the screen item, in screen coordinates. */ Common::Point _scaledPosition; /** - * The position & dimensions of the screen item in - * screen coordinates. This rect includes the offset of - * the parent plane and is clipped to the screen. + * The position & dimensions of the screen item in screen coordinates. This + * rect includes the offset of the parent plane and is clipped to the + * screen. */ Common::Rect _screenRect; /** - * Whether or not the screen item should be drawn - * with black lines drawn every second line. This is - * used when pixel doubling videos to improve apparent - * sharpness at the cost of your eyesight. + * Whether or not the screen item should be drawn with black lines drawn + * every second line. This is used when pixel doubling videos to improve + * apparent sharpness at the cost of your eyesight. */ bool _drawBlackLines; @@ -242,7 +225,6 @@ public: ScreenItem(const reg_t plane, const CelInfo32 &celInfo, const Common::Rect &rect); ScreenItem(const reg_t plane, const CelInfo32 &celInfo, const Common::Point &position, const ScaleInfo &scaleInfo); ScreenItem(const ScreenItem &other); - ~ScreenItem(); void operator=(const ScreenItem &); inline bool operator<(const ScreenItem &other) const { @@ -337,39 +319,36 @@ public: } /** - * Calculates the dimensions and scaling parameters for - * the screen item, using the given plane as the parent - * plane for screen rect positioning. + * Calculates the dimensions and scaling parameters for the screen item, + * using the given plane as the parent plane for screen rect positioning. * - * @note This method was called Update in SCI engine. + * @note This method was called Update in SSCI. */ void calcRects(const Plane &plane); /** - * Retrieves the corresponding cel object for this - * screen item. If a cel object does not already exist, - * one will be created and assigned. + * Retrieves the corresponding cel object for this screen item. If a cel + * object does not already exist, one will be created and assigned. */ CelObj &getCelObj() const; void printDebugInfo(Console *con) const; /** - * Updates the properties of the screen item from a - * VM object. + * Updates the properties of the screen item from a VM object. */ void update(const reg_t object); /** - * Updates the properties of the screen item for one not belonging - * to a VM object. Originally GraphicsMgr::UpdateScreenItem. + * Updates the properties of the screen item for one not belonging to a VM + * object. Originally GraphicsMgr::UpdateScreenItem. */ void update(); /** - * Gets the "now seen" rect for the screen item, which - * represents the current size and position of the - * screen item on the screen in script coordinates. + * Gets the "now seen" rect for the screen item, which represents the + * current size and position of the screen item on the screen in script + * coordinates. */ Common::Rect getNowSeenRect(const Plane &plane) const; }; -- cgit v1.2.3