Opened 17 years ago

Closed 17 years ago

Last modified 13 months ago

#8184 closed patch

Beneath a Steel Sky interim/initial support patch.

Reported by: joostp Owned by: fingolfin
Priority: normal Component: Engine: Sky
Keywords: Cc:
Game: Beneath a Steel Sky

Description

Diff against todays CVS (2003-03-04). (tarball ->
http://0x.7fc1.org/scummvmbass20030304.tar.gz )
Please note that this is NOT all of the old cbass code,
the remaining code (the rest of initialise(), some of intro()
and various bits and pieces) will follow soon.

Immediate fixme's include:

- detect and handle cd version (or the demo, for that
matter)
- clean up (atleast, replace the eax,ebx,.. variables with
something more descriptive.)

It should show the virgin screen if all is well. (opens
disk+dinner table, sets up screen+palette,
decompresses rnc file to buffer and displays it)
Has been tested and works with floppy demo (v0.2??)
and disk version v0.0331.

Reportedly it doesn't work with the datafiles of disk
version v0.0288, I'm looking into that.

Ticket imported from: #697312. Ticket imported from: patches/289.

Attachments (3)

scummvm-bass.diff (46.7 KB ) - added by joostp 17 years ago.
Diff against CVS of 2003-03-04
sky.patch (38.1 KB ) - added by fingolfin 17 years ago.
Cleaned up patch
skypatch2.diff (35.5 KB ) - added by joostp 17 years ago.
Version 2 of the patch against 20030304 CVS

Download all attachments as: .zip

Change History (24)

by joostp, 17 years ago

Attachment: scummvm-bass.diff added

Diff against CVS of 2003-03-04

comment:1 by fingolfin, 17 years ago

Can somebody tell me where to get data files for testing this?

comment:2 by fingolfin, 17 years ago

Uhm and note, a *patch* would be preferable. This is not a patch, this is just the whole ScummVM source, modified...

comment:3 by fingolfin, 17 years ago

I dunno which OS you use for it, but here are hints on how to properly submit this as a patch:

1) When you are ready, run "make distclean" to remove all generated files (includes .deps dirs for example)

2) Put the added files (sky/ only I hope ?) into a tar ball and attach that

3) For the rest, the modification should be in a .patch file. If you are having access to command line CVS, you can generate one via "cvs diff"...

That's for future patches. For now I'll try to convert this into the appropriate format and attach it to this item.

Note to all: I would appreciate if nobody checks this stuff in yet before we had time to review it. I'd like to have at least Endy and me agree on commiting it. E.g. the question I immediatly asked myself was: should this be "sky" or "bass" for the new engine dir ??

by fingolfin, 17 years ago

Attachment: sky.patch added

Cleaned up patch

comment:4 by fingolfin, 17 years ago

Gah quite obviously I am stupid, cause I only saw the tarball link, not the attached .diff! Doh!

Still, the diff needed to be cleaned up, there were all sorts of things in it that we don't really want... e.g. the "make distclean" comment still holds. Attached is a clean patch which I derived from the tarball.

comment:5 by joostp, 17 years ago

I am sorry for gobbled patch, I was under the impression
that "make clean" would suffice. sorry for the inconvience.

The demo datafiles can be found at: http://bass.revfans.com
(or directly at:
http://www.ismeonline.net/steelsky/downloads/skydemo.zip)

comment:6 by fingolfin, 17 years ago

OK here a list of first "complaints". Please don't get me wrong I much appreicate your efforst and work, it's just normal that I'll talk about what I find not optimal while not talking about the things which are good :-)

Naming an include file "include.h" is, uhm, not very descriptive. I am sure a better name can be found, at the very least use intern.h to parallel the other modules.

There is a warning which should be fixed:
sky/disk.cpp:137: warning: unsigned int format, pointer arg (arg 4)
(fixed by casting file_dest to int, though I fail to see how printing a pointer is useful debug info)

Use the debug() method instead of this code:
if (_debugLevel > 1) printf(....)

No idea what editor you use, but I almost fear it's emacs... in that case, could you please set its settings to use a more sane indention mode... using an indention level of 4 with tabs of width 8 is, uhm, "annoying" to say the least. If you use a 4 level indention, then use 4-tabs. This way, the code will be mostly readable even for people with 2,3,4 or 8 wide tabs; with the current indention, the code only looks right if one has the exact right tab settings.

comment:7 by joostp, 17 years ago

Thanks for the feedback.

Here's some clarification on your points:

- The "include.h" is named that way because it mimics the
original source's "include.asm", you're right the name is non-
descriptive and should be replaced with something else.

- The indentation, in some last minute editing sessions I used
a combination of editors (some crappy, apparantly) which
might have messed some things up. Again, sorry for the
inconvience and your points about tab-size are noted.

- The printing of the pointer is old code and can be removed.
(was used to check for null pointers).

comment:8 by joostp, 17 years ago

Here's a new patch, properly done this time.
I don't appear to have the rights to delete the old one, but
please forget about that one.

Here's what's new since the old one:

- renamed "include.h" to "skydefs.h"
- fixed palette conversion 0..252 -> 0..255
- now using File class for file access, should solve a file
loading bug on Big Endian system.
- now uses the debug() function instead of if (_debugLevel)
printf("aa");
- cd versions should also work now, as it allocates the dinner
table according to the amount of entries.

Still doesn't work with v0.0288 (old floppy version) of the
datafiles (it has 1404 dinner table entries) the palette
appears to be a different file #. (?)

by joostp, 17 years ago

Attachment: skypatch2.diff added

Version 2 of the patch against 20030304 CVS

comment:9 by SF/khalek, 17 years ago

The patch fingolfin has supplied has surplus CRs in it and
fails to actually load the virgin screen.

While joost's patch although maybe not endian safe and
including .deps which it shouldn't does actual load the
virgin screen here...

I believe using the name of sky is fine as that is the name
of the original binary and the basename of all the resource
files.

As I mentioned to joost earlier ideally this would also use
the file class etc

Besides that hmm

- I think the main in skymain.h/cpp can be dropped as it
would be rather obvious it is the main file in the module
and it doesn't include a main() in the traditional sense anyway.

- Should probably use the same style braces as
recommened/used in the rest of ScummVM

- Should have $Header$ in the headers of files in a similiar
manner as the rest of ScummVM

Obviously the sooner its in cvs the easier it is for people
to address any issues

comment:10 by SF/khalek, 17 years ago

ah must have just missed this new patch/comment oh well

yes the cd version does indeed work with this latest patch

comment:11 by fingolfin, 17 years ago

Yes khalek, the soon it's in, the better, but about the single one thing I would like to have right before a checkin are file names. Changing file contents is easy, renaming files in CVS is a PITA.

I'll give this a try.

comment:12 by fingolfin, 17 years ago

On and regardin $Header$ : personally I'd prefer if we removed it from all the other files, instead of introducing it here. It forces me to recompile files when I check them in, and I don't see why it would serve any real purpose to keep the $Header$ <shrug>.

comment:13 by fingolfin, 17 years ago

Owner: set to SF/ender

comment:14 by fingolfin, 17 years ago

I second khalek, renaming skymain -> sky makes more sense & would match the other modules (scumm/scumm.h and simon/simon.h).

Assigning this to Ender to make sure he keeps track.

Anyway, testing it on my BE machine, It gets a bit further:
Trying to start game 'Beneath a Steel Sky'
Entries in dinner table: 247
load file 29,719 (60111)
file 60111 found!
load file 29,718 (60110)
file 60110 found!
File is compressed...
but not with RNC! (?!)

The cause is obvious (don't fread structs, unless you use some #pragma or so to ensure the alignment is correct; and you also have to adjust the struct members to match the endianess.

But we can fix endian issues, the indentation etc. after we checked this in... so IMHO, once the files are renamed, and Ender has approved it, we can add you to the project and commit this... Ender?

comment:15 by SF/khalek, 17 years ago

$Header$ is useful to know which revision of a file you
have, and its only the committer who has to recompile
when they otherwise wouldn't have to.

Should we set a deadline for endy to look at this? he
doesn't seem to be around all that much these days...
I guess he needs to be involved to give joost commit
access though.

comment:16 by fingolfin, 17 years ago

Yes Endy needs to be involved, at the very least to add joost to the project.

Regarding $Header$... I don't understand your justification. I know what revision a file has by doing a "cvs status" on it. Not by looking at it. Looking at the $Header$ doesn't tell me if the file is modified, either. Nor does it tell me about (sticky) tags etc.

comment:17 by SF/ender, 17 years ago

Owner: changed from SF/ender to fingolfin

comment:18 by SF/ender, 17 years ago

Just looked over it - fine by me, if you want to commit with
those filename changes.

As for $header - please leave it :)
I use several cvs-related tools that use the revision numbers
in the $header for quick grepping and diffing.

joostp added to developers. Fingolfin promoted to Project
Admin. Feel free to add yourself to the credits in the readme
file, Joost.

Apologies for my absence recently, everyone - jobhunting
and related tasks is taking it's toll, I'm getting quite quite broke
right now. And I want to work on something new, which means
I need people to help with MM/Zak R.E :)

comment:19 by fingolfin, 17 years ago

Renamed skymain.* -> sky.* and commited the patch (+ one endian fix in it).
I also added $Header$ (if Ender does use it for some tools, then that's a reasonable justification I guess).

Next step should be to clean up the indention and brace style. Welcome aboard, Joost :-)

comment:20 by fingolfin, 17 years ago

Status: newclosed

comment:21 by digitall, 13 months ago

Component: Engine: Sky
Game: Beneath a Steel Sky
Note: See TracTickets for help on using tickets.