Code review

[atomheartother]

Hey, you asked for a code review, I'll go through your code and review it in this issue. I'll start with chip8.js.

Your entire time measuring system is broken:

function step() {
    now = Date.now()
    elapsed = now - then
    if (elapsed > fpsInterval) {
        cpu.cycle()
    }

    loop = requestAnimationFrame(step)
}

You never re-assign then, so the condition is always true, so first of all you might as well do this:

function step() {
    cpu.cycle()
    loop = requestAnimationFrame(step)
}

This is actually already a slight improvement on your code, and since browsers will typically run requestAnimationFrame 60 times per second, it works just fine for display. You already do multiple clock cycles in one cycle() call anyway with this.speed in the CPU, so the actual CPU speed isn't locked by this change.

That's all I have to say about timing, I'll do more in another comment.

[atomheartother]

I was extremely confused by the cpu.loadSpritesIntoMemory() call in chip8.js, but then i realized you actually store the memory inside the CPU. Personally, for separation of concerns, I would have a separate Memory class, but for a Chip-8 in particular it's fine to store it in the CPU if you're comfortable with that.

However I still don't understand why chip8.js needs to tell the CPU to load the sprites into memory at init? Memory will always contain the sprites, so you're just wasting time here. Just put the sprites into memory in the CPU's constructor, it'll save time and be better for separation of concerns, chip8.js should have nothing to with telling the CPU when to load sprites into memory.

[atomheartother]

The entire loadRom function is useless as you use loadProgramIntoMemory now, it can go.

[atomheartother]

Ok, now for the monster in the room, your gigantic switch/case of a CPU.

You need to use a better system than that. Chip-8 in particular is super easy to use a lookup table with, because you have 0x10 operands possible, matching the first 4 bits of the opcode, so you can just make a function array of 16 entries and have each instruction redirect to the proper function:

        this.instructionFns = [
            clsRet,
            jp,
            call,
            // ...
            f,
            ld
        ]

// then...
    executeInstruction(opcode) {
        this.pc += 2

        const x = (opcode & 0x0F00) >> 8
        const y = (opcode & 0x00F0) >> 4

        this.instructionFns[(opcode & 0xF000) >> 12](opcode, x, y)
}

This has multiple advantages:

• It's faster, like, way faster.
• It makes your code more legible
• It lets you debug more easily as putting some statements in case blocks is a pain in most languages (and I believe JS is one of those)
• It lets you break your code into smaller functions, which is definitely generally good practice

[atomheartother]

Oh also I forgot to mention, you may have noticed what I did above but, x and y should be const, not let. You'll see me do that in a few places in your code, it's just good practice to specify when a variable is const.

Also this is a minor gripe but this:

        this.pc += 2

Probably belongs here:

            if (!this.paused) {
                const opcode = (this.memory[this.pc] << 8 | this.memory[this.pc + 1])
                this.pc += 2
                this.executeInstruction(opcode)
            }

Why? Because the bit of code to get the opcode fetches data using this.pc, and starting now pc should be incremented, there is no reason to wait until the instruction is executed. I know it's the 1st thing you do, but semantically the pc incrementation shouldn't be the job of the execution function. Again though, this is a super tiny nitpick.

[atomheartother]

You keep re-calculating these values:

opcode & 0xFF
opcode & 0xFFF

It would be a bit cleaner to write small functions to do these calculations:

const kk = (opcode) => opcode & 0xFF;
const nnn = (opcode) => opcode & 0xFFF;

That or pre-calculate them like you did with x and y:

const kk = opcode & 0xFF;
const nnn = opcode & 0xFFF;
// Actually look at the opcode now

[atomheartother]

                        if ((sprite & 0x80) > 0) {
                            // If setPixel returns 1, which means a pixel was erased, set VF to 1
                            if (this.renderer.setPixel(this.v[x] + col, this.v[y] + row)) {
                                this.v[0xF] = 1;
                            }
                        }

So this may be me misreading the spec, but I don't see it said anywhere that sprites should only be xor'd onto the screen when the bit is set, at least not in the documentation I'm reading:

The interpreter reads n bytes from memory, starting at the address stored in I. These bytes are then displayed as sprites on screen at coordinates (Vx, Vy). Sprites are XORed onto the existing screen. If this causes any pixels to be erased, VF is set to 1, otherwise it is set to 0. If the sprite is positioned so part of it is outside the coordinates of the display, it wraps around to the opposite side of the screen.

If I am correct in my interpretation, then you should be doing this:

                        // If setPixel returns 1, which means a pixel was erased, set VF to 1
                        if (this.renderer.setPixel(this.v[x] + col, this.v[y] + row, !!(sprite & 0x80))) {
                            this.v[0xF] = 1;
                        }

and also:

    setPixel(x, y, set){
        let pixelLoc = (x % this.cols) + ((y % this.rows) * this.cols)
        const prevSet = this.display[pixelLoc];
        this.display[pixelLoc] ^= set

        return prevSet && !this.display[pixelLoc]
    }

And yeah, while I'm at it, pixel values can only overflow in the positives (you start from a positive number, and then go right for X, and down for Y) so you can skip a few conditions and just modulo the result.

Even if you could overflow negatively though, you would have some issues with your current code since you only apply x -= this.rows once, when technically the chip-8 should be able to ask you to draw a pixel at this.rows * 3 and you should still be able to handle it.

[atomheartother]

The way you put your ROMs in a JSON file in your repo is.... Interesting? It's definitely not something that would fly normally, why not just have the client request the ROM from the server, and read them dynamically that way? I can't imagine there's some huge limitation here. Anyway this makes changing your ROMs dynamically extremely slow for you, and also it means you don't actually need your roms/ folder in your repo since you're already shipping the ROMs in data/

But just to be clear, this doesn't mean you should be removing the roms/ folder, clearly you should instead not use a huge JSON file and serve the ROMs from the server to the client on request.

This would, incidentally, make your app way less network-heavy, since right now you're having to request the entire JSON file every single time you wanna start a game which isn't ideal.

[atomheartother]

Also while I'm here the whole "paused" system is an inventive solution to a few chip-8 behaviors, but I believe timers should still tick down while paused.

[atomheartother]

I'm done for now but I hope this little review helped you :)

[umairayub79]

Hey Liz, Thank you for everything.

why put roms in a json file? you ask well I was experimenting 😅 I thought ROMs would load faster that way but that didn't make much difference tho

Thanks Again, Have a great weekend.

Regards.