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

Blazing Dragons PS1 #2044

Merged
merged 221 commits into from Feb 25, 2020
Merged

Blazing Dragons PS1 #2044

merged 221 commits into from Feb 25, 2020

Conversation

@yuv422
Copy link
Member

yuv422 commented Feb 5, 2020

This PR adds support for the game Blazing Dragons (PS1)

The game is completable with this engine now. There are still a few things to implement though and lots of little bugs to squash.

I'm putting this PR up to get some feedback about the engine in its current state.

TODO

  • lots of renaming, removing gotos etc.
  • music support
  • sfx modulation support
  • better support for mini game input handling
  • Intro video
@yuv422 yuv422 changed the title Dragons Blazing Dragons PS1 Feb 5, 2020
@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 6, 2020

This is good, but is sad from the same time. As I see, it looks that you reversed it from scratch. And we have an almost finished engine created by john_doe :/

@yuv422

This comment has been minimized.

Copy link
Member Author

yuv422 commented Feb 6, 2020

This is good, but is sad from the same time. As I see, it looks that you reversed it from scratch. And we have an almost finished engine created by john_doe :/

I've had a look at john doe's version and it is missing a lot. There is a lot of hard coded logic (all the specialOpcodes + minigames) which isn't present in that version. I did use the path finder logic from his version though and the sprite scaling logic.:)

Copy link
Member

sev- left a comment

I started reviewing the code, and there are portability, code formatting issues.

Please address those, so I could continue my review.

* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
*/
#include <common/textconsole.h>

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

These must be in double quotes, not the angular ones

VabSound::VabSound(Common::SeekableReadStream *msfData, const DragonsEngine *_vm): _toneAttrs(NULL), _vbData(NULL) {
loadHeader(msfData);

auto dataSize = msfData->size() - msfData->pos();

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

We do not use auto yet. Please rework

error("Invalid VAB file");
}
// TODO: is sizeof(array) the right thing to do here?
vhData->read(&_programAttrs, sizeof(_programAttrs));

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

This is absolutely not portable. You must read it by individual members, otherwise, you end up with alignment and endianness issues.


const int numTones = 16 * _header.numPrograms;
_toneAttrs = new VabToneAttr[numTones];
vhData->read(_toneAttrs, sizeof(VabToneAttr) * numTones);

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Same thing. You have to do stuff like:

  _toneAttrs.field1 = vhData->readUint16LE();
  _toneAttrs.field2 = vhData->readByte();
vhData->read(_toneAttrs, sizeof(VabToneAttr) * numTones);

uint16 tempOffsets[0x100];
vhData->read(tempOffsets, sizeof(tempOffsets));

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Even this has to be read with readUint16LE(), otherwise you end up with wrong endianness, say, on ARM.

}
// 0x8001bcc8
int direction = startMoveToPoint(_walkDestX, _walkDestY);
if(direction != -1 && !isFlagSet(ACTOR_FLAG_800)) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Same problem with code formatting. Please, review the whole source.

uint16 actorFileDictionaryIndex;
int16 resourceID;
byte *_seqCodeIp;
void* frame_pointer_maybe;

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Code formatting is wrong. Should be void *frame_pointer_maybe;, just as the line above.

debug("Frame Count: %d", _framesCount);

_frames = new ActorFrame[_framesCount];
for (int i=0; i < _framesCount; i++) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Code formatting is incorrect.

return &_frames[frameNumber];
}

byte *ActorResource::getSequenceData(int16 sequenceId)

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

Code formatting for the curly brace is incorrect here.


} // End of namespace Dragons

#endif //DRAGONS_ACTORRESOURCE_H

This comment has been minimized.

Copy link
@sev-

sev- Feb 6, 2020

Member

This is where I stopped reviewing.

Copy link
Member

sev- left a comment

The amount of small issues is so big, that it looks it will take much more time from me to describe them than to fix them.

If you don't mind, please give me permissions to your fork, so I could go and just fix those formatting, naming things and leave you with the real logic stuff like thos dialog rewrites.

}

void VabSound::loadProgramAttributes(Common::SeekableReadStream *vhData) {
for (int i = 0; i < DRAGONS_VAB_NUM_PROG_ATTRS; i++) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

Indentation is wrong, spaces instead of the tabs

void PriorityLayer::overlayTileMap(byte *data, int16 x, int16 y, int16 w, int16 h) {
byte *ptr = _map + (x + y * _mapWidth) * 2;
byte *src = data;
for (int i=0;i < h; i++) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

Code formatting, and in the loops below.

_layerSurface[0] = NULL;
_layerSurface[1] = NULL;
_layerSurface[2] = NULL;
layerOffset[0] = Common::Point(0,0);

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

This is so confusing to see this mix of naming. All the class variables must start with an underscore, so they could be easily distinguished from the local vars.

pal[1] = 0x0;
for (int i = 1; i < 0x100; i++) {
uint c = READ_LE_INT16(&pal[i * 2]);
if ( c == 0 ) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

Code formatting is wrong here

#include "dragons/bigfile.h"

namespace Dragons {
typedef struct FileInfo {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

It looks like a C-style declaration

int16 tileX = flicker->actor->x_pos / 32;
int16 tileY = flicker->actor->y_pos / 8;

for (int i=0;i < _dragonINIResource->totalRecords(); i++) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

Thhs is thhe same story of i=0 vs i = 0

}

void DragonsEngine::gameLoop()
{

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

And the opening bracket is not hanging.

if (_dragonINIResource->getFlickerRecord()->sceneId == getCurrentSceneId()) {
uVar3 = ipt_img_file_related();
actorId_00 = uVar3 & 0xffff;
if (actorId_00 == 0) goto LAB_80026d34;

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

omg.

This comment has been minimized.

Copy link
@yuv422

yuv422 Feb 7, 2020

Author Member

yeah there's a fair bit to clean up here.

actorId = 0;

while (!shouldQuit()) {
_scene->draw();

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

This whole thing looks like raw decompilation.

if ((_cursor->_sequenceID != 4) && (_cursor->_sequenceID != 2)) {
_cursor->data_800728b0_cursor_seqID = _cursor->_sequenceID;
_cursor->data_80072890 = _cursor->_iniUnderCursor;
if (4 < _cursor->_sequenceID) {

This comment has been minimized.

Copy link
@sev-

sev- Feb 7, 2020

Member

l33t c0de.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 7, 2020

Okay, I did some fixes, so now I could continue the real review, without distracting on the formatting or naming issues.

@yuv422

This comment has been minimized.

Copy link
Member Author

yuv422 commented Feb 7, 2020

Great thanks for that @sev-


Screen::Screen() {
_pixelFormat = Graphics::PixelFormat(2, 5, 5, 5, 1, 10, 5, 0, 15);
initGraphics(320, 200, &_pixelFormat);

This comment has been minimized.

Copy link
@bgK

bgK Feb 8, 2020

Member

You will probably want to use a more common pixel format for the screen surface. Some backends may not recognize this one and refuse to start the game. Some others will have to to color conversion in software, which is likely to cause slowness on low performance devices.

This comment has been minimized.

Copy link
@yuv422

yuv422 Feb 8, 2020

Author Member

Good point, any recommendation on which one to use? I picked this because it aligns with the PS1 format used by the original game engine.

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

RGB565 is a pretty good choice. It's required to be supported by spec in all versions of OpenGL (including ES) so most devices have it.
RGB555 (without the alpha bit) is used by some engines as well, but is not always accelerated by the hardware.

This comment has been minimized.

Copy link
@yuv422

yuv422 Feb 10, 2020

Author Member

Hmm I can convert to rgb565 but it will require a bit of rework. I'm using the palette in rgb555+A in a few places. I wonder if just removing the alpha from this format would help? I'm not actually using the alpha bit here anyway.

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

Yes, using RGB555 (the same you use without the alpha bit) as the screen surface would work on most platforms. It's just more likely to trigger software color conversion in the backend than RGB565.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 23, 2020

OKay, it is time to merge the engine and continue in-tree. I'll do it tonight. Could you please send me a couple of nice screenshots, so I'll add a post on the FB.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 23, 2020

Merging

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 23, 2020

ah, wait. there are conflicts now :/

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 23, 2020

Could you please rebase it to the current master? GitHub got confused with it for some reason.

I tried to rebase it, and there is something wrong with the history, so it bails out pretty early.

You pull your repo and do git rebase master

@dreammaster

This comment has been minimized.

Copy link
Member

dreammaster commented Feb 23, 2020

If you're going to do a rebase anyway, you might want to add in the ", bool isAutosave" parameter that's now required for the saveGameState method. Also a good idea to use the "override" keyword instead of just virtual, so that problems like this are detected.

Copy link
Member

sev- left a comment

There are also some merge commits, but those will go away once rebased

@yuv422

This comment has been minimized.

Copy link
Member Author

yuv422 commented Feb 23, 2020

Thanks, I'll fix the conflicts shortly.

@yuv422

This comment has been minimized.

Copy link
Member Author

yuv422 commented Feb 23, 2020

Ok I've merged in master and fixed the overrides for the engine class.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 24, 2020

Not merge, rebase. Then you will see those conflicts. And with this merge, you added yet another merge commit. Please remove all of them.

@yuv422

This comment has been minimized.

Copy link
Member Author

yuv422 commented Feb 24, 2020

ok I'll give the rebase a go. I'm not very confident with git branch management so I tend to go for the simple solution. I can understand that a merge isn't the cleanest solution though. Sorry about that. :(

sev- and others added 26 commits Feb 7, 2020
@sev- sev- force-pushed the yuv422:dragons branch from caff61f to e743784 Feb 25, 2020
@sev-

This comment has been minimized.

Copy link
Member

sev- commented Feb 25, 2020

Thanks, merging.

@sev- sev- merged commit 0c12087 into scummvm:master Feb 25, 2020
0 of 2 checks passed
0 of 2 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.