Add a FixBugs flag and document and fix multiple bugs.

[Totally-Not-Filter]

I went through and added a FixBugs flag to this disassembly so it is consistent with the other two Sonic disassemblies. This closes issue #62.

[Brainulator9]

To be honest, I'm not a fan of the FixBugs flag in the Sonic 1 or 2 disassemblies. Maybe this is just a me thing, but I think having the disassemblies come with pre-included bugfixes presents problems that would not appear otherwise:

• It is possible that certain corrections may end up causing more problems, whether because the solution creates a new bug or because the fix is overly complicated.
• There may be multiple ways to fix certain mistakes; who's to say the one provided is the best solution? Doubly so if the one provided differs from how the bug was fixed between Sonic 1 and 2 or 2 and 3.
• Certain desirable behaviors are forcibly removed.

And, on a personal philosophical level, I see these disassemblies as being more geared towards documentation; something that's easier/better to make hacks from would, in many cases, require much more major modifications to the code than what a presumably conservative disassembly would contain. Sure, adding a VRAM DMA queue to Sonic 1 would probably be beneficial to the hacker, but that feels too out-of-scope for this project. Although, then again, perhaps the "complete" build option already has some of that going on for this project.

[Totally-Not-Filter]

Yeah, I just sort of wanted the disassemblies to be consistent rather than inconsistent, and for stability purposes it would be better without it. I'll be closing this pull-request.

[Clownacy]

The goal of providing bugfixes is not to make the disassembly more hacker-friendly: bugfixes are documentation, as they show what the code was meant to do. To document a bug without explaining how to fix it is not truly documenting the bug. While one could explain the fix in plain English instead of providing code, it would be needless obfuscation, as it would either not paint a clear-enough picture of how exactly the bug should be fixed, or it would be so detailed that it would practically be verbose pseudocode. The best way is to provide code and explain how it fixes the bug.

Before they were integrated into the disassembly, bugfixes were scattered around a wiki and multiple threads across various forums, and they often targetted different disassemblies. This fragmentation makes it needlessly difficult to find bugfixes, as well as to understand them, whereas having them within the disassembly centralises them and makes them target a common disassembly.

It is possible that certain corrections may end up causing more problems, whether because the solution creates a new bug or because the fix is overly complicated.

If a bugfix creates a problem, then the bugfix should be improved. Just because something has the potential to be done badly does not mean it should not be done at all. There could be some bugs that cannot be solved with a simple modification, and those massive overhauls and refactorings are out-of-scope for the disassembly, but that does not mean that simple bugs cannot have simple fixes provided for them.

There may be multiple ways to fix certain mistakes; who's to say the one provided is the best solution? Doubly so if the one provided differs from how the bug was fixed between Sonic 1 and 2 or 2 and 3.

Bugfixes that are backported from later games should function exactly as they do in said later games (unless those bugfixes are themselves broken, in which case an improved fix should be made available in both disassemblies).

As for who is the arbiter of which is the best way to fix a bug, the same can be said about many aspects of a disassembly: how files should be split, how labels should be named, whether the build tools should be optimised for size or speed, etc. As with those, how bugs are fixed is simply down to the discretion of the people maintaining the disassembly.

Certain desirable behaviors are forcibly removed.

If a person desires bugs, then they are free to not enable the bugfixes. If a person disagrees on what is or is not a bug (such as the spike bug), then they can search through the disassembly, removing any fixes that they do not approve of.

[Brainulator9]

The goal of providing bugfixes is not to make the disassembly more hacker-friendly: bugfixes are documentation, as they show what the code was meant to do. To document a bug without explaining how to fix it is not truly documenting the bug.

So what part of, in s2disasm, does:

• Adding an entire subroutine to display all of Oil Ocean Zone's background
• Modifying RAM to fix a bug with spindashing and the camera
• Changing FM note value selection from a pure look-up table to one that is partially calculated

serve to document code? In the case of the first two, they go far beyond "simple fixes for simple bugs".

While one could explain the fix in plain English instead of providing code, it would be needless obfuscation, as it would either not paint a clear-enough picture of how exactly the bug should be fixed, or it would be so detailed that it would practically be verbose pseudocode. The best way is to provide code and explain how it fixes the bug.

When I see:
; Bug: this should be d1
clr.w d0
I think "gee, maybe this can be fixed by substituting the correct character". I suppose this wouldn't help bugs of the "this instruction is misplaced" type, but that still seems feasible to succinctly explain.

If a bugfix creates a problem, then the bugfix should be improved. Just because something has the potential to be done badly does not mean it should not be done at all.

My concern is that people can and will just use whatever bugfixes are in the disassembly at any given time, without hooking up their project to s2disasm such that other bugfixes or improvements to bugfixes can be made. And when those bugfixes are made by people who don't know what they're doing (especially if they think they do), then...

[Totally-Not-Filter]

Honestly, I could just make it so that more bugs are documented with relatively okay fixes as I'm going through the entirety of the disassembly and write up an optional bug fix guide rather than using a flag if that helps. I'm just wanting to fix and document some bugs that go rather unnoticed during normal gameplay and once you see them you can no longer unsee them. I'm totally okay with getting rid of the FixBugs flag entirely and just making it so that fixes are included with comments near the code rather than using the optional bug fix flag. If we want to go in that direction we can.

[Clownacy]

The goal of providing bugfixes is not to make the disassembly more hacker-friendly: bugfixes are documentation, as they show what the code was meant to do. To document a bug without explaining how to fix it is not truly documenting the bug.

So what part of, in s2disasm, does:

* Adding an entire subroutine to display all of Oil Ocean Zone's background

* Modifying RAM to fix a bug with spindashing and the camera

* Changing FM note value selection from a pure look-up table to one that is partially calculated

serve to document code? In the case of the first two, they go far beyond "simple fixes for simple bugs".

Adding an entire subroutine to display all of Oil Ocean Zone's background

Instead of splitting the shared code to a common subsystem, Sonic 2's developers created the Chemical Plant background code by modifying a duplicate of Scrap Brain's function, so it stands to reason that, had they applied the same subsystem to Oil Ocean, they would have done the same thing. The actual modifications to the function are quite minimal. There are comments beneath Draw_BG3_CPZ and Draw_BG3_OOZ that try to explain this.

Refactoring the three functions to eliminate duplicate code seems out-of-scope for such a fix, though converting them to use a big macro might be useful for doing so while still remaining bit-perfect with the original game.

In any case, this bugfix shows how the developers should have addressed the issue of the vanilla background drawer not being up to the task, which is by using the fancy Scrap Brain/Chemical Plant drawer.

Modifying RAM to fix a bug with spindashing and the camera

Alternatively, an unused byte of RAM could be used to store the new flag instead. I did say that suboptimal bugfixes should be improved. Regardless, this does not contradict the idea of bugfixes serving to document bugs; this one just happens to be inefficient. As for whether this counts as a not-simple bugfix, that would depend on who you ask, since simplicity is subjective.

Changing FM note value selection from a pure look-up table to one that is partially calculated

That is an optimisation, not a bugfix; it is not guarded by the FixDriverBugs constant.

While one could explain the fix in plain English instead of providing code, it would be needless obfuscation, as it would either not paint a clear-enough picture of how exactly the bug should be fixed, or it would be so detailed that it would practically be verbose pseudocode. The best way is to provide code and explain how it fixes the bug.

When I see: ; Bug: this should be d1 clr.w d0 I think "gee, maybe this can be fixed by substituting the correct character". I suppose this wouldn't help bugs of the "this instruction is misplaced" type, but that still seems feasible to succinctly explain.

Not all bugs are so extremely simple, like the noise channel needing to be muted in zPSGNoteOff, the two-player mode sprite table race condition bug, and RunObjectDisplayOnly not working properly for multi-sprite objects.

If a bugfix creates a problem, then the bugfix should be improved. Just because something has the potential to be done badly does not mean it should not be done at all.

My concern is that people can and will just use whatever bugfixes are in the disassembly at any given time, without hooking up their project to s2disasm such that other bugfixes or improvements to bugfixes can be made. And when those bugfixes are made by people who don't know what they're doing (especially if they think they do), then...

I don't understand what you're saying here.