-
Notifications
You must be signed in to change notification settings - Fork 2k
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
vsnes: working sound + "improved" graphics in Vs. SMB bootleg sets #7360
Conversation
I wonder if the 'corrupt instructions' on set 3 are some kind of protection, if it's very specific corruption only on those opcodes it's fairly unlikely to be a dump problem. |
It's not just those opcodes; the set 3 version seems to be basically the same as the set 2 version but with a bunch of bits sporadically getting "lost", including in normally unused/empty regions at the beginning/end of the ROM. The instructions in the NMI/IRQ handlers just stood out to me since they're close to the beginning of the ROM and they make the audio totally non-functional in that set. Looking closer, the PPU/CHR ROMs in set 3 seem to have the same issue, but with less severity (i.e. a handful of random bytes with one bit flipped or so compared to the other sets). |
ah in that case, if set 3 just looks like a corrupt version of the other sets, it should probably just be removed It was apparently added in 0.215 "Vs. Super Mario Bros. (bootleg with Z80, set 3) [jordigahan, Clawgrip]" @clawgrip was this double checked? did it read consistently? any sign of issues with the ROMs? sounds for sure like they're corrupt and probably shouldn't have even read the same on any verification pass |
No, it was dumped by jordigahan, who often has reliability problems. If you suspect it's bad, then it probably is. Thanks a lot for checking. |
Should I just remove that set from the driver then? (Also, should the undumped EPLD from set 3 be added to sets 1 and 2, and maybe also add the PALs to set 2? They're all used for PPU stuff and timing generation according to the schematic so I suppose they're probably present on both) |
yeah both of those actions sounds sensible to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How robust is the OAM buffer flipping workaround? Could it cause desyncs if a system is found that has banked PPU tile storage or tile RAM? At first glance, it looks like one of those “easy to implement now, hard to untangle later” implementation decisions.
m_bank_vrom[0] = membank("bank2"); | ||
m_bank_vrom[1] = membank("bank3"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you’re going to be keeping these in members, why not use object finders rather than searching for them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I attempted to do at first, but it seems like the preceding calls to install_read_bank
cause the banks to only be created after memory bank finders would have all already been resolved. Maybe there's a different way to go about it though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it’s installing stuff in initialisation functions rather than using configured memory maps. That would mean the memory banks don’t exist when object finders are resolved.
Since this system isn’t cartridge-based, it could be converted to use configured memory maps (we want to get away from installing handlers in initialisation functions when it isn’t actually necessary), but that would be a pile more work to do. It’s probably a bit much unrelated work to worry about for this pull request.
I think the worst case there is that sprites might end up getting drawn with the wrong tiles (but correct position/attributes) for one frame, since the OAM and the actual tile memory are separate from one another both on the bootlegs and the real NES PPU. |
Can you at least add a comment that the read timings will be incorrect and it can potentially read the wrong tile graphics due to the nature of the workaround? |
The color change is due to these sets now using the actual bootleg color PROMs instead of falling back on the real 2C04 palette like they were doing before. If you check the video I linked in the original post (at around the 3:45 mark) you can see the colors as they appear on the actual hardware. |
All good then. They just got flagged in my testing so I wanted to make sure. I hadn't followed progress on this bug report so I didn't notice the initially posted video and indeed it looks close enough to me. Thanks. |
Based on kevtris' schematics and video, these commits:
ppu2c04_clone_device
which implements the bootlegs' register behavior, palettes, video timing, and other differencessuprmriobl3
set as a bad dump due to corrupted return instructions in both interrupt handlersI removed
MACHINE_NOT_WORKING
from sets 1 and 2 but left it on set 3 due to the bad Z80 ROM.What I'm not sure about at the moment: