ericgrandt / chip8-emulator

31 stars 21 forks source link

Question/error #1

Closed QuarkNerd closed 3 years ago

QuarkNerd commented 3 years ago

Overall, great tutorial, thank you very much. I do have one concern

In the renderer, we have this code.

if (x > this.cols) {
    x -= this.cols;
} else if (x < 0) {
    x += this.cols;
}

if (y > this.rows) {
    y -= this.rows;
} else if (y < 0) {
    y += this.rows;
}

However, surely the the >s should be >= since x and y start at 0? if x is 64, we want to treat that as a 0?

Bur, I tried to fix this in your code and it causes the blitz game to fail, so either I'm missing something or there is another mistake in the code.

ericgrandt commented 3 years ago

Thanks for the question @QuarkNerd. The reason for this is due to the way the display is set up and how we go about plotting pixels on that display. Our initial display is this: this.display = new Array(this.cols * this.rows);. And we display pixels based off the location in that array which is calculated as follows: let pixelLoc = x + (y * this.cols);. So we aren't displaying pixels based on an (x, y) coordinate, but instead we are displaying them based on a calculation of the x and y position passed in. So for example if the following function is called, setPixel(64, 32), the function would end up toggling the last pixel in the display (this.display[2048]).

If you swapped out the > with >= then you would not be able to toggle pixels in the last row or the last column. Hopefully that makes sense.

QuarkNerd commented 3 years ago

but x=64 , y=32 , this.cols=64 gives a value of 2112 not 2048. And the largest index in this.display should be 2047, as it is 0 indexed

ericgrandt commented 3 years ago

You're correct. Sorry about that.

It's been a very long time since I've looked at this code, but my guess is that when setPixel is called from cpu.js (https://github.com/Erigitic/chip8-emulator/blob/master/scripts/cpu.js#L259), it's adding a number to it that causes that if statement to make sense and not go out of bounds of that array..

QuarkNerd commented 3 years ago

Okay, so I've done some more digging. Turns out this is a common problem https://www.reddit.com/r/EmuDev/comments/bpjutr/chip8_y_wrap_or_no_wrap/

So I still think your code has a problem with how the wrapping works. However to make it properly work (if you want) you would need a toggle to turn Y-wrapping on and off.

Anyway, thank you for the tutorial, it was very helpful :)

ericgrandt commented 3 years ago

Glad the tutorial was helpful. I'm going to close this issue as I don't have plans to make any modifications at this time, but it has definitely resparked my interest in trying to understand these if statements and why they are the way they are. Thanks.