Skip to content

Commit

Permalink
SCI32: Clean up ScreenItem
Browse files Browse the repository at this point in the history 
* Rewrap comments to 80 columns
* Clarify comments where possible
* Use smart pointers where appropriate
  • Loading branch information
@csnover
csnover committed Oct 7, 2017
1 parent 5cffa38 commit 0ac5d84
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 152 deletions.
1 change: 0 additions & 1 deletion engines/sci/engine/object.h
View file
Expand Up @@ -169,7 +169,6 @@ class Object : public Common::Serializable {
}
}

// NOTE: In real engine, -info- is treated as byte size
void clearInfoSelectorFlag(infoSelectorFlags flag) {
if (getSciVersion() == SCI_VERSION_3) {
_infoSelectorSci3 &= ~flag;
Expand Down
5 changes: 2 additions & 3 deletions engines/sci/graphics/plane32.cpp
View file
Expand Up @@ -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;
}
Expand Down
82 changes: 31 additions & 51 deletions engines/sci/graphics/screen_item32.cpp
View file
Expand Up @@ -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()),
Expand All @@ -59,7 +58,6 @@ _plane(plane),
_useInsetRect(false),
_z(0),
_celInfo(celInfo),
_celObj(nullptr),
_fixedPriority(false),
_position(0, 0),
_object(make_reg(0, _nextObjectId++)),
Expand All @@ -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++)),
Expand All @@ -98,7 +95,6 @@ _scale(scaleInfo),
_useInsetRect(false),
_z(0),
_celInfo(celInfo),
_celObj(nullptr),
_fixedPriority(false),
_position(position),
_object(make_reg(0, _nextObjectId++)),
Expand All @@ -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),
Expand All @@ -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;
Expand All @@ -153,10 +148,6 @@ void ScreenItem::operator=(const ScreenItem &other) {
_drawBlackLines = other._drawBlackLines;
}

ScreenItem::~ScreenItem() {
delete _celObj;
}

void ScreenItem::init() {
_nextObjectId = 20000;
_nextCreationId = 0;
Expand All @@ -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);

Expand Down Expand Up @@ -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))) {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -386,7 +371,7 @@ void ScreenItem::calcRects(const Plane &plane) {

mulinc(temp, celToScreenX, Ratio());

CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj);
CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj.get());
if (celObjPic == nullptr) {
error("Expected a CelObjPic");
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -434,7 +418,7 @@ void ScreenItem::calcRects(const Plane &plane) {
temp.right -= 1;
}

CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj);
CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj.get());
if (celObjPic == nullptr) {
error("Expected a CelObjPic");
}
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -583,8 +567,7 @@ void ScreenItem::update() {
}
_deleted = 0;

delete _celObj;
_celObj = nullptr;
_celObj.reset();
}

Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit 0ac5d84

Please sign in to comment.