No Magic Numbers

[dustinlacewell]

This issue means to discuss and track an effort to remove all magic numbers from Z64Lib. Enumerations and constants ought to be utilized instead. These can be documented and their meaning discovered in use sites throughout the code using IDE features.

Totally random example:

https://github.com/hylian-modding/ML64-Z64Lib/blob/master/cores/Z64Lib/src/Common/CommandBuffer/CommandBuffer.ts#L465

    runWarp(entranceIndex: number, cutsceneIndex: number, age?: number, transition?: number): Promise<boolean> {
        let count = this.ModLoader.emulator.rdramRead16(this.cmdbuf);
        let offset = this.cmdbuf + COMMAND_OFFSET + COMMAND_SIZEOF * count;

        this.ModLoader.emulator.rdramWrite16(this.cmdbuf, count + 1);
        this.ModLoader.emulator.rdramWrite32(offset, CommandBuffer_CommandType.WARP);
        this.ModLoader.emulator.rdramWrite32(offset + 8, entranceIndex);
        this.ModLoader.emulator.rdramWrite32(offset + 8 + 4, cutsceneIndex);

        if (age === undefined) {
            this.ModLoader.emulator.rdramWrite32(offset + 8 + 8, 0xFFFFFFFF);
        }
        else {
            this.ModLoader.emulator.rdramWrite32(offset + 8 + 8, age);
        }

        if (transition === undefined) {
            this.ModLoader.emulator.rdramWrite32(offset + 8 + 0xC, 0xFFFFFFFF);
        } else {
            this.ModLoader.emulator.rdramWrite32(offset + 8 + 0xC, transition);
        }

        return new Promise((accept, reject) => {
            accept(true)
        });
    }

In the above,

        let offset = this.cmdbuf + COMMAND_OFFSET + COMMAND_SIZEOF * count;

Is much better than:

this.ModLoader.emulator.rdramWrite32(offset + 8 + 4, cutsceneIndex);

There are a significant number of magic numbers in the code so this issue represents a significant effort. However, capturing low-level hard-to-obtain knowledge in the semantics of the code will help increase accessibility to contribution and long term maintainability.

[Drahsid]

What are you complaining about here? The magic numbers (the +8, +x... etc.) in your example above are specific to each command (and type,) in the command buffer, so it would be much more disorganized to enumerate them (it would more than double the amount of code for each,) since they are only used once. Additionally, this isn't something the end-user will touch or see, this is pretty much entirely maintained my both myself and @denoflionsx. Do you have any other examples of this that are more significant?

I suppose the +4 (for the uuid), and +8 (for command params) for commands specifically could be enumerated, but quite frankly I've spent more time writing this than it would take to change this. If you would like to enumerate the stuff, open a PR and we can review it

[dustinlacewell]

Hey @Drahsid!

I do not mean to call out this code in particular, I picked out pretty much at random.

I had envisioned instead of something like,

this.ModLoader.emulator.rdramWrite32(offset + 8 + 8, age);

there could be

this.ModLoader.emulator.rdramWrite32(offset + WRITE_ARGS_BASE + (WRITE_ARG_SIZE * 2), age);

or even

this.ModLoader.emulator.rdramCall(
  CommandBuffer_CommandType.WARP, [
    entranceIndex,
    cutsceneIndex,
    age ?? 0xFFFFFFFF,
    transition ?? 0xFFFFFFFF])

though that isn't to say something like rdramCall wouldn't want to use enums/consts underneath.

runWarp may not be the most poignant example to give regarding magic numbers, and I didn't mean to call anything about this code out in particular. Simply, the goal would be to understand what the code does by reading it without foreknowledge. I understand this is a core library, but I'm a nerd and just remarking as I go through the code hehe.

Finally, this wasn't a call on anyone in particular to get to doing anything as it would be a big effort, no doubt. I opened this issue simply to track the idea of replacing magic numbers with enums/consts throughout the codebase in general. Thanks for the quick reply!

[Drahsid]

Hey, no worries about mentioning this issue. I didn't mean to make you feel like we were upset or anything. As I edited earlier, I think in this case (for Commands, not to be confused with CommandEvents and CommandReturns,) having +4 be enumerated as COMMAND_UUID_OFFSET and +8 be COMMAND_PARAMS_OFFSET or something along those lines might be a good change. Though I don't see a way to organize the individual command parameter offsets that isn't messier than just having the offset from the start of the params list. Unfortunately, because this is a frontend implementation of our compiled code, we can't just have a struct definition and write directly to memory using that as we would in c.

There are likely other magic numbers strewn throughout the codebase, it's hard to keep track of, but the more we refactor, the more modular we can make the mod, so if you spot anything, append them to the issue, and we can definitely check them out.

And again, if you see something that you can change for the better, we should be able to review PRs fairly quickly, and there isn't much reason to deny merging good code.