OneLoneCoder / olcNES

NES Emulator, and Tutorial Video Code
1.36k stars 229 forks source link

Make the PPU background color available and mirroring the pallet. #15

Open kentwidman opened 5 years ago

kentwidman commented 5 years ago

At line 210 We need to shift tile_msb so we can get values ranging from (0,1,2,3), instead of (0,1,2). Would this disable the background color/transparency??? There might be a more elegant way to do this than the way I did it. Only caught this after another YouTuber point it out, and your explanation earlier in the video was so good that I follow the logic behind this Nintendo cleverness.

At line 532: Again, big fan of your videos and I have learned so much about the NES and bitwise manipulation. I'm very grateful for everything you share and cover. I'm excited about the second PPU and sound episodes. Please double-check this suggestion, as I might have made a mistake and there is always a chance I might be wrong. Maybe it doesn't matter but shouldn't every 4th + 1 bit map to the background color at 0x0000?

Cheers.

felixjones commented 5 years ago

A lot of my instincts tell me this issue would have been avoided if the operation was described as OR'ing the bits rather than ADD'ing them as a reader could make the incorrect assumption that the carry bit of the ADD is what brings the values in range.

uint8_t pixel = ( ( tile_lsb & 0x01 ) << 0 ) |
                ( ( tile_msb & 0x01 ) << 1 );

An OR operation would describe the same thing, albeit without any implicit carries to muddy the interpretation of "how this works".

I'd consider removing the ADD entirely.

felixjones commented 5 years ago

The address mirroring code feels stinky too. Most of the time, mirroring can be accomplished with just one AND operation, that should be possible here too:

if ( 0 == ( addr & 0x0003 ) ) {
    addr = 0x0000; // Mirroring - any instance of colour 0 being used is the background colour
} else {
    addr &= 0x001f; // Not mirroring? Just mask the 5 bits we need
}

The ppuRead equivalent needs changing too, if I'm not mistaken. All this needs discussion and analysis, I could be missing something obvious.

kentwidman commented 5 years ago

These are much cleaner changes, and easy to understand. Nice!

brccabral commented 1 year ago

I tested this change, but it breaks SMB background