Skip to content

Commit

Permalink
SCI32: Clean up CelObj
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
* Change view/pic flags detection to always use word-size
  (byte-size check for flag 0x80 was a compiler optimisation)
  • Loading branch information
@csnover
csnover committed Oct 7, 2017
1 parent ac0a83a commit b6c3f0f
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 236 deletions.
7 changes: 3 additions & 4 deletions engines/sci/engine/segment.h
View file
Expand Up @@ -1102,12 +1102,11 @@ class SciBitmap : public Common::Serializable {

BITMAP_PROPERTY(32, DataOffset, 24);

// NOTE: This property is used as a "magic number" for
// validating that a block of memory is a valid bitmap,
// and so is always set to the size of the header.
// This property is used as a "magic number" for validating that a block of
// memory is a valid bitmap, and so is always set to the size of the header.
BITMAP_PROPERTY(32, UncompressedDataOffset, 28);

// NOTE: This property always seems to be zero
// This property always seems to be zero in SSCI
BITMAP_PROPERTY(32, ControlOffset, 32);

inline uint16 getXResolution() const {
Expand Down
118 changes: 50 additions & 68 deletions engines/sci/graphics/celobj32.cpp
View file
Expand Up @@ -35,7 +35,7 @@
namespace Sci {
#pragma mark CelScaler

CelScaler *CelObj::_scaler = nullptr;
Common::ScopedPtr<CelScaler> CelObj::_scaler;

This comment has been minimized.

Sign in to view

Copy link
@sev-

sev- Mar 28, 2018

Member

This now requires global constructor and needs to be fixed.

void CelScaler::activateScaleTables(const Ratio &scaleX, const Ratio &scaleY) {
for (int i = 0; i < ARRAYSIZE(_scaleTables); ++i) {
Expand Down Expand Up @@ -74,9 +74,9 @@ void CelScaler::buildLookupTable(int *table, const Ratio &ratio, const int size)
}
}

const CelScalerTable *CelScaler::getScalerTable(const Ratio &scaleX, const Ratio &scaleY) {
const CelScalerTable &CelScaler::getScalerTable(const Ratio &scaleX, const Ratio &scaleY) {
activateScaleTables(scaleX, scaleY);
return &_scaleTables[_activeIndex];
return _scaleTables[_activeIndex];
}

#pragma mark -
Expand All @@ -87,21 +87,13 @@ void CelObj::init() {
CelObj::deinit();
_drawBlackLines = false;
_nextCacheId = 1;
_scaler = new CelScaler();
_cache = new CelCache;
_cache->resize(100);
_scaler.reset(new CelScaler());
_cache.reset(new CelCache(100));
}

void CelObj::deinit() {
delete _scaler;
_scaler = nullptr;
if (_cache != nullptr) {
for (CelCache::iterator it = _cache->begin(); it != _cache->end(); ++it) {
delete it->celObj;
}
}
delete _cache;
_cache = nullptr;
_scaler.reset();
_cache.reset();
}

#pragma mark -
Expand Down Expand Up @@ -172,10 +164,9 @@ struct SCALER_Scale {
_minX(targetRect.left),
_maxX(targetRect.right - 1),
#endif
// The maximum width of the scaled object may not be as
// wide as the source data it requires if downscaling,
// so just always make the reader decompress an entire
// line of source data when scaling
// The maximum width of the scaled object may not be as wide as the source
// data it requires if downscaling, so just always make the reader
// decompress an entire line of source data when scaling
_reader(celObj, celObj._width) {
#ifndef NDEBUG
assert(_minX <= _maxX);
Expand Down Expand Up @@ -203,39 +194,39 @@ struct SCALER_Scale {
// games which use global scaling are the ones that use low-resolution
// script coordinates too.

const CelScalerTable *table = CelObj::_scaler->getScalerTable(scaleX, scaleY);
const CelScalerTable &table = CelObj::_scaler->getScalerTable(scaleX, scaleY);

if (g_sci->_gfxFrameout->getCurrentBuffer().scriptWidth == kLowResX) {
if (g_sci->_gfxFrameout->getScriptWidth() == kLowResX) {
const int16 unscaledX = (scaledPosition.x / scaleX).toInt();
if (FLIP) {
const int lastIndex = celObj._width - 1;
for (int16 x = targetRect.left; x < targetRect.right; ++x) {
_valuesX[x] = lastIndex - (table->valuesX[x] - unscaledX);
_valuesX[x] = lastIndex - (table.valuesX[x] - unscaledX);
}
} else {
for (int16 x = targetRect.left; x < targetRect.right; ++x) {
_valuesX[x] = table->valuesX[x] - unscaledX;
_valuesX[x] = table.valuesX[x] - unscaledX;
}
}

const int16 unscaledY = (scaledPosition.y / scaleY).toInt();
for (int16 y = targetRect.top; y < targetRect.bottom; ++y) {
_valuesY[y] = table->valuesY[y] - unscaledY;
_valuesY[y] = table.valuesY[y] - unscaledY;
}
} else {
if (FLIP) {
const int lastIndex = celObj._width - 1;
for (int16 x = targetRect.left; x < targetRect.right; ++x) {
_valuesX[x] = lastIndex - table->valuesX[x - scaledPosition.x];
_valuesX[x] = lastIndex - table.valuesX[x - scaledPosition.x];
}
} else {
for (int16 x = targetRect.left; x < targetRect.right; ++x) {
_valuesX[x] = table->valuesX[x - scaledPosition.x];
_valuesX[x] = table.valuesX[x - scaledPosition.x];
}
}

for (int16 y = targetRect.top; y < targetRect.bottom; ++y) {
_valuesY[y] = table->valuesY[y - scaledPosition.y];
_valuesY[y] = table.valuesY[y - scaledPosition.y];
}
}
}
Expand Down Expand Up @@ -412,8 +403,8 @@ struct MAPPER_NoMDNoSkip {
struct MAPPER_Map {
inline void draw(byte *target, const byte pixel, const uint8 skipColor) const {
if (pixel != skipColor) {
// NOTE: For some reason, SSCI never checks if the source
// pixel is *above* the range of remaps.
// For some reason, SSCI never checks if the source pixel is *above*
// the range of remaps, so we do not either.
if (pixel < g_sci->_gfxRemap32->getStartColor()) {
*target = pixel;
} else if (g_sci->_gfxRemap32->remapEnabled(pixel)) {
Expand All @@ -429,8 +420,8 @@ struct MAPPER_Map {
*/
struct MAPPER_NoMap {
inline void draw(byte *target, const byte pixel, const uint8 skipColor) const {
// NOTE: For some reason, SSCI never checks if the source
// pixel is *above* the range of remaps.
// For some reason, SSCI never checks if the source pixel is *above* the
// range of remaps, so we do not either.
if (pixel != skipColor && pixel < g_sci->_gfxRemap32->getStartColor()) {
*target = pixel;
}
Expand All @@ -444,9 +435,9 @@ void CelObj::draw(Buffer &target, const ScreenItem &screenItem, const Common::Re
_drawBlackLines = screenItem._drawBlackLines;

if (_remap) {
// NOTE: In the original code this check was `g_Remap_numActiveRemaps && _remap`,
// but since we are already in a `_remap` branch, there is no reason to check it
// again
// In SSCI, this check was `g_Remap_numActiveRemaps && _remap`, but
// since we are already in a `_remap` branch, there is no reason to
// check that again
if (g_sci->_gfxRemap32->getRemapCount()) {
if (scaleX.isOne() && scaleY.isOne()) {
if (_compressionType == kCelCompressionNone) {
Expand Down Expand Up @@ -612,7 +603,7 @@ void CelObj::submitPalette() const {
#pragma mark CelObj - Caching

int CelObj::_nextCacheId = 1;
CelCache *CelObj::_cache = nullptr;
Common::ScopedPtr<CelCache> CelObj::_cache;

This comment has been minimized.

Sign in to view

Copy link
@sev-

sev- Mar 28, 2018

Member

This too now requires global constructor and needs to be fixed.

int CelObj::searchCache(const CelInfo32 &celInfo, int *const nextInsertIndex) const {
*nextInsertIndex = -1;
Expand Down Expand Up @@ -648,12 +639,7 @@ void CelObj::putCopyInCache(const int cacheIndex) const {
}

CelCacheEntry &entry = (*_cache)[cacheIndex];

if (entry.celObj != nullptr) {
delete entry.celObj;
}

entry.celObj = duplicate();
entry.celObj.reset(duplicate());
entry.id = ++_nextCacheId;
}

Expand Down Expand Up @@ -807,8 +793,8 @@ void CelObj::drawUncompHzFlipNoMDNoSkip(Buffer &target, const Common::Rect &targ
}

void CelObj::scaleDrawNoMD(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
// In SSCI the checks are > because their rects are BR-inclusive;
// our checks are >= because our rects are BR-exclusive
// In SSCI the checks are > because their rects are BR-inclusive; our checks
// are >= because our rects are BR-exclusive
if (g_sci->_features->hasEmptyScaleDrawHack() &&
(targetRect.left >= targetRect.right ||
targetRect.top >= targetRect.bottom)) {
Expand All @@ -822,8 +808,8 @@ void CelObj::scaleDrawNoMD(Buffer &target, const Ratio &scaleX, const Ratio &sca
}

void CelObj::scaleDrawUncompNoMD(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
// In SSCI the checks are > because their rects are BR-inclusive;
// our checks are >= because our rects are BR-exclusive
// In SSCI the checks are > because their rects are BR-inclusive; our checks
// are >= because our rects are BR-exclusive
if (g_sci->_features->hasEmptyScaleDrawHack() &&
(targetRect.left >= targetRect.right ||
targetRect.top >= targetRect.bottom)) {
Expand Down Expand Up @@ -861,11 +847,11 @@ int16 CelObjView::getNumCels(const GuiResourceId viewId, int16 loopNo) {

const uint16 loopCount = data[2];

// Every version of SCI32 has a logic error in this function that causes
// random memory to be read if a script requests the cel count for one
// past the maximum loop index. For example, GK1 room 808 does this, and
// gets stuck in an infinite loop because the game script expects this
// method to return a non-zero value.
// Every version of SSCI has a logic error in this function that causes
// random memory to be read if a script requests the cel count for one past
// the maximum loop index. For example, GK1 room 808 does this, and gets
// stuck in an infinite loop because the game script expects this method to
// return a non-zero value.
// This bug is triggered in basically every SCI32 game and appears to be
// universally fixable simply by always using the next lowest loop instead.
if (loopNo == loopCount) {
Expand Down Expand Up @@ -904,7 +890,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
const int cacheIndex = searchCache(_info, &cacheInsertIndex);
if (cacheIndex != -1) {
CelCacheEntry &entry = (*_cache)[cacheIndex];
const CelObjView *const cachedCelObj = dynamic_cast<CelObjView *>(entry.celObj);
const CelObjView *const cachedCelObj = dynamic_cast<CelObjView *>(entry.celObj.get());
if (cachedCelObj == nullptr) {
error("Expected a CelObjView in cache slot %d", cacheIndex);
}
Expand All @@ -915,7 +901,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int

const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);

// NOTE: SCI2.1/SQ6 just silently returns here.
// SSCI just silently returns here
if (!resource) {
error("View resource %d not found", viewId);
}
Expand Down Expand Up @@ -944,8 +930,6 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
_info.loopNo = loopCount - 1;
}

// NOTE: This is the actual check, in the actual location,
// from SCI engine.
if (loopNo < 0) {
error("Loop is less than 0");
}
Expand Down Expand Up @@ -1011,10 +995,8 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
error("Compression type not supported - V: %d L: %d C: %d", _info.resourceId, _info.loopNo, _info.celNo);
}

if (celHeader[10] & 128) {
// NOTE: This is correct according to SCI2.1/SQ6/DOS;
// the engine re-reads the byte value as a word value
const uint16 flags = celHeader.getUint16SEAt(10);
const uint16 flags = celHeader.getUint16SEAt(10);
if (flags & 0x80) {
_transparent = flags & 1 ? true : false;
_remap = flags & 2 ? true : false;
} else if (_compressionType == kCelCompressionNone) {
Expand Down Expand Up @@ -1096,7 +1078,7 @@ Common::Point CelObjView::getLinkPosition(const int16 linkId) const {
Common::Point point;
point.x = linkTable.getInt16SEAt(0);
if (_mirrorX) {
// NOTE: SSCI had an off-by-one error here (missing -1)
// SSCI had an off-by-one error here (missing -1)
point.x = _width - point.x - 1;
}
point.y = linkTable.getInt16SEAt(2);
Expand Down Expand Up @@ -1127,7 +1109,7 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
const int cacheIndex = searchCache(_info, &cacheInsertIndex);
if (cacheIndex != -1) {
CelCacheEntry &entry = (*_cache)[cacheIndex];
const CelObjPic *const cachedCelObj = dynamic_cast<CelObjPic *>(entry.celObj);
const CelObjPic *const cachedCelObj = dynamic_cast<CelObjPic *>(entry.celObj.get());
if (cachedCelObj == nullptr) {
error("Expected a CelObjPic in cache slot %d", cacheIndex);
}
Expand All @@ -1138,7 +1120,7 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {

const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypePic, picId), false);

// NOTE: SCI2.1/SQ6 just silently returns here.
// SSCI just silently returns here
if (!resource) {
error("Pic resource %d not found", picId);
}
Expand Down Expand Up @@ -1183,10 +1165,9 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
_yResolution = 400;
}

if (celHeader.getUint8At(10) & 128) {
// NOTE: This is correct according to SCI2.1/SQ6/DOS;
// the engine re-reads the byte value as a word value
const uint16 flags = celHeader.getUint16SEAt(10);

const uint16 flags = celHeader.getUint16SEAt(10);
if (flags & 0x80) {
_transparent = flags & 1 ? true : false;
_remap = flags & 2 ? true : false;
} else {
Expand Down Expand Up @@ -1251,7 +1232,8 @@ CelObjMem::CelObjMem(const reg_t bitmapObject) {

SciBitmap *bitmap = g_sci->getEngineState()->_segMan->lookupBitmap(bitmapObject);

// NOTE: SSCI did no error checking here at all.
// SSCI did no error checking here at all so would just end up reading
// garbage or crashing if this ever happened
if (!bitmap) {
error("Bitmap %04x:%04x not found", PRINT_REG(bitmapObject));
}
Expand Down Expand Up @@ -1293,8 +1275,8 @@ CelObjColor::CelObjColor(const uint8 color, const int16 width, const int16 heigh
}

void CelObjColor::draw(Buffer &target, const ScreenItem &screenItem, const Common::Rect &targetRect, const bool mirrorX) {
// TODO: The original engine sets this flag but why? One cannot
// draw a solid color mirrored.
// One cannot draw a solid color mirrored, but SSCI sets it anyway, so we do
// too
_drawMirrored = mirrorX;
draw(target, targetRect);
}
Expand Down

0 comments on commit b6c3f0f

Please sign in to comment.