Add quirk for Ys (Japan) to fix graphics bug

[birdybro]

Resolves https://github.com/MiSTer-devel/SMS_MiSTer/issues/60

[birdybro]

The unique Product Code can be detected in the region I specified. Further quirks if necessary can follow this same format. For a list of batch headers ran on roms (many years ago) to check against in the future:

headerreaderbatchresults.zip

Here's the roms from that old list with the duplicate hex values

SMS duplicate product codes.csv

Most are just bad dumps or weird bootleg releases when they overlap.

More info:

https://www.smspower.org/Development/ROMHeader#ProductCode7ffc25Bytes

loosely related to this header detection, I've been looking into the game gear EEPROM, but am struggling on the mapping/banking changes needed, still reading the core. Here's the product codes for those 4 games for reference later:

https://github.com/birdybro/SMS_MiSTer/blob/eeprom/SMS.sv#L516-L519

[asturur]

hi @birdybro i have a question on this pr. The comment in the code says: -- Enable VDP version 1 for Ys (Japan). Is this VDP version one differing only for that change? and is this change bad for other games? I do not understand why this fix is welcome for Ys but enabled only for Ys

[sorgelig]

Enable it for specific game because this game has problem. General rule is not to break what is working fine. Testing all games against this change will require a lot of time. Some games may not like this change.

[asturur]

Ok so we don't how it behaves with other games, and we don't know if it is correct from an HW stand point. From a don't break what is working point of view, comments are important. It would be nice to have that in the comment. Instead of simply saying enable VDP version 1, we could say enable an extra behaviour on the vdp to mimic how YS behaves with original vdp version on the SMS. This behaviour hasn't been tested with other games.

Also i strongly believe that if we don't give an option ot enable it in the OSD, no one will ever test it with different games. 🤷‍♂️ In the scope of the mister project fixing a game is nice but should be belove finding the correct hardware pattern that works for all games, even if is not a priority being able to switch one that behaviour in order to test more roms should be as important as fixing the game.

[birdybro]

I think you are misunderstanding the issue @asturur. I made a quirk and have detected the game with the product ID in the header of the SMS ROM. This will only be enabled when that game is detected. I found no other game with an identical product ID when referencing a list that was made via batch scanning, so this change in behavior will not influence any other game.

According to the comments in the code that was present before, this is something that happens on real hardware in later revisions of the SMS because the VDP was slightly changed later on. I don't see any reason to treat this as an inherent bug in the core's design. With my change it basically swaps to the behavior of the oldest VDP revision upon detecting this game. You can see all sorts of quirks similar to this with the Sega Genesis core using a similar detection method if you are looking for precedent.

If you'd like to test it with other games I can compile a version of the core that has it force enabled just for you and you can test it against the whole library and report back for me.

[birdybro]

Here you go @asturur

SMS.zip

Let me know how the testing goes when you are done. Make sure to compare before and after when you notice what could be considered a bug. The error would be graphical corruption pretty much. Switching the option in the OSD to on enables the Old VDP (what this quirk does for Ys (Japan)), so test the games with that. Anything that changes behavior from Old VDP being turned off in a buggy kind of way, please put it in a spreadsheet so I can evaluate it.

Thank you! :)

EDIT: If this build doesn't work let me know, I didn't test it to make sure the toggle works or not.

[asturur]

no wait, i think you misread my comment :)

This will only be enabled when that game is detected

This was extremely clear, and that is what i m commenting about.

• if that fix is the correct implementation for an original version 1 sms, that fix should be always on.
• if that fix isn't enabled for all games there is a reason, and @sorgelig said that is because is untested.
• if we are not 100% sure that the fix is good for all cases, we should have an option to toggle it on/off independently from the ys jap cart, so someone of good will can test it. Then there may be no one that wants to do it. but that is another topic.

Why wouldn't you make the toggle available for everyone?

[birdybro]

if that fix is the correct implementation for an original version 1 sms, that fix should be always on.

I disagree that it should be on because if we put it always on then we could introduce new bugs unnecessarily. The first version of the SMS wasn't released in the US or Europe, only Japan. There was a revision shortly afterwards even before the SMS was released in US and Europe. I don't understand the purpose of the toggle without good reason to add the toggle, it may very well do nothing, or it may very well cause a bunch of bugs and increase the number of false positives submitted as issues.

if that fix isn't enabled for all games there is a reason, and @sorgelig said that is because is untested.

Precisely. Hence why I've given you the means to help test and contribute to the project if you wish. If there is value to the option being turned on for more than 1 or even 2 games, then we should have a toggle, I have no problem with that :)

if we are not 100% sure that the fix is good for all cases, we should have an option to toggle it on/off independently from the ys jap cart, so someone of good will can test it. Then there may be no one that wants to do it. but that is another topic.

The test build I provided is freely available for anyone who wants to test and report back. Adding debug options to cores blindly without demonstrated value behind them is not idiomatic with the MiSTer project typically. Maybe sorg would have a different opinion on this, but I typically try to remember not to introduce OSD options without a proven value on cores that are not mine.

I'll share the build with our discord as well, there are a few people there that might want to test it out. Maybe it would help with the sg-1000 games that have graphical corruption as well on this core (but don't have this corruption on the colecovision core) for instance.

Why wouldn't you make the toggle available for everyone?

Because typically adding toggles requires significant justification. If this fix is only affecting one game out of the entire library, it wouldn't be justified personally (and in fact would make the core less user friendly if someone had to manually turn it on to fix the problem with that game).

[birdybro]

@asturur I conscripted my favorite brave soldier of testing moondandy to help me out with testing out various games with that test build to see if there is any justification for this sort of toggle. ;)

There is a possibility that it would resolve the graphics corruption of various SG-1000 games in this core.

[asturur]

Want to be clear, mine was just a question to understand. In the megadrive core we can switch between 2 different audio implementations, 2 distinct chips, with barely any difference for my ears. And the toggle is there. This seems a similar, a VDP1 and a later release o the VDP. So it seems to me that is a very similar case.

And if we are running out of flags because of how flags are done technically well that can be a reason why you don't want the flag.

I still think it would be nice to have a more explicative comment in the code of why the patch is there, but eventually this comments on the pr are now attacched to the code change anyway so the issue is mitigated.

[birdybro]

We aren't running out of flags. There are 128 bits to work with now with the latest framework. It's not about the number of status bits available in the OSD, it's about not adding toggles when there is no sufficient justification. If sufficient justification is demonstrated, then it would be fine to add a toggle. :)

Just my personal thoughts.

[sorgelig]

In genesis there are many quirks per games. It doesn't mean all these cryptic options should present in OSD. It goes a general rule: if game doesn't work or misbehaves, report it here. If some one has solution then it's integrated. There is no point to discuss about which flag should be added to clutter the options. Core is opensource, so the one who is curious, can add customization for himself and experiment.

As for different audio chips - it's a whole different story.

[asturur]

@birdybro at the end explained clearly what i wanted to understand, this added 'there is no point in asking' is so unwelcoming.

you can also lock the pr right after they are merged if the questions on pr is something you don't want