Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLUDGE: Add Z-Buffers #5432

Merged
merged 2 commits into from Nov 15, 2023
Merged

SLUDGE: Add Z-Buffers #5432

merged 2 commits into from Nov 15, 2023

Conversation

polyesterswing
Copy link
Contributor

This commit removes the "SpriteLayer" logic previously used in favour of a Z-Buffer implementation

Copy link
Contributor

coderabbitai bot commented Nov 14, 2023

Walkthrough

Walkthrough

The changes across the Sludge engine's graphics system involve a significant overhaul of the Z-buffer and sprite layer handling. The SpriteLayers structure and related functions have been removed, indicating a shift away from sorting sprites by layers. Instead, a new Z-buffer surface and texture array are introduced, with functions to manage drawing and filling these buffers. This refactoring likely aims to streamline rendering processes, improve memory management, and enhance the depth handling of graphical elements.

Changes

File(s) Summary
.../freeze.cpp, .../freeze.h Modified freeze and unfreeze functions to handle zBuffer->tex instead of zBuffer->sprites. Changed zBufferSprites to zBufferTex of type uint8*.
.../graphics.cpp, .../graphics.h Removed SpriteLayers and related functions. Added Z-buffer surface handling and new functions drawSpriteToZBuffer and fillZBuffer.
.../sludger.cpp Removed call to displaySpriteLayers() from displayBase() function, affecting rendering behavior.
.../sprites.cpp, .../sprites.h Removed SpriteLayers struct and related functions. Adjusted sprite rendering functions to use new Z-buffer surface and texture handling. Introduced drawSpriteToZBuffer function.
.../zbuffer.cpp, .../zbuffer.h Removed sprites array logic, introduced tex array, and added new Z-buffer management functions. Updated ZBufferData struct to include width, height, and tex.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Comment on lines 43 to 49
ScreenRegionList *allScreenRegions;
Graphics::Surface backdropSurface;
Graphics::Surface lightMapSurface;
Graphics::Surface *zBufferSprites;
uint8 *zBufferTex;
int zPanels;
ParallaxLayers *parallaxLayers;
int lightMapNumber, zBufferNumber;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from zBufferSprites to zBufferTex suggests a shift in how the Z-buffer is managed. It's important to ensure that all references to the old zBufferSprites throughout the codebase have been updated to use zBufferTex. Additionally, since zBufferTex is a pointer to uint8, it's crucial to verify that memory management (allocation and deallocation) is handled correctly to prevent memory leaks or dangling pointers. Also, ensure that the serialization and deserialization logic for the FrozenStuffStruct is updated to accommodate this change, as it could affect save game compatibility.

Comment on lines 462 to 478
y2 = y1 + diffY;
}

float z;

if (useZB && _zBuffer->numPanels) {
int i;
for (i = 1; i < _zBuffer->numPanels; i++) {
if (_zBuffer->panel[i] >= y + _cameraY) {
i--;
break;
}
}
z = 0.999 - (double) i * (1.0 / 128.0);
} else {
z = -0.5;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation of the z value for depth sorting seems to be based on the number of panels in the zBuffer and the y position of the sprite. This logic is repeated in the scaleSprite and fixScaleSprite functions. Consider refactoring this into a separate function to reduce code duplication and improve maintainability.

Comment on lines 491 to 502
ptr = nullptr;
}
} else {
int d = useZB ? y + _cameraY : (y + _cameraY > _sceneHeight * 0.6 ? _sceneHeight + 1 : 0);
addSpriteDepth(blitted, d, x1, y1, (mirror ? Graphics::FLIP_H : Graphics::FLIP_NONE), diffX, diffY, ptr, 255 - thisPerson->transparency);

// TODO: you dont need to copy the whole render surface, just the part to which the sprite may be drawn
Graphics::ManagedSurface scaled(&_renderSurface, DisposeAfterUse::NO);

Graphics::ManagedSurface tmp(blitted, DisposeAfterUse::NO);
tmp.blendBlitTo(scaled, x1, y1, (mirror ? Graphics::FLIP_H : Graphics::FLIP_NONE), nullptr, MS_ARGB(255 - thisPerson->transparency, 255, 255, 255), diffX, diffY);

drawSpriteToZBuffer(0, 0, z, scaled.rawSurface());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code for drawing a sprite to the zBuffer is duplicated in the scaleSprite function. This could be refactored into a separate function to improve code reuse and maintainability. Additionally, the comment on line 495 suggests an optimization that should be considered to avoid unnecessary copying of the entire render surface.

Comment on lines 537 to 553
x1 = x - (int)((mirror ? (float)(single.surface.w - (single.xhot + 1)) : (float)single.xhot) * scale);
int y1 = y - (int)((single.yhot - thisPerson->floaty) * scale);

float z;

if (useZB && _zBuffer->numPanels) {
int i;
for (i = 1; i < _zBuffer->numPanels; i++) {
if (_zBuffer->panel[i] >= y + _cameraY) {
i--;
break;
}
}
z = 0.999 - (double) i * (1.0 / 128.0);
} else {
z = -0.5;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The z value calculation is duplicated here in the fixScaleSprite function, as previously mentioned. This should be refactored into a separate function.

Comment on lines 574 to 587
ptr = nullptr;
}
} else {
int d = useZB ? y + _cameraY : (y + _cameraY > _sceneHeight * 0.6 ? _sceneHeight + 1 : 0);
addSpriteDepth(&single.surface, d, x1, y1, (mirror ? Graphics::FLIP_H : Graphics::FLIP_NONE), diffX, diffY, ptr, 255 - thisPerson->transparency);
}
Graphics::ManagedSurface scaled(&_renderSurface, DisposeAfterUse::NO);

Graphics::ManagedSurface tmp(blitted, DisposeAfterUse::NO);
tmp.blendBlitTo(scaled, x1, y1, (mirror ? Graphics::FLIP_H : Graphics::FLIP_NONE), nullptr, MS_ARGB(255 - thisPerson->transparency, 255, 255, 255), diffX, diffY);

// draw all
displaySpriteLayers();
drawSpriteToZBuffer(0, 0, z, scaled.rawSurface());

}

// copy screen to backdrop
_backdropSurface.copyFrom(_renderSurface);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the code for drawing a sprite to the zBuffer is duplicated in the fixScaleSprite function. This should be refactored into a separate function. Also, the code for copying the screen to the backdrop at the end of the function could potentially be a significant performance hit and should be reviewed to ensure it's necessary and optimized.

Comment on lines 34 to 43
void GraphicsManager::killZBuffer() {
if (_zBuffer->sprites) {
for (int i = 0; i < _zBuffer->numPanels; ++i) {
_zBuffer->sprites[i].free();
}
delete []_zBuffer->sprites;
_zBuffer->sprites = nullptr;

if (_zBuffer->tex) {
delete []_zBuffer->tex;
_zBuffer->tex = nullptr;
}

_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The killZBuffer function correctly checks if _zBuffer->tex is not nullptr before attempting to delete it and sets it to nullptr after deletion to prevent dangling pointers. However, it does not check if _zBuffer itself is valid before accessing its members. This could lead to a crash if _zBuffer is nullptr.

void GraphicsManager::killZBuffer() {
+   if (!_zBuffer) return;
    if (_zBuffer->tex) {

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
void GraphicsManager::killZBuffer() {
if (_zBuffer->sprites) {
for (int i = 0; i < _zBuffer->numPanels; ++i) {
_zBuffer->sprites[i].free();
}
delete []_zBuffer->sprites;
_zBuffer->sprites = nullptr;
if (_zBuffer->tex) {
delete []_zBuffer->tex;
_zBuffer->tex = nullptr;
}
_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}
void GraphicsManager::killZBuffer() {
if (!_zBuffer) return;
if (_zBuffer->tex) {
delete []_zBuffer->tex;
_zBuffer->tex = nullptr;
}
_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}

Comment on lines 158 to 166
void saveZBuffer(Common::WriteStream *stream);
bool loadZBuffer(Common::SeekableReadStream *stream);

void drawSpriteToZBuffer(int x, int y, double depth, const Graphics::Surface &surface);
void fillZBuffer(double d);

// Colors
void setBlankColor(int r, int g, int b) { _currentBlankColour = _renderSurface.format.RGBToColor(r & 255, g & 255, b & 255);};
void setBurnColor(int r, int g, int b) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of Z-buffer management functions is in line with the new rendering strategy. However, ensure that the setBlankColor and setBurnColor functions are correctly implemented, as the current hunk seems to be missing the implementation for setBurnColor. The code snippet for setBurnColor should set the _currentBurnR, _currentBurnG, and _currentBurnB member variables, similar to how setBlankColor is setting _currentBlankColour.

- void setBurnColor(int r, int g, int b) {
+ void setBurnColor(int r, int g, int b) {
+   _currentBurnR = r;
+   _currentBurnG = g;
+   _currentBurnB = b;
+ }

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
void saveZBuffer(Common::WriteStream *stream);
bool loadZBuffer(Common::SeekableReadStream *stream);
void drawSpriteToZBuffer(int x, int y, double depth, const Graphics::Surface &surface);
void fillZBuffer(double d);
// Colors
void setBlankColor(int r, int g, int b) { _currentBlankColour = _renderSurface.format.RGBToColor(r & 255, g & 255, b & 255);};
void setBurnColor(int r, int g, int b) {
void saveZBuffer(Common::WriteStream *stream);
bool loadZBuffer(Common::SeekableReadStream *stream);
void drawSpriteToZBuffer(int x, int y, double depth, const Graphics::Surface &surface);
void fillZBuffer(double d);
// Colors
void setBlankColor(int r, int g, int b) { _currentBlankColour = _renderSurface.format.RGBToColor(r & 255, g & 255, b & 255);};
void setBurnColor(int r, int g, int b) {
_currentBurnR = r;
_currentBurnG = g;
_currentBurnB = b;
}

Comment on lines 121 to 133
sortZPal(yPalette, sorted, _zBuffer->numPanels);
for (int y = 0; y < _zBuffer->numPanels; y++) {
_zBuffer->panel[y] = yPalette[sorted[y]];
sortback[sorted[y]] = y;
debugC(2, kSludgeDebugZBuffer, "Y-value : %i", _zBuffer->panel[y]);
}

int picWidth = _sceneWidth;
int picHeight = _sceneHeight;

_zBuffer->sprites = nullptr;
_zBuffer->sprites = new Graphics::Surface[_zBuffer->numPanels];

for (int i = 0; i < _zBuffer->numPanels; ++i) {
_zBuffer->sprites[i].create(picWidth, picHeight, *g_sludge->getScreenPixelFormat());
}
_zBuffer->tex = nullptr;
_zBuffer->tex = new uint8 [picHeight * picWidth];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setZBuffer function sorts the Z-buffer panels and allocates memory for _zBuffer->tex. It is important to ensure that the memory allocation is successful and handle the case where it might fail. Additionally, the code does not check if _zBuffer->panel and sortback arrays are large enough to hold _zBuffer->numPanels elements, which could lead to buffer overflows.

Comment on lines 168 to 188
void GraphicsManager::drawSpriteToZBuffer(int x, int y, double depth, const Graphics::Surface &surface) {

outFile->open(Common::String::format("dumps/zbuffer%04d-%d.png", num, i));
Image::writePNG(*outFile, _zBuffer->sprites[i]);
outFile->finalize();
outFile->close();
delete outFile;
}
for (uint y1 = 0; y1 < (uint)surface.h; y1++) {
for (uint x1 = 0; x1 < (uint)surface.w; x1++) {
if (x1 + x >= _sceneWidth || y1 + y >= _sceneHeight) {
continue;
}

return true;
byte *target = (byte *)_renderSurface.getBasePtr(x1 + x, y1 + y);
const byte *source = (const byte *)surface.getBasePtr(x1, y1);

if (depth <= _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {

if (source[0] == 0xff) {
// Completely opaque, so copy RGB values over
target[0] = 0xff;
target[1] = source[1];
target[2] = source[2];
target[3] = source[3];

}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawSpriteToZBuffer function checks if the sprite is within the scene bounds before drawing it. It also checks the depth value before drawing a pixel, which is correct for Z-buffering. However, it uses a magic number 0xff to check for opacity, which should ideally be defined as a constant for clarity.

Comment on lines 149 to 156
initGraphics(_winWidth, _winHeight, _vm->getScreenPixelFormat());
_renderSurface.create(_winWidth, _winHeight, *_vm->getScreenPixelFormat());

_zBufferSurface = new double[_winWidth * _winHeight];

if (!killResizeBackdrop(_winWidth, _winHeight))
return fatal("Couldn't allocate memory for backdrop");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation of _zBufferSurface as an array of double is correct if the Z-buffer is intended to store depth values as floating-point numbers. However, the type double is typically 64 bits, which might be more precision than needed for a Z-buffer, potentially wasting memory. Consider using float if the extra precision is not required. Additionally, ensure that the size of the Z-buffer matches the size of the screen or the intended render target.

// Change the allocation from double to float if extra precision is not needed
_zBufferSurface = new float[_winWidth * _winHeight];

Comment on lines 273 to 277
// kill zBuffer
if (_zBuffer->originalNum >= 0 && _zBuffer->sprites) {
if (_zBuffer->originalNum >= 0 && _zBuffer->tex) {
int num = _zBuffer->originalNum;
killZBuffer();
_zBuffer->originalNum = num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for killing the Z-buffer seems to be duplicated in the pasteSpriteToBackDrop function. This could be refactored into a separate function to avoid code duplication and improve maintainability. Additionally, the check for _zBuffer->originalNum >= 0 suggests that there might be a special case or an invalid state for the Z-buffer that should be documented for clarity.

Comment on lines 296 to 300
// kill zBuffer
if (_zBuffer->originalNum >= 0 && _zBuffer->sprites) {
if (_zBuffer->originalNum >= 0 && _zBuffer->tex) {
int num = _zBuffer->originalNum;
killZBuffer();
_zBuffer->originalNum = num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, the Z-buffer killing logic is duplicated in the burnSpriteToBackDrop function. Refactoring this into a separate function would improve code maintainability.

Comment on lines 465 to 478
float z;

if (useZB && _zBuffer->numPanels) {
int i;
for (i = 1; i < _zBuffer->numPanels; i++) {
if (_zBuffer->panel[i] >= y + _cameraY) {
i--;
break;
}
}
z = 0.999 - (double)i * (1.0 / 128.0);
} else {
z = -0.5;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation of the z value for Z-buffering is done within the scaleSprite function. This logic could potentially be encapsulated in a separate function or method to improve readability and maintainability, especially if the Z-buffering logic becomes more complex in the future.

Comment on lines 540 to 553
float z;

if (useZB && _zBuffer->numPanels) {
int i;
for (i = 1; i < _zBuffer->numPanels; i++) {
if (_zBuffer->panel[i] >= y + _cameraY) {
i--;
break;
}
}
z = 0.999 - (double)i * (1.0 / 128.0);
} else {
z = -0.5;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation of the z value for Z-buffering is duplicated in the fixScaleSprite function. This logic should be refactored into a separate function or method to improve code maintainability and to avoid duplication.

This commit removes the "SpriteLayer" logic previously used in favour of
a Z-Buffer implementation
Comment on lines 65 to 74
newFreezer->parallaxLayers = _parallaxLayers;
_parallaxLayers = NULL;

newFreezer->zBufferSprites = _zBuffer->sprites;
newFreezer->zBufferTex = _zBuffer->tex;
newFreezer->zBufferNumber = _zBuffer->originalNum;
newFreezer->zPanels = _zBuffer->numPanels;
_zBuffer->sprites = NULL;
_zBuffer->tex = NULL;
// resizeBackdrop kills parallax stuff, light map, z-buffer...
if (!killResizeBackdrop(_winWidth, _winHeight))
return fatal("Can't create new temporary backdrop buffer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code correctly updates the FrozenStuffStruct with the current state of the parallax layers and Z-buffer textures, and then nullifies the pointers in the GraphicsManager to avoid dangling references. However, there is a potential memory leak if _zBuffer->tex is not properly deallocated before being set to NULL. Ensure that any dynamic memory associated with _zBuffer->tex is released before this assignment.

if (_zBuffer->tex) {
    delete[] _zBuffer->tex; // Assuming tex was allocated with new[]
    _zBuffer->tex = NULL;
}

Comment on lines +141 to 144
_zBuffer->tex = _frozenStuff->zBufferTex;
killZBuffer();
_zBuffer->originalNum = _frozenStuff->zBufferNumber;
_zBuffer->numPanels = _frozenStuff->zPanels;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code restores the Z-buffer texture and panel information from the frozen state. However, it calls killZBuffer() after assigning the texture pointer, which could lead to a memory leak if killZBuffer() deallocates the Z-buffer's texture. The order of operations should be reviewed to ensure that no memory is leaked and that the texture is not inadvertently deleted.

// Ensure that the Z-buffer texture is not deleted if it's being restored
if (_zBuffer->tex != _frozenStuff->zBufferTex) {
    killZBuffer();
}
_zBuffer->tex = _frozenStuff->zBufferTex;

Comment on lines 271 to 277
}

// kill zBuffer
if (_zBuffer->originalNum >= 0 && _zBuffer->sprites) {
if (_zBuffer->originalNum >= 0 && _zBuffer->tex) {
int num = _zBuffer->originalNum;
killZBuffer();
_zBuffer->originalNum = num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for killing the Z-buffer is duplicated in the pasteSpriteToBackDrop function. This could be refactored into a separate function to avoid code duplication and improve maintainability. Additionally, the check for _zBuffer->originalNum >= 0 suggests that there might be a special case or an invalid state for the Z-buffer that should be documented for clarity.

Comment on lines 294 to 300
}

// kill zBuffer
if (_zBuffer->originalNum >= 0 && _zBuffer->sprites) {
if (_zBuffer->originalNum >= 0 && _zBuffer->tex) {
int num = _zBuffer->originalNum;
killZBuffer();
_zBuffer->originalNum = num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, the Z-buffer killing logic is duplicated in the burnSpriteToBackDrop function. Refactoring this into a separate function would improve code maintainability.

Comment on lines 462 to 482
y2 = y1 + diffY;
}

uint8 z;

if (useZB && _zBuffer->numPanels) {
int i;
for (i = 1; i < _zBuffer->numPanels; i++) {
if (_zBuffer->panel[i] >= y + _cameraY) {
i--;
break;
}
}
z = ((i + 1) * 2) + 1;
} else {
z = 0xFF;
}

Graphics::Surface *blitted = &single.surface;
Graphics::Surface *ptr = applyLightmapToSprite(blitted, thisPerson, mirror, x, y, x1, y1, diffX, diffY);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [462-502]

The code for drawing a sprite to the zBuffer is duplicated in the scaleSprite function. This could be refactored into a separate function to improve code reuse and maintainability. Additionally, the comment on line 495 suggests an optimization that should be considered to avoid unnecessary copying of the entire render surface.

Comment on lines 34 to 43
void GraphicsManager::killZBuffer() {
if (_zBuffer->sprites) {
for (int i = 0; i < _zBuffer->numPanels; ++i) {
_zBuffer->sprites[i].free();
}
delete []_zBuffer->sprites;
_zBuffer->sprites = nullptr;

if (_zBuffer->tex) {
delete[] _zBuffer->tex;
_zBuffer->tex = nullptr;
}

_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The killZBuffer function does not check if _zBuffer is a valid pointer before dereferencing it. This could lead to a crash if _zBuffer is nullptr. It is recommended to add a check for _zBuffer at the beginning of the function.

void GraphicsManager::killZBuffer() {
+   if (!_zBuffer) {
+       return;
+   }
    if (_zBuffer->tex) {

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
void GraphicsManager::killZBuffer() {
if (_zBuffer->sprites) {
for (int i = 0; i < _zBuffer->numPanels; ++i) {
_zBuffer->sprites[i].free();
}
delete []_zBuffer->sprites;
_zBuffer->sprites = nullptr;
if (_zBuffer->tex) {
delete[] _zBuffer->tex;
_zBuffer->tex = nullptr;
}
_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}
void GraphicsManager::killZBuffer() {
+ if (!_zBuffer) {
+ return;
+ }
if (_zBuffer->tex) {
delete[] _zBuffer->tex;
_zBuffer->tex = nullptr;
}
_zBuffer->numPanels = 0;
_zBuffer->originalNum = -1;
}

Comment on lines 72 to 78

debug (kSludgeDebugGraphics, "Setting zBuffer");
uint32 stillToGo = 0;
int yPalette[16], sorted[16];
int yPalette[16], sorted[16], sortback[16];

killZBuffer();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setZBuffer function calls killZBuffer without checking if _zBuffer is valid. This could lead to a crash if _zBuffer is nullptr. The same check as suggested for killZBuffer should be applied here.

Comment on lines 94 to 114
if (readStream->readByte() != 'b')
return fatal("Not a Z-buffer file");

uint width, height;
switch (readStream->readByte()) {
case 0:
width = 640;
height = 480;
_zBuffer->width = 640;
_zBuffer->height = 480;
break;

case 1:
width = readStream->readUint16BE();
height = readStream->readUint16BE();
_zBuffer->width = readStream->readUint16BE();
_zBuffer->height = readStream->readUint16BE();
break;

default:
return fatal("Extended Z-buffer format not supported in this version of the SLUDGE engine");
}
if (width != _sceneWidth || height != _sceneHeight) {
Common::String tmp = Common::String::format("Z-w: %d Z-h:%d w: %d, h:%d", width, height, _sceneWidth, _sceneHeight);
if (_zBuffer->width != _sceneWidth || _zBuffer->height != _sceneHeight) {
Common::String tmp = Common::String::format("Z-w: %d Z-h:%d w: %d, h:%d", _zBuffer->width, _zBuffer->height, _sceneWidth, _sceneHeight);
return fatal("Z-buffer width and height don't match scene width and height", tmp);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages could be more descriptive, and the magic numbers for width and height (640 and 480) could be replaced with named constants for better readability and maintainability.

- _zBuffer->width = 640;
- _zBuffer->height = 480;
+ const uint16 DEFAULT_ZBUFFER_WIDTH = 640;
+ const uint16 DEFAULT_ZBUFFER_HEIGHT = 480;
+ _zBuffer->width = DEFAULT_ZBUFFER_WIDTH;
+ _zBuffer->height = DEFAULT_ZBUFFER_HEIGHT;

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if (readStream->readByte() != 'b')
return fatal("Not a Z-buffer file");
uint width, height;
switch (readStream->readByte()) {
case 0:
width = 640;
height = 480;
_zBuffer->width = 640;
_zBuffer->height = 480;
break;
case 1:
width = readStream->readUint16BE();
height = readStream->readUint16BE();
_zBuffer->width = readStream->readUint16BE();
_zBuffer->height = readStream->readUint16BE();
break;
default:
return fatal("Extended Z-buffer format not supported in this version of the SLUDGE engine");
}
if (width != _sceneWidth || height != _sceneHeight) {
Common::String tmp = Common::String::format("Z-w: %d Z-h:%d w: %d, h:%d", width, height, _sceneWidth, _sceneHeight);
if (_zBuffer->width != _sceneWidth || _zBuffer->height != _sceneHeight) {
Common::String tmp = Common::String::format("Z-w: %d Z-h:%d w: %d, h:%d", _zBuffer->width, _zBuffer->height, _sceneWidth, _sceneHeight);
return fatal("Z-buffer width and height don't match scene width and height", tmp);
}
if (readStream->readByte() != 'b')
return fatal("Not a Z-buffer file");
const uint16 DEFAULT_ZBUFFER_WIDTH = 640;
const uint16 DEFAULT_ZBUFFER_HEIGHT = 480;
switch (readStream->readByte()) {
case 0:
_zBuffer->width = DEFAULT_ZBUFFER_WIDTH;
_zBuffer->height = DEFAULT_ZBUFFER_HEIGHT;
break;
case 1:
_zBuffer->width = readStream->readUint16BE();
_zBuffer->height = readStream->readUint16BE();
break;
default:
return fatal("Extended Z-buffer format not supported in this version of the SLUDGE engine");
}
if (_zBuffer->width != _sceneWidth || _zBuffer->height != _sceneHeight) {
Common::String tmp = Common::String::format("Z-w: %d Z-h:%d w: %d, h:%d", _zBuffer->width, _zBuffer->height, _sceneWidth, _sceneHeight);
return fatal("Z-buffer width and height don't match scene width and height", tmp);
}

Comment on lines +164 to 188
void GraphicsManager::drawSpriteToZBuffer(int x, int y, uint8 depth, const Graphics::Surface &surface) {

outFile->open(Common::String::format("dumps/zbuffer%04d-%d.png", num, i));
Image::writePNG(*outFile, _zBuffer->sprites[i]);
outFile->finalize();
outFile->close();
delete outFile;
}
for (uint y1 = 0; y1 < (uint)surface.h; y1++) {
for (uint x1 = 0; x1 < (uint)surface.w; x1++) {
if (x1 + x >= _sceneWidth || y1 + y >= _sceneHeight) {
continue;
}

return true;
byte *target = (byte *)_renderSurface.getBasePtr(x1 + x, y1 + y);
const byte *source = (const byte *)surface.getBasePtr(x1, y1);

if (depth > _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {

if (source[0] == 0xff) {
// Completely opaque, so copy RGB values over
target[0] = 0xff;
target[1] = source[1];
target[2] = source[2];
target[3] = source[3];

}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawSpriteToZBuffer function does not check if the surface parameter is valid or if the x and y coordinates are within the bounds of _renderSurface. Additionally, the check for depth should be >= instead of > to include the case where the depth is equal to the current depth value in _zBufferSurface.

- if (depth > _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {
+ if (depth >= _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
void GraphicsManager::drawSpriteToZBuffer(int x, int y, uint8 depth, const Graphics::Surface &surface) {
outFile->open(Common::String::format("dumps/zbuffer%04d-%d.png", num, i));
Image::writePNG(*outFile, _zBuffer->sprites[i]);
outFile->finalize();
outFile->close();
delete outFile;
}
for (uint y1 = 0; y1 < (uint)surface.h; y1++) {
for (uint x1 = 0; x1 < (uint)surface.w; x1++) {
if (x1 + x >= _sceneWidth || y1 + y >= _sceneHeight) {
continue;
}
return true;
byte *target = (byte *)_renderSurface.getBasePtr(x1 + x, y1 + y);
const byte *source = (const byte *)surface.getBasePtr(x1, y1);
if (depth > _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {
if (source[0] == 0xff) {
// Completely opaque, so copy RGB values over
target[0] = 0xff;
target[1] = source[1];
target[2] = source[2];
target[3] = source[3];
}
}
}
}
}
void GraphicsManager::drawSpriteToZBuffer(int x, int y, uint8 depth, const Graphics::Surface &surface) {
for (uint y1 = 0; y1 < (uint)surface.h; y1++) {
for (uint x1 = 0; x1 < (uint)surface.w; x1++) {
if (x1 + x >= _sceneWidth || y1 + y >= _sceneHeight) {
continue;
}
byte *target = (byte *)_renderSurface.getBasePtr(x1 + x, y1 + y);
const byte *source = (const byte *)surface.getBasePtr(x1, y1);
if (depth >= _zBufferSurface[(y1 + y) * _sceneWidth + (x1 + x)]) {
if (source[0] == 0xff) {
// Completely opaque, so copy RGB values over
target[0] = 0xff;
target[1] = source[1];
target[2] = source[2];
target[3] = source[3];
}
}
}
}
}

Comment on lines 190 to 213
void GraphicsManager::drawZBuffer(int x, int y, bool upsidedown) {
if (!_zBuffer->numPanels || !_zBuffer->sprites)
if (!_zBuffer->numPanels || !_zBuffer->tex)
return;

g_sludge->_gfxMan->resetSpriteLayers(_zBuffer, x, y, upsidedown);
fillZBuffer(0);

for (uint y1 = y; y1 < _zBuffer->height + y; y1++) {
for (uint x1 = x; x1 < _zBuffer->width + x; x1++) {

uint8 z = 0;

if (upsidedown) {
z = (_zBuffer->tex[(_zBuffer->height - y1) * _zBuffer->width + x1] + 1) * 2;
} else {
z = (_zBuffer->tex[y1 * _zBuffer->width + x1] + 1) * 2;
}


if ( z > _zBufferSurface[y1 * _winWidth + x1])
_zBufferSurface[y1 * _winWidth + x1] = z;

}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawZBuffer function is using a fixed value of 2 to scale the Z-buffer values. This magic number should be replaced with a named constant or a parameter to the function to allow for more flexible usage. Additionally, the function does not check if _zBufferSurface is a valid pointer or if it has been allocated with the correct size.

@sev-
Copy link
Member

sev- commented Nov 15, 2023

Thank you!

@sev- sev- merged commit 00e72a1 into scummvm:master Nov 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants