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/graphics/screen_item32.cpp | 82 +++++++++++++--------------------- 1 file changed, 31 insertions(+), 51 deletions(-) (limited to 'engines/sci/graphics/screen_item32.cpp') 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; } -- cgit v1.2.3