#8184 closed patch
Beneath a Steel Sky interim/initial support patch.
| Reported by: | joostp | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Sky |
| Version: | 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)
Change History (24)
by , 22 years ago
| Attachment: | scummvm-bass.diff added |
|---|
comment:2 by , 22 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 , 22 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 ??
comment:4 by , 22 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 , 22 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 , 22 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 , 22 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 , 22 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 #. (?)
comment:9 by , 22 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 , 22 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 , 22 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 , 22 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 , 22 years ago
| Owner: | set to |
|---|
comment:14 by , 22 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 , 22 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 , 22 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 , 22 years ago
| Owner: | changed from to |
|---|
comment:18 by , 22 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 , 22 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 , 22 years ago
| Status: | new → closed |
|---|
comment:21 by , 6 years ago
| Component: | → Engine: Sky |
|---|---|
| Game: | → Beneath a Steel Sky |
Diff against CVS of 2003-03-04