don't click here

Sonic & Knuckles Collection C port

Discussion in 'Engineering & Reverse Engineering' started by BenoitRen, Jan 11, 2024.

  1. Devon

    Devon

    help me, i am in hell Tech Member
    1,407
    1,704
    93
    your mom
    A few things with unlze.

    So, this line uses "offset", right?
    Code (C):
    1.     *p_destination++ = p_destination[offset];

    Well, before this, in the "else" block, you erroneously define a new "offset" variable instead of using the already defined one above, which gets discarded once it leaves the "else" block.
    Code (C):
    1.     short offset = -256 + (byte >> 8);

    Speaking of this "else" block, the overall way "offset" is read and calculated is wrong, too.
    Code (C):
    1.     unsigned short byte = *((unsigned short*)p_source);
    2.     short offset = -256 + (byte >> 8);
    3.     offset *= 32;
    4.     offset += byte & 0xFF;
    5.     repeat_cnt = (byte >> 8) & 7;

    So, with reading, it looks like you forgot to advance "p_source" by 2 after reading. As for calculating the offset, the first 2 lines are correct, but the last one isn't. In the original 68000 code, the lower byte is completely overwritten. This is to discard the repeat count bits from the shifted offset value when adding in the other part.
    Code (ASM):
    1.     move.b  (a0)+,d0        ; Read offset and repeat count and advance
    2.     move.b  (a0)+,d1
    3.     moveq   #-1,d2          ; Calculate offset
    4.     move.b  d1,d2           ; Overwriting the lower byte of -1 is the same as -256 + lower byte
    5.     lsl.w   #5,d2
    6.     move.b  d0,d2
    7.     andi.w  #7,d1           ; Get repeat count

    So, the C code should be
    Code (C):
    1.     unsigned short byte = *((unsigned short*)p_source);
    2.     p_source += 2;
    3.     offset = -256 + (byte >> 8);
    4.     offset *= 32;
    5.     offset = (offset & 0xFF00) | byte & 0xFF;
    6.     repeat_cnt = (byte >> 8) & 7;

    While not a bug, I'm not quite understanding why this AND mask is being applied
    Code (C):
    1.     repeat_cnt = (repeat_cnt << 1) | descbit & 0xFFFFFFFF;

    Since "descbit" is only ever going to be 0 or 1, as it already gets masked prior
    Code (C):
    1.     descbit = desc & 1;

    That should be it.
     
    Last edited: Aug 29, 2024
    • Informative Informative x 1
    • List
  2. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Thanks! I do like to get some encouragement from time to time. :)
    D'oh! No doubt an oversight caused by refactoring.
    Fixed!
    This line implements a left rotation with carry. On a system where a long is wider than 32 bits, there'll be an unwanted bit in zero-based position 32, and I wanted to prevent that by masking the result with the maximum value I expect. But as you correctly interpreted, a bitwise AND has precedence over a bitwise OR, so it's operating on descbit. I really have to pay more attention, or at least not wait until compiler warnings tell me to put parentheses for clarity.

    Thanks for the corrections! I've fixed the scolling text in the meantime, and now it looks closer to what we expect:



    Notably, the animated parts (Tails's eyes, Sonic's arm, Knuckles's face, and Robotnik's upper torso and face) are still missing for some reason. The copyright text is also missing from the lower right corner.

    I thought there was something wrong with the pixel blitting code, but it uses the same routines as the level select, so that isn't looking likely.

    Still, we're getting closer!
     
    • Like Like x 2
    • Informative Informative x 1
    • List
  3. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Someone should rename the two LZEXE chunks for the character sprites in the disassembly. The names "Character Sprites" and "Characters" aren't distinct enough and it made me spend half an hour until I realised that the big differences in outputs and size I was seeing was because I mixed them up.

    Anyway, I embedded the decompressed data into my code, and now the sprites show up correctly. Except for the animated parts, of course. There must still be a bug in my implementation of unlze. Time to start at Sonic CD PC's implementation.

    upload_2024-8-29_21-58-59.png
     
    Last edited: Aug 29, 2024
  4. MainMemory

    MainMemory

    Kate the Wolf Tech Member
    4,789
    370
    63
    SonLVL
    Bit of a random thing, but I could make the mod loader's music replacement system into its own DLL for use with this port, so you'd get MIDI, SMPS, and streamed audio support.
     
  5. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    That'd be nice. :)

    Do keep in mind that my plan is to eventually leave the Windows wrapper behind to make this port cross-platform. I'm personally looking forward to having this on my PlayStation Portable if performance isn't an issue.

    Back to unlze.

    I tried Sonic CD's way of calculating the offset:
    Code (C):
    1. sD2 = -1;
    2. sD2 = sD2 & 65280 | *pSrc++;
    Code (C):
    1. offset = (-1 & 0xFF00) | *p_source++;
    But it inexplicably makes the result look even less than what's intended.

    Being out of ideas, I decided to finally heed the following compiler warning: "operation on 'p_destination' may be undefined". It's about this piece of code:
    Code (C):
    1. *p_destination++ = p_destination[offset];
    I changed it to this:
    Code (C):
    1. unsigned char value = p_destination[offset];
    2. *p_destination++ = value;
    I still don't understand the problem, but the result doesn't lie: the decompression is now accurate!

    All right, now let's figure out what's wrong in the mode's actual logic and fix it. Ah, I forgot to put a function call there. Let's see what happens now. ...Whoa!



    EDIT: It turns out the vscroll variable is signed. This makes me nervous, because the code uses a bitwise AND on the value to do a larger increment every two frames. I'm probably doing something wrong with the incrementing because the vertical scrolling is supposed to stop at zero and it misses that by one.

    EDIT2: The small increment is done exclusively on the lower two bytes in the original ASM. Once I do the same, the vertical scrolling stops where it should. This incrementing business is going to be a pain to do in a portable way.
     
    Last edited: Aug 30, 2024
  6. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    I failed to handle a signed vscroll variable in a portable way without breaking anything. As they're essentially two values packed together, I split them up into vscrolla and vscrollb. The vertical scrolling now stops.

    There were a couple bugs in my sprite blitting functions. Now they show up!



    Some observations:
    • At first, Tails's face doesn't appear, but the other sprites do. After that, Tails's face shows up, but the first animation frame of the other sprites show Tails's face. I'm guessing it's an off-by-one error.
    • Something is off with the rotating star sphere as well. Only one animation frame is correct; the others show two mirrored halves.
    • Only the first code digit is correct. The others stay zero.
    • There's a Stage header instead of a Level header, and it doesn't switch between them.
     
  7. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    I've fixed the issue with Tails's face not appearing initially, and the rotating star sphere now shows correctly.

    For some reason only the right star sphere shows up. I thought maybe both were blitted at the same position, but the logic, which depends on bit 0 of actflg (horizontal flip) to select the correct position array, is correct. As a test I tried not setting bit 0 of the right star sphere's actflg value. The result is that the left star sphere shows up, but the right star sphere doesn't. It's so strange!

    After fixing the condition in init_specmodevars the stage code is set in memory, and it shows up on the screen. But I really have to wonder what the logic is behind the following code that converts 50462976 (0x3020100) to 16643 (0x4103):
    Code (C):
    1. unsigned long code = BYTESWAP(g_mdRam.specmodecode);
    2. code = (code & 0xFFFFFF00) | (code & 0x000000FF) << 1;
    3. code = (code & 0xFFFF0000) | (code & 0x0000FFFF) << 1;
    4. ROTATE_RIGHT(code, 16);
    5. code = (code & 0xFFFFFF00) | (code & 0x000000FF) << 1;
    6. code = (code & 0xFFFF0000) | (code & 0x0000FFFF) >> 1;
    7. ROTATE_RIGHT(code, 16);
    8. code >>= 2;
    9. code &= 0xFFFFFFF;
     
  8. MainMemory

    MainMemory

    Kate the Wolf Tech Member
    4,789
    370
    63
    SonLVL
    So Blue Sphere maps consist of four quadrants, with each quadrant choosing from one of 128 possible chunks. This code is condensing the four section IDs into 7-bit values and pushing them all together.
    If the original data can be represented in binary as 0AAAAAAA 0BBBBBBB 0CCCCCCC 0DDDDDDD, the result is 0000AAAAAAABBBBBBBCCCCCCCDDDDDDD.
     
    • Like Like x 3
    • Useful Useful x 1
    • List
  9. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Today I only managed to fix the header. It now shows the level number initially, and switches to the stage code when you move the spheres up. However, there's empty space next to the level number that I can't seem find the cause of:

    upload_2024-8-31_22-3-5.png

    No luck with getting the left star sphere to show up, either. Haven't spent much time looking at why the stage code input at the bottom is incorrect.

    I'm starting to get discouraged not being able to fix these, so for now I've switched to porting the title screens.
     
  10. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    As there is no new decompression routine to worry about this time, I was hoping Sonic 3 & Knuckles's title screen would turn out close to the original. Alas, it was not to be:

     
  11. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    I've tried to diagnose this problem, but I honestly don't even know where to look. It's as if pixel blitting starts in the wrong place, which causes all of the tiles to misalign. All I could confirm is that it uses the same pixel blitting routines as the two other screens.
     
  12. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Went back to fixing Special Stage Mode, and this time I noticed that I forgot to port the loop at the bottom of set_level_digits. Adding it fixed the level display.

    To debug the left star sphere not being displayed, I decided to sneak the sprite status object ID into the sprite table. I used the unused 7 bits next to the x position for that. When the sprite table is converted into a struct for the pixel blitting routines, I put it in one of the struct's two unused fields.

    I noticed that this object never made it to the blitting routines, so I started to follow the trail at the beginning.

    • actionsub puts it into the priority table.
    • patset processes it.
    • uploadSprites converts it into a struct.

    I made a mental note that the object was the very first item in the list, and followed the code to blitSprites. There, it uses the loop at the bottom, where the sprites are processed from the last to the first. Aha, must be an off-by-one error! I looked at the original code in Ghidra, and sure enough, the line where the address of the last sprite is taken was wrong. To get an index to the last element, you have to subtract one from the count.

    The only thing left to fix is the stage code at the bottom:

    upload_2024-9-3_14-45-56.png
     
  13. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Ported the first part of Sonic & Knuckles's title screen, and it's also misaligned. Could there still be a problem with decompression using mapdevr? It's hard to verify, because decompression tools don't allow you to pass in an offset.

    upload_2024-9-4_15-52-47.png
     
  14. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Comparing the result of decompression by KENS with the result of that by my implementation, I do see more differences than just endianness. The hunt for decompression bugs is back on. :(

    EDIT: Looked back at Devon's corrections for mapdevr, and it looks like I missed some. After also making those corrections, the logo and Sonic jumping towards the screen look correct! The title screen itself, however, is not entirely there yet:

    upload_2024-9-4_22-34-3.png
     
    Last edited: Sep 4, 2024
  15. Bobblen

    Bobblen

    Member
    429
    216
    43
    I realise that the decompression issues are probably driving you nuts right now, but it's got to feel pretty good to finally be seeing some Sonic on the screen instead of just endless assembly code :-D It's cool to see it progressing.
     
  16. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    It turns out that the title screen itself not showing correctly had nothing to do with decompression. The compressed source data for the title screen was incomplete. In SK&C, frame 14 seems to be twice the size of the Mega Drive original.

    Next I had to fix the code responsible for the banner jumping into place, which required me to reevaluate how I handle fixed point integers.

    I've now added the rest of the code, added some missing actionsub calls (no wonder the finger and eye didn't seem to be animating), and now it's complete!

     
  17. Blastfrog

    Blastfrog

    See ya starside. Member
    I just want to say that I love the work you're doing on this! It's really starting to come together.

    This is admittedly a little bit off-topic, but I think it'd be really neat if there were added support for a Gravis Ultrasound module, and a custom hybrid FM/GM OST to go with it. That'd be a lot of work and I'm just spitballing/fantasizing here, but the point is that a lot of novel stuff could be done with this once completed.
     
  18. MainMemory

    MainMemory

    Kate the Wolf Tech Member
    4,789
    370
    63
    SonLVL
    I don't think any of that needs a decomp, we've had ways to totally replace the music player for a long time now.
     
  19. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    Sonic & Knuckles's title screen is more complex.
    • It uses special routines to shake the screen and create a static effect on the Sega logo.
    • It copies parts of tile maps during the sequence.
    • It uses a moduled variant of LZEXE/Kosinko to compress the tons of tiles the title screen uses.
    I've ported most of the code. All that's missing are the chibi characters with their name next to them, because that requires me to add code to handle compound sprites.



    The static effect works, but the logo doesn't dissipate. The background around the mountain is missing until Sonic lands. Sonic & Knuckles are missing most of their graphics.

    The missing background might be because of a scroll-related bug. The missing graphics are probably due to my implementation of moduled LZEXE. For the Sega logo, it could be the partial map copying, but I've already double-checked the code.
     
  20. BenoitRen

    BenoitRen

    Tech Member
    775
    382
    63
    There was something wrong with how I calculated the padding after each LZEXE module. Now that I understand how the original code handles it, I've changed the code accordingly.

    Most calls to unlze_moduled had the arguments swapped.

    Unfortunately, the result after these corrections doesn't look that much better:

    upload_2024-9-8_13-41-25.png

    EDIT: Fixed a bug in the first check in copy_mapline_vram that caused the function to never be executed. Now there is a background while Sonic is falling, but the rest still looks wrong.
     
    Last edited: Sep 8, 2024