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

WAGE: Various bug fixes and Improvements #4148

Merged
merged 43 commits into from Aug 5, 2022
Merged

Conversation

StableSteady
Copy link
Contributor

@StableSteady StableSteady commented Jul 30, 2022

No description provided.

Copy link
Member

@sev- sev- left a comment

Needs some changes. The major one is the sudden dependency of graphics/macgui code from engines/wage. This must be untangled.

@@ -372,6 +433,13 @@ void Design::drawPolygon(Graphics::ManagedSurface *surface, Common::ReadStream &
xcoords.push_back(x1);
ycoords.push_back(y1);

if (borderThickness > 1) {
for (int i = 0; i < xcoords.size(); ++i) {
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

please use uint here

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done


byte *pat = p->patterns->operator[](p->fillType - 1);

if (p->thickness == 1) {
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Please add comment here, something like

// Draw circle when thickness is > 1, put a pixel otherwise

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done

(pat[yu % 8] & (1 << (7 - xu % 8))) ? color : kColorWhite;
}
} else {
int x1 = x - p->thickness / 2;
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

What happens when the thickness is odd, e.g. 3, 5, etc? Could you please test and see if it rounds up or down?

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

My test already had all possible thickness and this behavior works for them.

if (borderThickness > 0 && borderFillType <= patterns.size())
Graphics::drawEllipse(x1, y1, x2-1, y2-1, kColorBlack, false, drawPixel, &pd);
Graphics::drawEllipse(x1, y1, x2-1, y2-1, kColorBlack, false, drawPixelCircle, &pd);
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Please write x2 - 1, y2 - 1. E.g. spaces around operators

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done

@@ -159,6 +159,13 @@ void Scene::paint(Graphics::ManagedSurface *surface, int x, int y) {
Designed *Scene::lookUpEntity(int x, int y) {
for (ObjList::const_iterator it = _objs.end(); it != _objs.begin(); ) {
it--;
if (((*it)->_name == "continue" && (*it)->_resourceId == 22259) ||
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Please add a comment explaining the reason here.

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done

@@ -215,6 +215,9 @@ bool World::loadWorld(Common::MacResManager *resMan) {
if (res != NULL) {
scene->_textBounds = readRect(res);
int fontType = res->readUint16BE();
if (_name == "***DUNE ETERNITY*** ")
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Please add explanation in a comment why is this needed

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done

x1 = in->readSint16BE();
y2 = in->readSint16BE() + 4;
x2 = in->readSint16BE() + 4;
y1 = in->readSint16BE() - 2;
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Please explain in a comment why we're doing this

Copy link
Contributor Author

@StableSteady StableSteady Aug 2, 2022

Choose a reason for hiding this comment

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

Done

#include "common/events.h"
#include "common/system.h"

//#include "engines/engine.h"
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Maybe remove it altogether?


Design::drawFilledRect(&_gui->_screen, bb, kColorBlack, _gui->_wm->getPatterns(), kPatternSolid);
Wage::Design::drawFilledRect(&_gui->_screen, bb, kColorBlack, _gui->_wm->getPatterns(), kPatternSolid);
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

Err, how Graphics:: could call a specific engine? Please refactor, the common code cannot refer to any engine

#include "common/system.h"

//#include "engines/engine.h"
#include "engines/wage/design.h"
Copy link
Member

@sev- sev- Jul 31, 2022

Choose a reason for hiding this comment

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

This should never be included here.

StableSteady added 28 commits Aug 2, 2022
Copy link
Member

@sev- sev- left a comment

Getting there. Added couple more notes

@@ -194,6 +194,24 @@ bool Design::isPointOpaque(int x, int y) {
return pixel != kColorGreen;
}

bool Design::isInBounds(int x, int y) {
if (_surface == NULL)
error("Surface is null");
Copy link
Member

@sev- sev- Aug 4, 2022

Choose a reason for hiding this comment

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

I'd suggest writing "Design::isInBounds(): Surface is null"

Copy link
Contributor Author

@StableSteady StableSteady Aug 5, 2022

Choose a reason for hiding this comment

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

Done

@@ -215,6 +215,10 @@ bool World::loadWorld(Common::MacResManager *resMan) {
if (res != NULL) {
scene->_textBounds = readRect(res);
int fontType = res->readUint16BE();
// WORKAROUND: Dune Eternity has a weird fontType ID so we override it to the correct one
Copy link
Member

@sev- sev- Aug 4, 2022

Choose a reason for hiding this comment

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

Could you please specify what is this "weird type ID"

Copy link
Contributor Author

@StableSteady StableSteady Aug 5, 2022

Choose a reason for hiding this comment

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

The fontType it reads is 14614 which is way outside of the range of font Types supported. So I hardcoded it to what it should be.

1, kColorBlack, _gui->_wm->getPatterns(), kPatternSolid);
}

int Dialog::run() {
Copy link
Member

@sev- sev- Aug 4, 2022

Choose a reason for hiding this comment

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

I wonder, why is this running on an engine level but is not here? You may just return different things like "should quit" if EVEN_QUIT is set and kDialogOption1/kDialogOption2 for the requested options

@sev-
Copy link
Member

@sev- sev- commented Aug 5, 2022

Thanks!

@sev- sev- merged commit 4f3dc24 into scummvm:master Aug 5, 2022
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