Jump to content
Search In
  • More options...
Find results that contain...
Find results in...
Sign in to follow this  
Worst

Doom picture format parsing trouble(solved)

Recommended Posts

Well first off,

I havent ever really messed around with binary files, let alone any image files. (which might be easy to see looking at my code)

The doom picture format seemed fairly simple, so I started writing this tool that converts it to something..

I managed to read succesfully the width,height,left offset and top offset of a sprite lump..

Output:

SHELA0.lmp said:

width is: 15
height is: 7
left is: 5
top is: 7
column n.1 start_offset:5242880
column n.2 start_offset:6029312
column n.3 start_offset:6815744
column n.4 start_offset:6881280
column n.5 start_offset:7667712
column n.6 start_offset:8454144
column n.7 start_offset:9240576
column n.8 start_offset:9306112
column n.9 start_offset:10092544
column n.10 start_offset:10878976
column n.11 start_offset:11665408
column n.12 start_offset:11730944
column n.13 start_offset:12517376
column n.14 start_offset:13303808
column n.15 start_offset:117440512

However the column_array offsets (4 byte long int) values exceed the size of the file a lot, and I cant figure out what I did wrong.. the code seems alright to me :S

Link: http://pastebin.com/VfV7UQAZ


I used both the doom wiki, and the Unofficial doom specs as help.


Any advice would be appreciated!

Share this post


Link to post

Edit:

column_array = new unsigned long int [width];

Shouldn't these be signed integers?

Share this post


Link to post

First off, you need to modify your code not to assume that unsigned long is 4 bytes, because on GCC 64-bit this is false, and it will make your code unportable.

Second, turn this:

for(int i = 0; i < width; i++)
{
   memPos += 4;//move 4 bytes
   memmove (&column_array[i],memblock+memPos,4); //start offsets for each column			
   cout << "column n." << i+1 << " start_offset:" << column_array[i] << endl;
   outfile << "column n." << i+1 << " start_offset:" << column_array[i] << endl;
}
Into this:
memcpy(column_array, memblock+memPos, width*sizeof(int));
The memPos += 4 is wrong because it will increment 4 past the start of the columnofs array before reading the first one; the rest is simply suboptimal via doing a bunch of memmoves rather than just a single memcpy.

I didn't read the rest of it yet. Try this change first and see if it fixes anything. BTW to read a single byte of the data you don't need to use memmove. You can do *(memblock+memPos), assuming memblock is char * or unsigned char * (and that is also a bad assumption to make, that char is a one-byte type, but it is currently a safe one to make on every compiler I've used).

Share this post


Link to post

Actually it looks like the issue is that you're forgotten a final memPos += 2; after you read the top offset, prior to the columns array.

Share this post


Link to post

Got it working now.

When reading the values for the column array, memPos had indeed a bad offset. I forgot a memPos += 2; after the top offset, and should have had the memPos += 4; only after reading the column offsets. (so it skipped 2 bytes from the start of the array, and went 2 bytes too far)

After this I had another problem, the part of the code which was supposed to go through the posts of a column, looped infinitely.

Before:

doom wiki column post format:
1 byte - rowStart
1 byte - pixel_count
1 byte - dummy_value
1 byte * pixel_count - pixel
1 byte - dummy_value
After:
my column post format:
1 byte - rowStart
1 byte - pixel_count
2 bytes - dummy_value
1 byte * pixel_count - pixel
1 byte - dummy_value
after debugging the read values, I figured out that there was a (dummy)byte missing from the post format. The pixel_count got the 255 value when it should have been read for the rowStart, 1 byte earlier.

column_array = new unsigned long int [width];

Shouldn't these be signed integers?

umm well according to Doom Wikia, column_array takes values beetween 0 - 4294967295, so I suppose its a unsigned long int.

BTW to read a single byte of the data you don't need to use memmove. You can do *(memblock+memPos), assuming memblock is char * or unsigned char *

hmm I'm not exactly sure how you meant to use that.
when I eg. replaced:

memmove (&pixel_count,memblock+memPos,1); //Read pixel_count, 1 byte	
with:
pixel_count = *(memblock+memPos);
it didnt work right. :/

First off, you need to modify your code not to assume that unsigned long is 4 bytes, because on GCC 64-bit this is false, and it will make your code unportable.

Ok, I added a sizeof(datatype), to every place where I had a hardcoded value for them. Though now that I think of it.. when I'm reading the values from a doom lump, shouln't the datatype byte sizes always be the same regardless of the operating system.. I mean its not like you convert the doom graphics to another format, when using them on another OS.

Link: http://pastebin.com/5QLFV4pA (diff)

Anyways, thanks for the help Jonathan and Quasar! :)

Share this post


Link to post
Worst said:

Ok, I added a sizeof(datatype), to every place where I had a hardcoded value for them.

You did the reverse of what the fix should have been, though. :)

The values expected by the format are those from the 32-bit architecture. The bytes will not move further apart in the file just because it's on a 64-bit OS. So instead of using sizeof() to make sure the offsets are in sync with the data types, you need to use non-ambiguous data types to make sure that they are in sync with the offsets.

To make it portable, you need to use explicit types, such as int8_t, int16_t, int32_t and their unsigned uintX_t versions. They are normally defined in a system header between a lot of #ifdefs. For example, for Windows MSVC you'd find that:

#ifdef _MSC_VER
typedef __int8 int8_t;
typedef unsigned __int8 uint8_t;
typedef __int16 int16_t;
typedef unsigned __int16 uint16_t;
typedef __int32 int32_t;
typedef unsigned __int32 uint32_t;
typedef __int64 int64_t;
typedef unsigned __int32 uint64_t;
#endif
Other platforms have different definitions, so you search for what's appropriate to each platform and define it in that single header. Makes portability a lot easier if you just get in the habit of using explicit types such as uint16_t rather than ambiguous types such as short. At least it is when manipulation of binary files is concerned.

Likewise, for reading multi-byte values, you're encouraged to define some sort of macro such as LE16, BE16, LE32, BE32, etc. that do the byte-swapping or not depending on the platform. E.g.:
#define SWAP16(val) (((val & 0x00FF)<<8)|((val & 0xFF00)>>8))
#define SWAP32(val) (((val & 0x000000FF)<<24)|((val & 0x0000FF00)>>8)
                     ((val & 0x00FF0000)>>8)|((val & 0xFF000000>>24))
#ifdef WORDS_BIGENDIAN
#define BE16(val) (val)
#define BE32(val) (val)
#define LE16(val) (SWAP16(val))
#define LE32(val) (SWAP32(val))
#else
#define BE16(val) (SWAP16(val))
#define BE32(val) (SWAP32(val))
#define LE16(val) (val)
#define LE32(val) (val)
#endif
Then when you read an int16 from a file that you know is written in a little-endian format, you use something like realvalue = LE16(valueread) and you'll be sure it'll be correct even on a different platform. (Actually I simplify a bit because there are weirder types of endianness around, see NUXI problem. Just dig in the definition headers of any cross-platform library to find workable examples.)

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  
×