don't click here

Some changes and fixes for Sonic 2

Discussion in 'Engineering & Reverse Engineering' started by Esrael, Jun 7, 2012.

  1. DeltaWooloo

    DeltaWooloo

    Be a boss-man and come to my big and tall man shop Member
    Earlier last month, I've had a conversation with vladikcomper to find the best methods to optimize Sonic 1. He came across how it is possible to optimize the BossMove subroutine, which moves the bosses. And after doing my quick analysis by playing GHZ3, I've noticed that the platforms tend to glitch for about a frame or two every time the boss moves.

    In Sonic 2, the code is only used twice, and that is for the Chemical Plant Zone boss and the rest is for most of the Sonic 2 bosses. So this mini-tutorial will show you how to optimize that code in vein to S3K. It won't look fast, but it takes less processing time which matters if you intend to have custom bosses with different objects. This will work under any Sonic 2 disassembly. (Sonic 2 Git will be referenced with comments for Xenowhirl's 2007 disassembly).

    For Sonic 1 users, I've made a guide for you, so please check it out here.

    This will work under any Sonic 2 disassembly. (Sonic 2 Git will be referenced with comments for Xenowhirl's 2007 disassembly).

    So let's take a look at Obj_CPZBoss_Main_Move: (loc_2DB0E for the 2007 disassembly)

    Code (Text):
    1. Obj_CPZBoss_Main_Move:
    2.     move.l    Obj_CPZBoss_x_pos_next(a0),d2
    3.     move.l    Obj_CPZBoss_y_pos_next(a0),d3
    4.     move.w    x_vel(a0),d0
    5.     ext.l    d0
    6.     asl.l    #8,d0
    7.     add.l    d0,d2
    8.     move.w    y_vel(a0),d0
    9.     ext.l    d0
    10.     asl.l    #8,d0
    11.     add.l    d0,d3
    12.     move.l    d2,Obj_CPZBoss_x_pos_next(a0)
    13.     move.l    d3,Obj_CPZBoss_y_pos_next(a0)
    14.     rts
    I optimised it by comparing how redhotsonic shortened the subroutine and applied it here. The result you should get is this:

    Code (Text):
    1. Obj_CPZBoss_Main_Move:
    2.     move.w    x_vel(a0),d0
    3.     ext.l    d0
    4.     lsl.l    #8,d0
    5.     add.l    d0,Obj_CPZBoss_x_pos_next(a0) ; for 2007 users, Obj_CPZBoss_x_pos_next should be objoff_30
    6.     move.w    y_vel(a0),d0
    7.     ext.l    d0
    8.     lsl.l    #8,d0
    9.     add.l    d0,Obj_CPZBoss_y_pos_next(a0) ; for 2007 users, Obj_CPZBoss_y_pos_next should be objoff_38
    10.     rts
    Now, let's check out Boss_MoveObject (loc_2D5DE for 2007 users). This is the code:

    Code (Text):
    1. Boss_MoveObject:
    2.     move.l    (Boss_X_pos).w,d2
    3.     move.l    (Boss_Y_pos).w,d3
    4.     move.w    (Boss_X_vel).w,d0
    5.     ext.l    d0
    6.     asl.l    #8,d0
    7.     add.l    d0,d2
    8.     move.w    (Boss_Y_vel).w,d0
    9.     ext.l    d0
    10.     asl.l    #8,d0
    11.     add.l    d0,d3
    12.     move.l    d2,(Boss_X_pos).w
    13.     move.l    d3,(Boss_Y_pos).w
    14.     rts
    We will need to do the same optimisations as RHS did. The outcome should look something like this:

    Code (Text):
    1. Boss_MoveObject:
    2.     move.w   (Boss_X_vel).w,d0 ; Boss_X_vel should be $FFFFF758 for 2007 users
    3.     ext.l    d0
    4.     lsl.l    #8,d0      
    5.     add.l    d0,(Boss_X_pos).w ; Boss_X_pos should be $FFFFF750 for 2007 users
    6.     move.w    (Boss_Y_vel).w,d0  ; Boss_Y_vel should be $FFFFF75A for 2007 users
    7.     ext.l    d0
    8.     lsl.l    #8,d0        
    9.     add.l     d0,(Boss_Y_pos).w ; Boss_Y_pos should be $FFFFF754 for 2007 users
    10.     rts
    And that's it. Every time the boss moves, it will do the coding quicker in each frame.

    EDIT - 17/1/21: Updated the Sonic 2 side of the guide to mention a subroutine that multiple bosses share.
     
    Last edited: Jan 17, 2022
  2. flamewing

    flamewing

    Emerald Hunter Tech Member
    1,161
    65
    28
    France
    Sonic Classic Heroes; Sonic 2 Special Stage Editor; Sonic 3&K Heroes (on hold)
    Note that because S2 used 2 registers in the original functions, you can do slightly better than S3&K with the optimization:

    Code (ASM):
    1. Obj_CPZBoss_Main_Move:
    2.     movem.w x_vel(a0),d0/d2                 ; Does sign extension for free
    3.     asl.l   #8,d0                           ; shift velocity to line up with the middle 16 bits of the 32-bit position
    4.     add.l   d0,Obj_CPZBoss_x_pos_next(a0)   ; add X speed to X position ; note this affects the subpixel position
    5.     asl.l   #8,d2                           ; shift velocity to line up with the middle 16 bits of the 32-bit position
    6.     add.l   d2,Obj_CPZBoss_y_pos_next(a0)   ; add Y speed to Y position ; note this affects the subpixel position
    7.     rts
    Code (ASM):
    1.  
    2. Boss_MoveObject:
    3.     movem.w (Boss_X_vel).w,d0/d2            ; Does sign extension for free
    4.     asl.l   #8,d0                           ; shift velocity to line up with the middle 16 bits of the 32-bit position
    5.     add.l   d0,(Boss_X_pos).w               ; add X speed to X position ; note this affects the subpixel position
    6.     asl.l   #8,d2                           ; shift velocity to line up with the middle 16 bits of the 32-bit position
    7.     add.l   d2,(Boss_Y_pos).w               ; add Y speed to Y position ; note this affects the subpixel position
    8.     rts
    These save 8 cycles compared to the S3&K-based versions you have. At this level, you can also gain at least 34+ cycles by inlining the calls into the place (eliminating at least a bsr.w+rts), preferably with macros, if you are *really* starved for cycles.

    But there are other methods to make S2 (or S1) much faster; for starters, porting S3&K's collision response list, ring manager, and object manager are moderately easy to to and will yield far greater savings in terms of cycles than these micro-optimizations will.
     
    • Like Like x 1
    • Agree Agree x 1
    • Informative Informative x 1
    • Useful Useful x 1
    • List
  3. Esrael

    Esrael

    Neto Tech Member
    304
    257
    63
    Brazil, São Paulo, Guarulhos
    Neto Assembler Editor / Sonic 2 Delta / Neto MD-DOS
    Recently I have decided recode Sonic Delta 40Mb from a fresh SK disassembly that I have made, and I am rewriting all Sonic 1 and Sonic 2 objects to make full usage of Sk main routines avoiding porting S2 ones.
    When porting Hammer Eggman (Aquatic Ruin Boss) I get crashes or strange behaviour when Robotnik hit the pillar with his hammer.
    After a lot of revisions in recoded boss I have found that this is an issue in the original code. And here I will explain the issue and how to fix.

    Look in these two routines (using S2 disassembly as example)
    Code (Text):
    1.  
    2. Obj89_Pillar:
    3.    moveq   #0,d0
    4.    movea.l   obj89_pillar_parent(a0),a1 ; a1=object
    5.    cmpi.b   #8,boss_routine(a1)       ; has boss been defeated?
    6.    blt.s   Obj89_Pillar_Normal       ; if not, branch
    7.    move.b   #4,routine_secondary(a0)
    8.  
    9. ; loc_309BC:
    10. Obj89_Pillar_Normal:
    11.  
    Code (Text):
    1.  
    2. Obj89_Arrow:
    3.    moveq   #0,d0
    4.    movea.l   obj89_arrow_parent(a0),a1 ; a1=object
    5.    cmpi.b   #8,boss_routine(a1)       ; has boss been defeated?
    6.    blt.s   Obj89_Arrow_Normal       ; if not, branch
    7.    move.b   #6,obj89_arrow_routine(a0)   ; => Obj89_Arrow_Sub6
    8.  
    9. ; loc_30BB2:
    10. Obj89_Arrow_Normal:
    11.  
    Explanation:
    - The variables obj89_arrow_parent(a0) and obj89_pillar_parent(a0) sometimes have an invalid RAM address resulting in the read of Start of ROM vector. If you don't change / add an exception handler the read value will have a lower value ($02) in a fresh Sonic 2. With the original value the next line will read a value lower than 8 and the boss will work as expected, but if you add an exception handler and the address of it result in any value greater or equal 8 in the position read by the boss code the issues will start.

    How to fix:
    - In Obj89_Pillar:
    After moveq #$00, D0, add
    Code (Text):
    1.  
    2.     tst.l   obj89_pillar_parent(a0)
    3.     beq.s  Obj89_Pillar_Normal
    4.  
    and in Obj89_Arrow:
    After moveq #$00, D0, add
    Code (Text):
    1.  
    2.     tst.l   obj89_arrow_parent(a0)
    3.     beq.s  Obj89_Arrow_Normal
    4.  
    The new code should look like:
    Code (Text):
    1.  
    2. Obj89_Pillar:
    3.    moveq   #0,d0
    4.     tst.l   obj89_pillar_parent(a0)
    5.     beq.s  Obj89_Pillar_Normal
    6.    movea.l   obj89_pillar_parent(a0),a1 ; a1=object
    7.    cmpi.b   #8,boss_routine(a1)       ; has boss been defeated?
    8.    blt.s   Obj89_Pillar_Normal       ; if not, branch
    9.    move.b   #4,routine_secondary(a0)
    10.  
    11. ; loc_309BC:
    12. Obj89_Pillar_Normal:
    13.  
    Code (Text):
    1.  
    2. Obj89_Arrow:
    3.    moveq   #0,d0
    4.     tst.l   obj89_arrow_parent(a0)
    5.     beq.s  Obj89_Arrow_Normal
    6.    movea.l   obj89_arrow_parent(a0),a1 ; a1=object
    7.    cmpi.b   #8,boss_routine(a1)       ; has boss been defeated?
    8.    blt.s   Obj89_Arrow_Normal       ; if not, branch
    9.    move.b   #6,obj89_arrow_routine(a0)   ; => Obj89_Arrow_Sub6
    10.  
    11. ; loc_30BB2:
    12. Obj89_Arrow_Normal:
    13.  
    Build and Now you can use your custom exception handler without causing issues to this boss.

    Best regards:
    --------------------
    Esrael Neto
     
    Last edited: Feb 28, 2022
    • Informative Informative x 1
    • List
  4. Uh, so this happened when jumping on a vine.
    upload_2022-4-1_11-15-17.png
     
  5. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    This fix should correct that. The problem isn't with the vine, but the drawbridge next to it: its hitbox gets misplaced, potentially nudging Sonic from the vine switch slightly if you land on the switch while moving at a high-enough speed.
     
  6. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Somewhere in this thread, I probably mentioned a "bugfix" that was only in REV00: in BuildSprites, there's an extra check that prevents objects from being displayed if their 'mappings' pointer is 0 (null). This check just so happens to fix a crash in Sonic 2 that occurs if you die, enter Debug Mode, and then try to place an object while dead.

    The reason for this crash is that, when you're dead, most objects don't run their code. This means that objects which were spawned on that frame will not initialise themselves. The mappings pointer is one such thing that will not be initialised, leaving it at the default value of 0. Usually, objects have to run their code in order to be displayed, because objects aren't displayed unless they call DisplaySprite. However, when you're dead, the game manually displays objects instead, bypassing this. This is how an object with a null mappings pointer ends up being processed by BuildSprites. The extra check in REV00 detects this, and avoids displaying it.

    For unrelated reasons, this situation still causes a crash in REV00. The reason for the crash in REV00 is that it manually reads a word from address 0x00000001, which seems to be a deliberate attempt to crash the console. Perhaps it was used for debugging, to detect the 'clusterfuck bug' that plagued Sonic 1, which too would cause objects with a null mappings pointer to be displayed.

    Curiously, Sonic & Knuckles (and Sonic 3?) don't crash when this happens. I looked into it, but it appears that it doesn't actually fix the bug, it just hides it: when BuildSprites (or 'Render_Sprites' as it's called in the Sonic & Knuckles disassembly) processes an object with a null mappings pointer, it causes it to read from address 0. The value at this location just so happens to be the stack pointer part of the vector table. What Sonic 3 and Sonic & Knuckles does is set this stack pointer to 0, which, long story short, causes BuildSprites to not render anything. Indeed, if you modify Sonic & Knuckles to have a proper stack pointer in its vector table, then the bug will manifest just like in Sonic 2.

    REV00's solution, however, is not a proper fix. The mappings pointer check in BuildSprites is just a sanity check: a workaround for a situation that should never occur in the first place. This check *should* be pointless, and objects *shouldn't* be queued for display if they don't have a valid mappings pointer. This check just slows BuildSprites down. In fact, it's not the only one of its kind: Sonic 1 and Sonic 2 both feature a similar sanity check which checks if an object's ID is 0.

    What's notable, however, is that Sonic 3 and Sonic & Knuckles do not have either of these sanity checks: they show that they're not needed. Indeed, there is a much more proper fix for the crash which makes the mappings pointer sanity check necessary in the first place:

    'RunObjects' is the function responsible for making objects run their code every frame. When the player is dead, it branches to 'RunObjectsWhenPlayerIsDead', which causes some objects to not run their code. For these objects, this code is run instead:

    Code (ASM):
    1. ; sub_15FF2:
    2. RunObjectDisplayOnly:
    3.         moveq   #0,d0
    4.         move.b  id(a0),d0       ; get the object's ID
    5.         beq.s   +       ; if it's obj00, skip it
    6.         tst.b   render_flags(a0)        ; should we render it?
    7.         bpl.s   +                       ; if not, skip it
    8.         bsr.w   DisplaySprite
    9. +
    10.         lea     next_object(a0),a0 ; load 0bj address
    11.         dbf     d7,RunObjectDisplayOnly
    12.         rts
    13. ; End of function RunObjectDisplayOnly
    You might think that the solution is to add some code that skips the object if its mappings pointer is 0, but the real solution is actually staring us in the face: the 'render_flags' check. You might be wondering what it's for, but it actually seems to be a way of preventing an object from being display if it wasn't displayed on the previous frame: the high bit of 'render_flags' is set by BuildSprites when it successfully renders the objects, and cleared when it doesn't. The point of this check seems to be to prevent objects from being displayed if they had just been spawned on this frame, and haven't initialised themselves yet.

    ...Wait, what? Isn't that exactly what causes this crash in the first place? Well, yes. But it certainly seems to work for objects spawned regularly: it just doesn't work for objects spawned with Debug Mode.

    Here's the code that spawns an object when you get close enough to it:

    Code (ASM):
    1. ; ---------------------------------------------------------------------------
    2. ; Subroutine to check if an object needs to be loaded.
    3. ;
    4. ; input variables:
    5. ;  d2 = respawn index of object to be loaded
    6. ;
    7. ;  a0 = address in object placement list
    8. ;  a2 = object respawn table
    9. ;
    10. ; writes:
    11. ;  d0, d1
    12. ;  a1 = object
    13. ; ---------------------------------------------------------------------------
    14. ;loc_17F36:
    15. ChkLoadObj:
    16.         tst.b   2(a0)   ; does the object get a respawn table entry?
    17.         bpl.s   +       ; if not, branch
    18.         bset    #7,2(a2,d2.w)   ; mark object as loaded
    19.         beq.s   +               ; branch if it wasn't already loaded
    20.         addq.w  #6,a0   ; next object
    21.         moveq   #0,d0   ; let the objects manager know that it can keep going
    22.         rts
    23. ; ---------------------------------------------------------------------------
    24.  
    25. +
    26.         bsr.w   SingleObjLoad   ; find empty slot
    27.         bne.s   return_17F7E    ; branch, if there is no room left in the SST
    28.         move.w  (a0)+,x_pos(a1)
    29.         move.w  (a0)+,d0        ; there are three things stored in this word
    30.         bpl.s   +               ; branch, if the object doesn't get a respawn table entry
    31.         move.b  d2,respawn_index(a1)
    32. +
    33.         move.w  d0,d1           ; copy for later
    34.         andi.w  #$FFF,d0        ; get y-position
    35.         move.w  d0,y_pos(a1)
    36.         rol.w   #3,d1   ; adjust bits
    37.         andi.b  #3,d1   ; get render flags
    38.         move.b  d1,render_flags(a1)
    39.         move.b  d1,status(a1)
    40.         _move.b (a0)+,id(a1) ; load obj
    41.         move.b  (a0)+,subtype(a1)
    42.         moveq   #0,d0
    43.  
    44. return_17F7E:
    45.         rts
    As you can see, 'd1' is ANDed with 3 before it is written to 'render_flags', meaning that the high bit of 'render_flags' will be 0. This prevents the object from being queued for display by RunObjectDisplayOnly.

    Now here's the code for spawning an object in DebugMode:

    Code (ASM):
    1. ; loc_41C12:
    2. Debug_SpawnObject:
    3.         btst    #button_C,(Ctrl_1_Press).w
    4.         beq.s   Debug_ExitDebugMode
    5.         ; Spawn object
    6.         jsr     (SingleObjLoad).l
    7.         bne.s   Debug_ExitDebugMode
    8.         move.w  x_pos(a0),x_pos(a1)
    9.         move.w  y_pos(a0),y_pos(a1)
    10.         _move.b mappings(a0),id(a1) ; load obj
    11.         move.b  render_flags(a0),render_flags(a1)
    12.         move.b  render_flags(a0),status(a1)
    13.         andi.b  #$7F,status(a1)
    14.         moveq   #0,d0
    15.         move.b  (Debug_object).w,d0
    16.         lsl.w   #3,d0
    17.         move.b  4(a2,d0.w),subtype(a1)
    18.         rts
    Here, the 'render_flags' of the object is just set to the 'render_flags' of Sonic. No attempt is made to set the high bit to 0. Since Sonic is successfully displayed by BuildSprites every frame (even in Debug Mode), this bit will always be 1. This is what causes the object to be queued for display by RunObjectDisplayOnly, which causes it to be processed by BuildSprites, which causes its null, uninitialised mappings pointer to be used, causing a crash.

    The solution? AND 'render_flags' with $7F, just like is done with 'status'.

    EDIT: I guess this needs updating.
     
    Last edited: Apr 12, 2022
    • Like Like x 2
    • Informative Informative x 2
    • List
  7. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    This bug is pretty minor, but it can become a problem if you ever try to improve the game's object allocation system: in my case, I was encountering a crash after making the object RAM buffer into a linked-list.

    The Chemical Plant Zone boss does this weird thing where it allocates an object without using it. Not only that, but it writes to a portion of the object's data before it decides not to use it. This potentially leaves an object in memory that has its ID set to 0, but not all of its bytes set to 0. This is bad because the game engine usually tries to make sure that all unused object RAM slots are completely set to 0, and this bug violates that.

    The culprit is in this code:
    Code (ASM):
    1. Obj5D_Pipe_2_Load:
    2.         jsr     (SingleObjLoad2).l
    3.         beq.s   +
    4.         rts
    5. ; ---------------------------------------------------------------------------
    6. +
    7.         move.l  a0,Obj5D_parent(a1)
    8.  
    9. Obj5D_Pipe_2_Load_Part2:
    10.         subq.w  #1,Obj5D_pipe_segments(a0)      ; is pipe fully extended?
    11.         blt.s   Obj5D_Pipe_2_Load_End           ; if yes, branch
    12.  
    13.         _move.b #ObjID_CPZBoss,id(a1)   ; load obj5D
    14.         move.l  #Obj5D_MapUnc_2EADC,mappings(a1)
    15.         move.w  #make_art_tile(ArtTile_ArtNem_CPZBoss,1,0),art_tile(a1)
    16.         move.b  #4,render_flags(a1)
    17.         move.b  #$20,width_pixels(a1)
    18.         move.b  #5,priority(a1)
    19.         move.w  x_pos(a0),x_pos(a1)
    20.         move.w  y_pos(a0),y_pos(a1)
    As you can see, 'SingleObjLoad2' is used to allocate an object, its 'Obj5D_parent' value is modified, but then, depending on 'Obj5D_pipe_segments', the object may be discarded.

    The solution to this is to move the two lines below 'Obj5D_Pipe_2_Load_Part2' to above the call to 'SingleObjLoad2', so that the allocation is skipped if the object is just going to be discarded anyway.

    That still leaves the other bit of code that calls 'Obj5D_Pipe_2_Load_Part2': 'Obj5D_Pipe_0'. By moving the 'subq.w' instruction, the counter will now not be decremented when 'Obj5D_Pipe_0' is ran, which we need to fix. This is simple enough: just make it set 'Obj5D_pipe_segments' to $B instead of $C.
     
    Last edited: Apr 15, 2022
  8. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Here's another one: object $BD (the ascending/descending conveyor belt platform from Wing Fortress Zone) calls 'DeleteObject' and 'MarkObjGone' on the same frame. This can cause two things:
    • It can cause the object to call 'DeleteObject' and 'DisplaySprite' on the same frame, which would cause an empty object slot to be queued for display. This is the same kind of bug that plagued Sonic 1.
    • It can cause the object to call 'DeleteObject' on the same frame. Normally, this is just plain redundant, but it becomes a much bigger problem if you've modified the object RAM buffer into a linked list like I have, since it results in a double-free.
    Here's the code responsible for this:
    Code (ASM):
    1. loc_3BC50:
    2.         moveq   #0,d0
    3.         move.b  routine_secondary(a0),d0
    4.         move.w  off_3BC62(pc,d0.w),d1
    5.         jsr     off_3BC62(pc,d1.w)
    6.         jmpto   (MarkObjGone).l, JmpTo39_MarkObjGone
    7. ; ===========================================================================
    8. off_3BC62:      offsetTable
    9.                 offsetTableEntry.w loc_3BC6C    ; 0
    10.                 offsetTableEntry.w loc_3BCAC    ; 2
    11.                 offsetTableEntry.w loc_3BCB6    ; 4
    12.                 offsetTableEntry.w loc_3BCCC    ; 6
    13.                 offsetTableEntry.w loc_3BCD6    ; 8
    Code (ASM):
    1. loc_3BCD6:
    2.         bsr.w   loc_3B7BC
    3.         bra.w   JmpTo65_DeleteObject
    'loc_3BC50' calls 'loc_3BCD6' (which calls 'DeleteObject') and then calls 'MarkObjGone'. To correct this, you can meddle with the stack to prevent 'loc_3BC50' being returned to after 'loc_3BCD6' is called. This is a bit of a hack, but this same kind of hack appears multiple times in the game's code already, so it's nothing new. Just add 'addq.w #4,sp' to the start of 'loc_3BCD6'.
     
    Last edited: Apr 15, 2022
  9. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    How about another one? It turns out that the Chemical Plant Zone boss has *more* bugs in it:

    'Obj5D_Pipe_Retract' is an extremely jank bit of code for finding and updating pipe segment objects in RAM.
    • Problem 1: it searches the entirely of object RAM (reserved and dynamic) when it only needs to search the dynamic objects. This is just a waste of CPU cycles.
    • Problem 2: it uses 'd7', which is a register that objects should never modify, because it is permanently in use by 'RunObjects'. This could theoretically cause the object updater to crash, or just fail to update some objects.
    • Problem 3: the second ending of the loop does not advance to the next object in RAM, causing the loop to get stuck repeatedly checking the same object until its counter reaches 0. If the boss's hovering motion is disabled, then it's actually possible to get the boss's pipe stuck because of this bug by positioning Sonic or Tails at the same Y-coordinate as a pipe segment. Even if the boss's hovering motion isn't disabled, this bug can still cause the pipe's updating to be delayed by a frame.
    To fix the first issue, make the code iterate from 'Dynamic_Object_RAM' to 'Dynamic_Object_RAM_End' instead of from 'Object_RAM' to 'Object_RAM_End'.

    To fix the second issue, the code can be rewritten to not use 'd7'. In the process, the code can also be made a lot less dumb:

    From:
    Code (ASM):
    1.         moveq   #0,d7
    2.         move.b  #ObjID_CPZBoss,d7
    3.         cmp.b   id(a1),d7       ; is object a subtype of the CPZ Boss?
    To:
    Code (ASM):
    1.         cmpi.b  #ObjID_CPZBoss,id(a1)       ; is object a subtype of the CPZ Boss?
    To fix the third issue, insert a 'lea next_object(a1),a1' instruction above the 'dbf' instruction of 'Obj5D_Pipe_Retract_ChkID'.
     
  10. Jesus Christ, how badly implemented was the CPZ boss?
     
  11. Esrael

    Esrael

    Neto Tech Member
    304
    257
    63
    Brazil, São Paulo, Guarulhos
    Neto Assembler Editor / Sonic 2 Delta / Neto MD-DOS
    Another issue that I get when recoding all objects in Sonic Delta.
    This time I am getting crashes with Pressure Springs in Oil Ocean (Obj45).

    Look in the routine Obj45_Init (using S2 disassembly as example)
    Code (Text):
    1.  
    2.    move.b   subtype(a0),d0
    3.    lsr.w   #3,d0
    4.    andi.w   #$E,d0   ; <<<- Issue here
    5.    move.w   Obj45_InitRoutines(pc,d0.w),d0
    6.    jmp   Obj45_InitRoutines(pc,d0.w)
    7. ; ===========================================================================
    8. ; off_24146:
    9. Obj45_InitRoutines: offsetTable
    10.    offsetTableEntry.w loc_2416E   ; 0
    11.    offsetTableEntry.w loc_2414A   ; 2
    12. ; ===========================================================================
    13. loc_2414A:
    14.    move.b   #4,routine(a0)
    15.  
    Explanation:
    - The offsetTable have only two entries, but the object code allow the input of 8 entries when the subtype is passed to D0 divided by 8 (lsr.w #3, D0), and "and" by $0E (andi.w #$E, D0). If you use any subtype that result in value greater than $02, this can result in an unexpected behaviour or crash when the code jmp Obj45_InitRoutines(pc,d0.w) is run. Looking in the object layout of Oil Ocean 2 at coordinates X $1E34 / Y $2230 and X $1E34 / Y $2350 we found two of this object with subtype $30 which results in $06 after the subtype is passed to D0, divided by 8 and "and" by $E. In a fresh Sonic 2 the code at loc_2414A when built result in "$117C", "$0004", "$0024". When the code move.w Obj45_InitRoutines(pc,d0.w),d0 is executed the value $0004 is read and this point to loc_2414A which is valid and match with orignal behaviour of object, but if you decide to make some changes in object the issues will start:

    How to fix:
    -The fix is very easy, just change the $0E to $02 in the "and" line
    Code (Text):
    1.  
    2.    move.b   subtype(a0),d0
    3.    lsr.w   #3,d0
    4.    andi.w   #$2,d0   ; <<<- Changed from $E To $2
    5.    move.w   Obj45_InitRoutines(pc,d0.w),d0
    6.    jmp   Obj45_InitRoutines(pc,d0.w)
    7. ; ===========================================================================
    8. ; off_24146:
    9. Obj45_InitRoutines: offsetTable
    10.    offsetTableEntry.w loc_2416E   ; 0
    11.    offsetTableEntry.w loc_2414A   ; 2
    12. ; ===========================================================================
    13. loc_2414A:
    14.    move.b   #4,routine(a0)
    15.  
     
    Last edited: Apr 18, 2022
  12. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    I can't recreate this issue: if I make 'Obj89_Arrow' and 'Obj89_Pillar' check if those RAM addresses are 0, and run an 'illegal' instruction if they are, then the game never crashes. Also, looking at how and where the code sets 'obj89_arrow_parent' and 'obj89_pillar_parent', it seems that it should be impossible for those to ever not be set to a valid value, as they're always set when those objects are spawned.
     
  13. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    How is my fix broken? Devon's fix does the same thing, except it also removes the redundant 'bmi' and changes the 'bge' to 'bhs', neither of which should make a difference because the sign bit will never be set anyway. I also can't recreate the bug you're describing where the rings break after Y-wrapping the level.
     
    Last edited: Apr 20, 2022
  14. Esrael

    Esrael

    Neto Tech Member
    304
    257
    63
    Brazil, São Paulo, Guarulhos
    Neto Assembler Editor / Sonic 2 Delta / Neto MD-DOS
    Is Very easy to trigger the issue, get a clear Sonic 2 ROM and open with a hex editor and go to offset 0x26 and change the value $02 to $0A (or any value greater than $08) and go to ARz Boss and wait for the hammer to hit the pillar.

    Best regards:
    --------------------
    Esrael Neto
     
  15. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Aha, thank you! I've found the problem:

    The problem is that the arrow object and the 'bulging eye' object are both subtypes of the pillar object, which itself is a subtype of the boss object (Obj89). Subtypes of the pillar object rely on 'obj89_pillar_parent' to detect if the boss is defeated, and, if it is, they will switch themselves to the 'lowering pillar' subtype. It doesn't make sense for the arrow or the bulging eye object to become a 'lowering pillar' object, so having these objects as subtypes of the pillar object is wrong. When these objects are created, their 'obj89_pillar_parent' variable is never set to anything, which is why they end up being 0. Worse yet, the arrow subtype actually overwrites 'obj89_pillar_parent' with 'obj89_arrow_routine', meaning that 'obj89_pillar_parent' could be set to 0, 2, 4, 6, or 8.

    To fix this issue, the 'Obj89_Arrow' and 'Obj89_Pillar_BulgingEyes' entries in 'Obj89_Pillar_Index' should be moved to the end of 'Obj89_Index'. Then, 'Obj89_Pillar_Shoot' needs modifying so that instead of this...
    Code (ASM):
    1.         move.b  #4,boss_subtype(a1)
    2.         move.b  #8,routine_secondary(a1)        ; => Obj89_Pillar_BulgingEyes
    ...it does this:
    Code (ASM):
    1.         move.b  #8,boss_subtype(a1)     ; => Obj89_Pillar_BulgingEyes
    And, instead of this...
    Code (ASM):
    1.         move.b  #4,boss_subtype(a1)
    2.         move.b  #6,routine_secondary(a1)        ; => Obj89_Arrow
    ...it does this:
    Code (ASM):
    1.         move.b  #6,boss_subtype(a1)     ; => Obj89_Arrow
    With this fix, it is no longer necessary to add checks to 'Obj89_Pillar' and 'Obj89_Arrow'.
     
  16. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Here is a bug that has been bothering me for almost seven years: in Casino Night Zone Act 2, the background has a strange bug where the top of it is missing. This is actually partly caused by a workaround for a similar bug in 'DrawInitialBG': this workaround checks for if the current zone is Casino Night Zone, and offsets the region of the background that gets uploaded to the VDP. Removing this workaround does fix Casino Night Zone Act 2... but it breaks Act 1: now Act 1 will be missing the bottom of its background.

    Of course, you could just make the workaround only apply to Act 1, and while this would "fix" the issue, it is only treating the symptom of a bug, and not the cause. We can do better than this.

    So why is any of this happening? Well, the job of 'DrawInitialBG' is to load just enough of the background to cover the screen (and then some extra). To do this, it selects a region of background around the camera and uploads it to the VDP's Plane B. What's notable about the missing top and bottom of the background? They're really far away from where the camera is at the start of the level. As you can guess, these parts of the background simply aren't uploaded to the VDP for display. Why isn't this an issue for other levels? Because other levels dynamically stream the background to the VDP as the camera moves around, and Casino Night Zone just doesn't. Now, this alone isn't a bug, mind you: it's an optimisation, as Casino Night Zone's entire background can fit in Plane B, eliminating the need to dynamically stream off-screen parts of it.

    The bug is that 'DrawInitialBG' isn't aware of this: if the camera is high enough, then the function will skip loading the bottom of the background, and load data from above the top of the background instead (which, in this case, is just blank). This is fine for backgrounds that intended for this, but not for Casino Night Zone's.

    Hopefully you now understand how the hack in 'DrawInitialBG' works: it pushes the region of the background to load downwards, to mitigate the camera being too high.

    Sonic 3 & Knuckles appears to fix this, by giving each zone its own code for loading its background in the 'LevelSetup' function.

    Our fix, however, will be a bit simpler: you see, the answer is sitting right in front of us: there's one other level that doesn't dynamically load its background, and its background is also prone to not being loaded fully by 'DrawInitialBG'. That level is Mystic Cave Zone... in two player mode.

    In single player mode, Mystic Cave Zone streams its background like many other zones, but in two player mode, it can fit its whole background in Plane B. This is because, in two player mode, Plane B is twice as tall: 64x64 instead of 64x32. 'DrawInitialBG' is only capable of pre-loading 64x32 tiles, so it can't be relied on to load this level's background. To avoid this problem, Mystic Cave Zone 2P just uses its own background initialisation code, which can be found at 'loc_E396'.

    This solution bypasses the dynamic background loader completely, not only allowing it to load more tiles than the dynamic loader could, but also bypassing the issue that affects Casino Night Zone. So, Casino Night Zone should do this too, except only loading 64x32.

    Here's a function that does just that:
    Code (ASM):
    1. DrawInitialBG_LoadWhole64x32Plane:
    2.         moveq   #0,d4
    3.  
    4.         moveq   #16-1,d6 ; Height of plane in blocks
    5. -       movem.l d4-d6,-(sp)
    6.         moveq   #0,d5
    7.         move.w  d4,d1
    8.         ; This is just a fancy efficient way of doing 'if true then call this, else call that'.
    9.         pea     +(pc)
    10.         tst.w   (Two_player_mode).w
    11.         beq.w   CalcBlockVRAMPos_NoCamera
    12.         bra.w   CalcBlockVRAMPos_2P_NoCamera
    13. +
    14.         move.w  d1,d4
    15.         moveq   #0,d5
    16.         moveq   #32-1,d6 ; Width of plane in blocks
    17.         move    #$2700,sr
    18.         bsr.w   DrawBlockRow3
    19.         move    #$2300,sr
    20.         movem.l (sp)+,d4-d6
    21.         addi.w  #16,d4
    22.         dbf     d6,-
    23.  
    24.         rts
    Now, in 'DrawInitialBG', replace this...
    Code (ASM):
    1.         moveq   #0,d4
    2.         cmpi.b  #casino_night_zone,(Current_Zone).w
    3.         beq.w   ++
    ...with this:
    Code (ASM):
    1.         cmpi.b  #casino_night_zone,(Current_Zone).w
    2.         beq.w   DrawInitialBG_LoadWhole64x32Plane
    And with that, Casino Night Zone's background is fixed.

    However, Casino Night Zone isn't the only level that uses a static background: Emerald Hill Zone and Hill Top Zone do too. In fact, they both suffer from the same bug as Casino Night Zone: you just can't see it because the camera never lets you go low enough to see the bottom of the background being cut off. We might as well extend this bugfix to address those two as well: just make those zones branch to the new function too, like this:
    Code (ASM):
    1.         move.b  (Current_Zone).w,d0
    2.         cmpi.b  #emerald_hill_zone,d0
    3.         beq.w   DrawInitialBG_LoadWhole64x32Plane
    4.         cmpi.b  #casino_night_zone,d0
    5.         beq.w   DrawInitialBG_LoadWhole64x32Plane
    6.         cmpi.b  #hill_top_zone,d0
    7.         beq.w   DrawInitialBG_LoadWhole64x32Plane
    EDIT: I forgot, you might need to defined some labels: 'CalcBlockVRAMPos_2P_NoCamera' is 'loc_E2AC', and 'CalcBlockVRAMPos_NoCamera' is a label that you should insert after the 'add.w 4(a3),d4' in 'CalcBlockVRAMPos2'.
     
    Last edited: Apr 22, 2022
  17. LuigiXHero

    LuigiXHero

    Member
    45
    33
    18
    Sonic 1 Character Pak

    Edit: added the last paragraph
     
    Last edited: Apr 21, 2022
  18. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Oh, I get it now. Thank you. How the heck did I miss that twice?
     
  19. Uh, CalcBlockVRAMPos_NoCamera and CalcBlockVRAMPos_2P_NoCamera don't seem to exist in a regular S2 disassembly.
     
  20. Clownacy

    Clownacy

    Tech Member
    1,053
    581
    93
    Oh right, whoops. I have some changes to the Git disassembly that I haven't pushed yet: 'CalcBlockVRAMPos_2P_NoCamera' is 'loc_E2AC', and 'CalcBlockVRAMPos_NoCamera' is a label that you should insert after the 'add.w 4(a3),d4' in 'CalcBlockVRAMPos2'.