PPU Random Stripes of Color

PPU Random Stripes of Color
by ace314159 on 2018-04-17 (#216979)

Hi, I've been trying to develop an NES emulator and my PPU has been displaying stripes of incorrect color. The red color is color 0 of palette 0, so I've associated the problem with either shifting, loading, or reading the 8 bit attribute registers, but I'm not sure what's wrong.

From what I've read on the forums and wiki, in order to load the registers I have to pass the attribute byte through a 4 to 1 multiplexer with bit 1 of the coarse X and coarse Y as select bits and fill the registers with that:

Code:

uint8_t yBit = (this->currentVramAddr & 0x40) >> 5; // Bit 1 of coarse y in pos 1
uint8_t xBit = (this->currentVramAddr & 0x2) >> 1; // Bit 1 of carse x
// yx is used to select the corresponding 2 bits from the attribute byte
uint8_t paletteNum = (this->atByte >> ((yBit | xBit) * 2)) & 0x3;
this->highAttrShiftRegister = (paletteNum >> 1) * 0xFF;
this->lowAttrShiftRegister = (paletteNum & 0x1) * 0xFF;

I shift all 4 registers between cycles 2 and 257, and 322 and 337:

Code:

if(this->cycleNum >= 2 && this->cycleNum <= 257 || this->cycleNum >= 322 && this->cycleNum <= 337) {
   this->lowBGShiftRegister <<= 1;
   this->highBGShiftRegister <<= 1;         
   this->lowAttrShiftRegister <<= 1;
   this->highAttrShiftRegister <<= 1;         
}

I get the palette number by passing the shift register into a 8 to 1 multiplexer with the fine scroll as the select bits:

Code:

uint8_t lowAttrBit = (this->lowAttrShiftRegister >> (7 - this->fineXScroll)) & 0x1;
uint8_t highAttrBit = (this->highAttrShiftRegister >> (7 - this->fineXScroll - 1)) & 0x2;

Is my understanding correct? Thanks for any help!

Re: PPU Random Stripes of Color
by Quietust on 2018-04-17 (#217012)

ace314159 wrote:

I get the palette number by passing the shift register into a 8 to 1 multiplexer with the fine scroll as the select bits:

Code:

uint8_t highAttrBit = (this->highAttrShiftRegister >> (7 - this->fineXScroll - 1)) & 0x2;

When fineXScroll is 7, that will try to shift right by a negative number, which I'm pretty sure is Undefined Behavior™.

What happens if you change it to this?

Code:

uint8_t highAttrBit = ((this->highAttrShiftRegister >> (7 - this->fineXScroll)) & 0x1) << 1;

Re: PPU Random Stripes of Color
by ace314159 on 2018-04-17 (#217023)

I feel so stupid now! Thanks so much! The stripes are no longer there, but there are the random blocks which are either white or red instead of the correct color. I looked at Super Mario Bros, and saw that the incorrect color was there for certain pixels in the same column.

I'm pretty sure that my VRAM is fine, because I checked the dumps with Nintendulator at the end of the frame and saw that it matched (until 0x3000, the wiki said it doesn't matter because the ppu doesn't render from here).

Here's my code for determining the bg tile data. Maybe I messed something up here too:

Code:

uint8_t lowBGBit  = (this->lowBGShiftRegister  >> (15 - this->fineXScroll)) & 0x1;
uint8_t highBGBit = ((this->highBGShiftRegister >> (15 - this->fineXScroll)) & 0x1) << 1;

Reloading code:

Code:

if(this->cycleNum >= 9 && this->cycleNum <= 257 || this->cycleNum >= 329 && this->cycleNum <= 337) {
   this->highBGShiftRegister |= this->highTileByte;
   this->lowBGShiftRegister |= this->lowTileByte;

   uint8_t yBit = (this->currentVramAddr & 0x40) >> 5; // Bit 1 of coarse y in pos 1
   uint8_t xBit = (this->currentVramAddr & 0x2) >> 1; // Bit 1 of carse x
   // yx is used to select the corresponding 2 bits from the attribute byte
   uint8_t paletteNum = (this->atByte >> ((yBit | xBit) * 2)) & 0x3;
   this->highAttrShiftRegister = (paletteNum >> 1) * 0xFF;
   this->lowAttrShiftRegister = (paletteNum & 0x1) * 0xFF;
}

Re: PPU Random Stripes of Color
by Dwedit on 2018-04-17 (#217024)

It looks like the attributes aren't lined up with the tiles.
Super Mario Bros looks like it failed to read the title screen from CHR-ROM. PPU reads are effectively delayed by one read, so the game has to read a junk byte before reading the rest of the data.

Re: PPU Random Stripes of Color
by tokumaru on 2018-04-17 (#217028)

Doesn't SMB also rely on palette mirroring? Maybe that's worth checking out too.

Re: PPU Random Stripes of Color
by ReaperSMS on 2018-04-18 (#217064)

It does, but digging up memories of getting it working in an emulator suggests that when the palette mirroring is wrong, the sky turns black.

Is there an if wrapping that "Reloading code" that restricts it to just after the actual reads? The one as written looks like it would be reloading the shift register every pixel.

Re: PPU Random Stripes of Color
by ace314159 on 2018-04-18 (#217066)

@Dwedit, if you mean the accesses at $2007 described here, I've implemented it and tested it with blargg's test roms and the result is still the same for Mario Bros. But for Super Mario Bros, the play menu now appears, but the colors are still not correct.

@tokumaru I think I mirrored the palette correctly (at least according to blargg's test rom).

@ReaperSMS The if statement in the reloading code is checked every 8 cycles between dots 0-256 and 321-340 (inclusive) on all the visible scanlines + pre-render scanline. I run it on the cycles 1, 9, 17, 25, 33, etc. (basically when cycleNum % 8 == 1).

Re: PPU Random Stripes of Color
by lidnariq on 2018-04-18 (#217068)

What Dwedit said—the attributes start too far to the left by 7 pixels.

Re: PPU Random Stripes of Color
by ace314159 on 2018-04-18 (#217069)

I looked through my code manyyy times, but I can't seem to figure out where the attribute bytes are going wrong. Am I doing something wrong during the scanlines? Here's my code:

Code:

if(this->scanlineNum < 240 && this->isRendering()) { // Pre-render Scanline and Visible Scanlines
   if(this->cycleNum <= 256 || this->cycleNum >= 321) {
      this->fetchData();
      if(this->scanlineNum != -1 && this->cycleNum <= 256) this->renderDot();
      if(this->cycleNum >= 2 && this->cycleNum <= 257 || this->cycleNum >= 322 && this->cycleNum <= 337) {
         this->lowBGShiftRegister <<= 1;
         this->highBGShiftRegister <<= 1;
         this->lowAttrShiftRegister <<= 1;
         this->highAttrShiftRegister <<= 1;
      }

      if(this->cycleNum % 8 == 0) this->incrementScrollX(); // Inc.hori(v)
      if(this->cycleNum == 256) this->incrementScrollY(); // Inc. vert(v)
   }
   if(this->cycleNum == 257) // hori(v) = hori(t)
      this->currentVramAddr = (this->currentVramAddr & ~0x41F) | (this->tempVramAddr & 0x41F);


   if(this->scanlineNum == 239 && this->cycleNum == 256) {
      Graphics::renderScreen();
   }
}

fetchData contains the reloading of data and preparation of new data. renderDot() contains the code for finding the pixel colors.

Re: PPU Random Stripes of Color
by ReaperSMS on 2018-04-18 (#217073)

what does fetchData() look like? Does it have the proper checks to fetch the right bytes at the right times?

I suspect you are trampling the attribute data immediately, rather than letting it properly shift itself out.

Re: PPU Random Stripes of Color
by ace314159 on 2018-04-18 (#217076)

Code:

uint8_t fineY = (this->currentVramAddr >> 12) & 0x7;
uint16_t bgPage = (this->CTRL << 8) & 0x1000;
uint16_t v = this->currentVramAddr;
switch(this->cycleNum % 8) {
case 1: 
   if(this->cycleNum >= 9 && this->cycleNum <= 257 || this->cycleNum >= 329 && this->cycleNum <= 337) {
      this->highBGShiftRegister |= this->highTileByte;
      this->lowBGShiftRegister |= this->lowTileByte;

      this->highAttrShiftRegister = (this->paletteNum >> 1) * 0xFF;
      this->lowAttrShiftRegister = (this->paletteNum & 0x1) * 0xFF;
   }
   break;
case 2: // NT Byte
   this->ntByte = mem.get8(0x2000 | (v & 0x0FFF));
   break;
case 4: // AT Byte
   {
   uint8_t atByte = mem.get8(0x23C0 | (v & 0x0C00) | ((v >> 4) & 0x38) | ((v >> 2) & 0x07));
   uint8_t yBit = (v & 0x40) >> 5; // Bit 1 of coarse y in pos 1
   uint8_t xBit = (v & 0x2) >> 1; // Bit 1 of carse x
   // yx is used to select the corresponding 2 bits from the attribute byte
   this->paletteNum = (atByte >> ((yBit | xBit) * 2)) & 0x3;
   }
   break;
case 6: // Low BG Tile Byte
   this->lowTileByte = mem.get8(bgPage | (this->ntByte << 4) | fineY);
   break;
case 0: // High BG Tile Byte
   this->highTileByte = mem.get8(bgPage | (this->ntByte << 4) | fineY | 8);
   break;
}   

I update the shift register every 8 dots, so I think I'm letting it shift.

Re: PPU Random Stripes of Color
by ReaperSMS on 2018-04-18 (#217083)

I think I see what's happening. You should make your attribute shift registers 16 bits wide, to match the pattern registers, and update them in a similar fashion, by ORing a 00 or FF into the lower byte.

Your current code is stomping your entire attribute shift register the moment the new tile gets merged in, but that tile won't hit the screen for 1-8 more pixels depending on the fine X scroll, hence your attributes appearing to be 7 pixels off.

Re: PPU Random Stripes of Color
by ace314159 on 2018-04-18 (#217099)

That fixes everything! Thanks so much for the help!
Though, I'm wondering why the wiki mentions that the attribute shift register is only 8 bits wide. Am I doing something that forces me to use a 16 bit attribute shift register?

Re: PPU Random Stripes of Color
by tepples on 2018-04-18 (#217102)

It's because all pixels in a sliver (8x1-pixel area) have the same attribute, so the same bit can be shifted into the attribute register 8 times in a row.