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

STARTREK: New engine (WIP) #1263

Merged
merged 198 commits into from Aug 9, 2018

Conversation

Projects
None yet
5 participants
@Drenn1
Contributor

Drenn1 commented Jul 28, 2018

This adds support for Star Trek: 25th anniversary (only DOS version has been tested so far).

The first 6 of 7 away missions are playable. The sections on the enterprise with pseudo-3d ship combat are not yet implemented, so it currently jumps directly from one away mission to the next. Players will be missing some pre-mission context until this is implemented.

clone2727 and others added some commits Feb 20, 2011

STARTREK: Fixes to file loading.
The "numbered" files that I implemented can also use letters as the last
character.
STARTREK: Begin implementation of showText.
Also started using SharedPtrs.
STARTREK: Implement drawAllSprites properly.
Still need to do the part which calculates sprite priorities.
STARTREK: Begin implementing event system.
Textbox now responds to mouse input.
STARTREK: Implement sound effects in the midis
Removed amiga and mac sound code for now since I can't test it.
STARTREK: Objects and animations.
Testing them with the transporter room.
STARTREK: Initialization of away mission.
Crew successfully beams in and does their idle animation.
STARTREK: Refactor text and menus
Moved them out of the Graphics class and into their own files.
@Drenn1

This comment has been minimized.

Show comment
Hide comment
@Drenn1

Drenn1 Jul 28, 2018

Contributor

Thanks for the feedback. I've replaced AD_ENTRY1 with AD_ENTRY1s for those versions that I have. Most were added by clone2727 a decade ago! I'll update the other versions as I add support for them.

Contributor

Drenn1 commented Jul 28, 2018

Thanks for the feedback. I've replaced AD_ENTRY1 with AD_ENTRY1s for those versions that I have. Most were added by clone2727 a decade ago! I'll update the other versions as I add support for them.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Jul 28, 2018

Member

Then I recommend to remove/comment out those entries, so the users will be able to submit them. Also, I'll take a look at my archives in couple of weeks, perhaps I will be able to provide the file sizes.

Member

sev- commented Jul 28, 2018

Then I recommend to remove/comment out those entries, so the users will be able to submit them. Also, I'll take a look at my archives in couple of weeks, perhaps I will be able to provide the file sizes.

@sev-

I skimmed through rest of the engine (skipping most of the room logic, as they're pretty simple), and made few more comments.

In general, it is in a pretty solid shape.

What comes to my mind, is that you perhaps need already to think about localized versions: how to extract text from them and how to select the proper texts based on the game language.

@@ -0,0 +1,52 @@
#ifndef STARTREK_BITMAP_H

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

This file is missing ScummVM license header.

@sev-

sev- Jul 28, 2018

Member

This file is missing ScummVM license header.

Show outdated Hide outdated engines/startrek/detection.cpp
uint8 gameType;
uint32 features;
bool isCDEdition;

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

I highly recommend using GF_ for that.

@sev-

sev- Jul 28, 2018

Member

I highly recommend using GF_ for that.

@@ -0,0 +1,80 @@
#include "startrek/filestream.h"

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

This file is missing ScummVM copyright header

@sev-

sev- Jul 28, 2018

Member

This file is missing ScummVM copyright header

Show outdated Hide outdated engines/startrek/filestream.cpp
// ReadStream functions
uint32 FileStream::read(void *dataPtr, uint32 dataSize) {

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

This should be dropped completely for the sake of MemoryReadStreamEndian. The functionality is the same as I see.

@sev-

sev- Jul 28, 2018

Member

This should be dropped completely for the sake of MemoryReadStreamEndian. The functionality is the same as I see.

This comment has been minimized.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

This was originally used like a stream, and still sometimes is, but in certain places I actually read from the byte array directly, so extending MemoryReadStreamEndian doesn't quite work. It's not purely a "stream" anymore so perhaps it should be renamed.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

This was originally used like a stream, and still sometimes is, but in certain places I actually read from the byte array directly, so extending MemoryReadStreamEndian doesn't quite work. It's not purely a "stream" anymore so perhaps it should be renamed.

This comment has been minimized.

@sev-

sev- Aug 2, 2018

Member

You may extend MemoryReadStream() with getData() method. This is better than reimplementing the whole thing. Could you please point me to where do you read from the array directly?

@sev-

sev- Aug 2, 2018

Member

You may extend MemoryReadStream() with getData() method. This is better than reimplementing the whole thing. Could you please point me to where do you read from the array directly?

This comment has been minimized.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Turns out it's only it one spot, the "StarTrekEngine::getLoadedText" function, since I'm reading from it as a string (char array).

In terms of a "getData" method, it seems that I'll need to make a second copy of the file in memory to access it that way, since there seems to be no direct way to access the MemoryReadStream's data. In this particular case I could just copy the portion of the string I need, which isn't much overhead, though still technically unnecessary.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Turns out it's only it one spot, the "StarTrekEngine::getLoadedText" function, since I'm reading from it as a string (char array).

In terms of a "getData" method, it seems that I'll need to make a second copy of the file in memory to access it that way, since there seems to be no direct way to access the MemoryReadStream's data. In this particular case I could just copy the portion of the string I need, which isn't much overhead, though still technically unnecessary.

This comment has been minimized.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Oh, nevermind, I can keep track of the initial byte array I pass to the MemoryReadStream. Yeah, I'll do that...

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Oh, nevermind, I can keep track of the initial byte array I pass to the MemoryReadStream. Yeah, I'll do that...

_numSprites = 0;
_pushedNumSprites = -1;
_palData = new byte[256 * 3];

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

I think, here we have a memory leak. You're not freeing it anywhere.

@sev-

sev- Jul 28, 2018

Member

I think, here we have a memory leak. You're not freeing it anywhere.

Show outdated Hide outdated engines/startrek/object.h
public:
Actor() :
spriteDrawn(),

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

Shouldn't a proper initializarion go here?

@sev-

sev- Jul 28, 2018

Member

Shouldn't a proper initializarion go here?

This comment has been minimized.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

It should be calling the default constructors / setting default values for primitive types (0), but I can make that explicit.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

It should be calling the default constructors / setting default values for primitive types (0), but I can make that explicit.

Show outdated Hide outdated engines/startrek/room.cpp
#define ADD_ROOM_OLD(ROOM) {\
if (name.equalsIgnoreCase(#ROOM)) {\
_roomActionList = ROOM##ActionList;\
_numRoomActions = sizeof(ROOM##ActionList) / sizeof(RoomAction);\

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

should be replaced with ARRAYSIZE()

@sev-

sev- Jul 28, 2018

Member

should be replaced with ARRAYSIZE()

delete[] _rdfData;
}
uint16 Room::readRdfWord(int offset) {

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

replace this function with FROM_LE_16() macro

@sev-

sev- Jul 28, 2018

Member

replace this function with FROM_LE_16() macro

This comment has been minimized.

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Do you mean READ_LE_UINT16()?

@Drenn1

Drenn1 Aug 2, 2018

Contributor

Do you mean READ_LE_UINT16()?

This comment has been minimized.

@sev-

sev- Aug 2, 2018

Member

That could also work.

@sev-

sev- Aug 2, 2018

Member

That could also work.

Common::String desc;
int slot;
dialog = new GUI::SaveLoadChooser(_("Save game:"), _("Save"), true);

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

Then you need to add file POTFILES to your engine

@sev-

sev- Jul 28, 2018

Member

Then you need to add file POTFILES to your engine

return (char *)data;
}
} // End of namespace StarTrek

This comment has been minimized.

@sev-

sev- Jul 28, 2018

Member

This file is quite big. I recommend to move the methods to different files (leaving them in StarTreckEngine class): intro.cpp, inventory.cpp, animation.cpp, actors.cpp, or whatever else makes sense.

@sev-

sev- Jul 28, 2018

Member

This file is quite big. I recommend to move the methods to different files (leaving them in StarTreckEngine class): intro.cpp, inventory.cpp, animation.cpp, actors.cpp, or whatever else makes sense.

@Drenn1

This comment has been minimized.

Show comment
Hide comment
@Drenn1

Drenn1 Jul 28, 2018

Contributor

In terms of localization: text is dumped with an external tool (https://github.com/drenn1/startrek25_rtools) which, in most cases, should also work with the other languages. Since all languages use the same english audio files (which the text is named by), it should be possible to simply put the other language strings into their own arrays and modify showText to choose the correct one, with just a bit of manual intervention for the strings that don't have audio files.

However, the text dumping program won't work on the floppy versions, since it relies on looking for the distinctive pattern of the audio filename to identify strings; if I want to support changes made between the floppy and CD versions, that will be a bit more work.

Contributor

Drenn1 commented Jul 28, 2018

In terms of localization: text is dumped with an external tool (https://github.com/drenn1/startrek25_rtools) which, in most cases, should also work with the other languages. Since all languages use the same english audio files (which the text is named by), it should be possible to simply put the other language strings into their own arrays and modify showText to choose the correct one, with just a bit of manual intervention for the strings that don't have audio files.

However, the text dumping program won't work on the floppy versions, since it relies on looking for the distinctive pattern of the audio filename to identify strings; if I want to support changes made between the floppy and CD versions, that will be a bit more work.

}
FileStream::~FileStream() {
free(_data);

This comment has been minimized.

@sev-

sev- Aug 3, 2018

Member

Instead of doing this, I suggest to extend MemoryReadStreamEndian class constructor with the optional "disposeMemory" flag, just as the MemoryReadStream does, pass it to MemoryReadStream, and kill this FileStream class altogether.

@sev-

sev- Aug 3, 2018

Member

Instead of doing this, I suggest to extend MemoryReadStreamEndian class constructor with the optional "disposeMemory" flag, just as the MemoryReadStream does, pass it to MemoryReadStream, and kill this FileStream class altogether.

This comment has been minimized.

@Drenn1

Drenn1 Aug 3, 2018

Contributor

I can do that, but as I'm still using the "_data" field, I'd prefer to still keep this class unless you feel strongly about it.

@Drenn1

Drenn1 Aug 3, 2018

Contributor

I can do that, but as I'm still using the "_data" field, I'd prefer to still keep this class unless you feel strongly about it.

{
"st25",
"Floppy",
AD_ENTRY1("data.000", "f0918b6d096455ce2ae6dd5ef973292e"),

This comment has been minimized.

@sev-

sev- Aug 3, 2018

Member

Please, comment these out, so we could collect the filesizes. Users will be able to report them.

@sev-

sev- Aug 3, 2018

Member

Please, comment these out, so we could collect the filesizes. Users will be able to report them.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 3, 2018

Member

Only couple things left to do: detection entries and that FileStream class, and I will merge the engine in.

Member

sev- commented Aug 3, 2018

Only couple things left to do: detection entries and that FileStream class, and I will merge the engine in.

@sev- sev- merged commit 5f1f19b into scummvm:master Aug 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 9, 2018

Member

Merging. Please, continue your work in-tree

Member

sev- commented Aug 9, 2018

Merging. Please, continue your work in-tree

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 9, 2018

Member

buildbot is not happy for many ports. Please, take care of it ASAP. I made sure it compiles on macOS, but there are tons of warnings, all of those must be addressed. See the buildbot logs:

http://buildbot.scummvm.org/grid?category=changes;category=master

Member

sev- commented Aug 9, 2018

buildbot is not happy for many ports. Please, take care of it ASAP. I made sure it compiles on macOS, but there are tons of warnings, all of those must be addressed. See the buildbot logs:

http://buildbot.scummvm.org/grid?category=changes;category=master

@Strangerke

This comment has been minimized.

Show comment
Hide comment
@Strangerke

Strangerke Aug 9, 2018

Member

I'll give a hand this evening, but I can't help while I'm at work.

Member

Strangerke commented Aug 9, 2018

I'll give a hand this evening, but I can't help while I'm at work.

@Drenn1

This comment has been minimized.

Show comment
Hide comment
@Drenn1

Drenn1 Aug 9, 2018

Contributor

Thanks for merging. I'll try to get it fixed, but I don't have push access yet.

Contributor

Drenn1 commented Aug 9, 2018

Thanks for merging. I'll try to get it fixed, but I don't have push access yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment