Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kentwidman
Copy link

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.

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
Copy link

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
Copy link

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
Copy link
Author

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

@brccabral
Copy link

I tested this change, but it breaks SMB background

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants