Limit bugfixes to documentation only

[Clownacy]

As stated before on the forum, I think that bugfixes in their current form are a mistake, and should have remained purely as documentation in comments rather than buildable code:

• The goal of the disassemblies is to document the inner-workings of these games, not improve them. With that in mind, providing bugfixes is scope-creep, and forces extra work onto the disassembly maintainers, as now they have to not only point out that a bug exists, but write and test code to fix it. Improving the games should be left to forks that do not care about producing an accurate ROM.

• People do not always agree on what a bug is, let alone what the correct fix for it is. When bugfixes exist solely as documentation, it is simple enough to just say 'this might be a bug, and it could be corrected by doing X or Y', but when the bugfixes are code which is batch-enabled with a flag, then they're essentially authoritatively declaring that 'this is a bug, and this is the correct way to fix it'.

• Sometimes, suggesting how a bug can be fixed is way simpler and tidier than actually providing the fix. The two worst examples of this are...

  • The Oil Ocean Zone background bugfix, which requires a massive custom function that is full of duplicate code from Chemical Plant Zone's background (which itself is full of duplicate code from Scrap Brain Zone's background).
  • The 2-player mode sprite table page-flipping bugfix, which requires a bunch of extra code and $500 bytes of RAM to fix. This bugfix prevents people from being able to install Sonic 1's sound driver or my Clone Driver v2, as it uses up the spare RAM which those drivers would normally use.

• Some bugfixes can only be provided as documentation, leaving the FixBugs flag ironically unable to fix every known bug. Particularly, this is the case for bugs in the game's data rather than its code. For instance, how would the FixBugs flag fix the fact that Tails's arms are the wrong colour in the Special Stages? How would it fix the fact that Aquatic Ruin Zone's block data is too large, and that decompressing it causes its buffer to overflow?

  • Some data bugs could theoretically be "fixed" by having the game detect and patch erroneous data at runtime, but this is not a true bugfix and instead a workaround. This can already be seen in bugfixes which correct objects with invalid subtypes, like in Obj45_Init. These half-solutions fail to correct the root issue and clutter the codebase with nasty hacks which could just break more things.

• Providing bugfixes can cause some branches to be pushed out of range, requiring that extra conditional assembly logic be provided to extend those branches when bugfixes are enabled, adding yet more clutter to the codebase.

• Bugfixed code overall just makes the codebase an inflexible mess, as users and maintainers have to constantly accommodate two sets of code, one of which they likely do not care about (a ROM-hacker would likely not care about the original buggy code, while those interested in code accuracy would not care about the bugfixes). When adding a feature, one either has to be mindful that it works with both code paths, or disregard one of them entirely and just let it break (in which case there is now a bunch of useless unused broken code all over the place, making the disassembly harder to work with).

  • Having two sets of code in one codebase could be avoided by using Git branches, having one codebase that contains only the bugged code, and another which contains only the fixed code. This would solve the issue of the two sets of code conflicting with each other, but then what you really have is just a bug-fixed fork, and at that point that may as well be a third-party project because it has completely deviated from the disassembly's goal of providing a codebase which produces an accurate ROM.

• Some bugfixes simply cannot exist without the game being refactored or optimised first to accommodate it, once again creating scope-creep where the focus is not just on fixing bugs, but changing unrelated, perfectly-working code so that the bugfix can even be there in the first place.

  • One instance of this is the Spin Dash camera delay bugfix, which requires an extra byte of RAM, which is obtained by optimising a word-sized variable down to byte-sized and then repurposing the spare byte. This bugfix is modifying completely unrelated code.
  • Z80 sound driver bugfixes can fall victim to this, due to the extremely limited Z80 RAM being exhausted by the introduction of extra code, requiring that size optimisations to the driver's code be added along with the bugfixes.
  • Were there not $500 bytes of unused RAM conveniently lying around, then the 2-player mode sprite table page-flipping bugfix would have been the absolute worst-case version of this, as it would necessitate extensive modifications to the game to free-up the RAM needed to fix the bug.

Overall, proving fixes for every bug in the game, whilst also providing the original bugged code/data and producing an accurate ROM, is simply not feasible; these goals clash horribly and just make the disassemblies far worse than if they had just chosen to do one or the other.

[Brainulator9]

looks at sonicretro/skdisasm#67 and then back here Well.

Would this extend to the sound driver's optimizations?

[AlexField442]

I agree with this, I feel that fixing bugs in general should be something the person hacking should handle, as it is honestly a good way to ensure that they don't create their own programming errors. The only exception I would make would be to bugs that could potentially break the game, such as the "multisprite objects using wrong draw function" thing.

That being said, I know many would be annoyed that suddenly the bug fixes would be removed, especially ones that are difficult to implement or are buried in the 'fix-it megathread'. So perhaps we could go with the bug-fixed fork idea, marking all code changes similar to Knuckles in Sonic 2.

[Clownacy]

looks at sonicretro/skdisasm#67 and then back here Well.

Would this extend to the sound driver's optimizations?

For consistency, sure. Like bugfixes, they too are outside the scope of an accuracy-focussed disassembly, create extra work for maintainers, and create clutter for people using the codebase.

[kirisamemofo]

I'd like to add an additional point in regards to the overall visual look of code with the fixBugs flag in place. Here's a simple example:

    if fixBugs
        ; Multi-sprite objects cannot use the 'priority' SST value, so they
        ; must use 'DisplaySprite3' instead of 'DisplaySprite'.
        ; This object's 'priority' is overwritten by 'sub3_y_pos', causing it
        ; to display on the wrong layer.
        move.w  #object_display_list_size*0,d0
        jmp     (DisplaySprite3).l
    else
        jmpto   JmpTo_DisplaySprite
    endif

For this example, let's change it to something like this, where the fixed code is turned into a comment:

        ; BUG: Multi-sprite objects cannot use the 'priority' SST value, so they
        ; must use 'DisplaySprite3' instead of 'DisplaySprite'.
        ; This object's 'priority' is overwritten by 'sub3_y_pos', causing it
        ; to display on the wrong layer.

        ; Here's what the fixed code should look like:
        ; move.w  #object_display_list_size*0,d0
        ; jmp     (DisplaySprite3).l
            
        jmpto   JmpTo_DisplaySprite

The tab spacing before each line here is made more consistent without the if/else/endif, and in a text editor with syntax highlighting, the comments and fixed code will be colored differently from the actual code. In my opinion, this is overall cleaner approach to providing fixed code without using a build flag and having to maintain the codebase around it, and works decently well enough for simple fixes like this.

[AlexField442]

Another thing, it might be worth including links back to the guides the fixes originated from. Either ones on the Retro wiki, or the forums, as not all fixes are on either.

[Clownacy]

We'll need a convenient way of identifying bug documentation too, so that people can easily search for it. Devon's earlier example used 'BUG:', and I think I've seen decompilations use something more akin to '[BUG]'. Any thoughts?

Personally I think 'BUG:' can too easily be mistaken for part of a sentence rather than a tag, and it would also likely require that searches be done in case-sensitive mode to avoid false-positives. Using square brackets helps it stand out better visually, and is distinct enough to not require a case-sensitive search to find.

[AlexField442]

[BUG] should work.

Of note, so far we include fixes so that the ARZ leaf generators and HTZ/MTZ lava generators display in debug mode, as they are setup for it (but lack the function to actually display themselves). Perhaps for consistency, the rest of the debug objects setup (those being, I believe, the pathswappers and autorollers, maybe others) could have similar changes applied, as well as the two objects that did display in REV00 before their code was removed.