MiSTer-devel / NeoGeo_MiSTer

NeoGeo for MiSTer
GNU General Public License v2.0
148 stars 78 forks source link

Arcade core with MRA support. H/V-sync controls. Convenient pause. #160

Closed blackwine closed 2 years ago

blackwine commented 2 years ago

Adds:

sorgelig commented 2 years ago

H/V sync adjustment for console already discussed many times - NO. It's arcade cores option because they are not for TV originally. Consoles should be made with timings like originals. MRA support is redundant for this core. It already has support for full game library and i see no reason why it should be changed.

blackwine commented 2 years ago

This doesn't break console build by any means. It just provides independent core build with support for an alternative way to load data. Which provides some additional benefits, e.g. easy way to create rom patches in MRA files. For example this enabled me to quickly provide patches for Xeno Crisis' issues with sound on mister. To elaborate, this doesn't change behaviour of a current core when built from original NeoGeo.sv file. It is just complementary core to the old one. Many people endorsed this effort.

sorgelig commented 2 years ago

Keyboard handling is nonsense. MiSTer already supports JPAC, JammaSD and basically any keyboard assigned as dual-controller. Even arcade cores get rid of this ugly hack, while you introduce it here.

atrac17 commented 2 years ago

This statement isn't true at all... Even arcade cores get rid of this ugly hack, while you introduce it here.

sorgelig commented 2 years ago

You tell to the one who removed this hack from arcades? MiSTer already supports it on system level

sorgelig commented 2 years ago

As for patches - better to implement it in already working way of ROM loading than bring a whole new way to load.

sorgelig commented 2 years ago

https://github.com/MiSTer-devel/Main_MiSTer/blob/d8edab069e4108180b03255d08b9d7b7d5bb4e22/input.cpp#L3358

sorgelig commented 2 years ago

@blackwine i understand your possible frustration. You probably spent quite some time to create all those MRA. But could you propose the changes BEFORE you start to make all those MRA? I think yes. I simply cannot accept any random and especially big change as it will be a part of system and will require further support. Having 2 ways of load with completely different files is redundant and brings no new feature. It will be just another subfolder in myriads of arcades which has no different when you click on NeoGeo core and go to ROM loading. About the same amount of button presses. MRA is part of arcades because it's otherwise impossible to know how to load arcades. NeoGeo already has loading system. It's well established and semi-automated so you can see that most ROMs have just one line in xml. It's possible to convert all cores and use MRA, but it's wrong way. MiSTer has OSD with many features. It has recent cores and recent ROMs in cores. It's unified across all cores.

As i've told above, you are welcome to implement patches to existing system. I think both MVS and AES will benefit from it.

thehughhefner commented 2 years ago

I really hope Sorg can reconsider this. Neo Geo is first and foremost an arcade system, and I would like to know, respectfully, why it was implemented as a Console core for the mister? .neo files are a pain to maintain and darksoft romset is not trivial, so I think mra is the way to go.

sorgelig commented 2 years ago

darksoft romset is not trivial

and MRA uses exactly the same romset. If you will look MRA it's much more complicated to maintain and more cryptic while 99% of games in romsets.xml are one-liners. So it's exactly opposite what you've said.

birdybro commented 2 years ago

Some of the features in this could be split up into more easily agreeable separate pull requests and be excellent additions to the core.

sorgelig commented 2 years ago

such as?

birdybro commented 2 years ago

The added DIP switches in neo_f0.sv and the top level. I wrongly assumed this included decryption of MAME roms in the MRA's, but it doesn't, so that was my other feature thought.

However, I personally think that the HV adjustments ought to be in this core at some point, but I completely understand your perspective that this is a console core as it is on the MiSTer, and that HV adjustments are costly and difficult and add complexity for a niche use.

At a minimum this should have been 3 separate pull requests. 1 of which would have probably gotten denied for good reason (MRA), another which would have been something I think deserves consideration (HV adjust), but is fair to deny.

Interesting side comment... Neo Geo in practice as a system is more like CPS1/CPS2 than it is like the SNES. But as a MiSTer core it's more like SNES than it is CPS1/CPS2 (because it was made before the MRA standard). I think the average user doesn't understand the distinction. Neo Geo isn't a console, but the MiSTer Neo Geo core is a console core is what you mean, right?

thehughhefner commented 2 years ago

The added DIP switches in neo_f0.sv and the top level. I wrongly assumed this included decryption of MAME roms in the MRA's, but it doesn't, so that was my other feature thought.

However, I personally think that the HV adjustments ought to be in this core at some point, but I completely understand your perspective that this is a console core as it is on the MiSTer, and that HV adjustments are costly and difficult and add complexity for a niche use.

At a minimum this should have been 3 separate pull requests. 1 of which would have probably gotten denied for good reason (MRA), another which would have been something I think deserves consideration (HV adjust), but is fair to deny.

Interesting side comment... Neo Geo in practice as a system is more like CPS1/CPS2 than it is like the SNES. But as a MiSTer core it's more like SNES than it is CPS1/CPS2 (because it was made before the MRA standard). I think the average user doesn't understand the distinction. Neo Geo isn't a console, but the MiSTer Neo Geo core is a console core is what you mean, right?

Then CPS1,5/CPS2 should be added to the console section as well.

birdybro commented 2 years ago

Then CPS1,5/CPS2 should be added to the console section as well.

You didn't understand my point. Reread this:

Neo Geo in practice as a system is more like CPS1/CPS2 than it is like the SNES. But as a MiSTer core it's more like SNES than it is CPS1/CPS2 (because it was made before the MRA standard).

When the NeoGeo core was made, you had to use annoying scripts and carefully place files in the right place to create an RBF of the arcade game for arcade cores. This means, at the time, people would have to generate 158 rbf's from their rom collection, making the distinction between the current .neo situation and the potential MRA (but still .neo) situation moot.

sorgelig commented 2 years ago

.neo? It's supplemental format, not main one. Main format is zip files (from darksoft) and romsets.xml. No need to generate anything else. Everything is ready to use.

birdybro commented 2 years ago

In practice .neo is the only format that works 100% of the time. The MAME format only works if the ROM isn't encrypted. Darksoft works because it is decrypted as a format, but the user has to make sure their romset.xml is updated. People have had problems with the regular darksoft set as well (probably user error from no romset.xml usually) and have to use .neo for some roms because the .neo sets are always decrypted and have the appropriate mapping in the headers. Darksoft/MAME needs the XML, which is a detriment because someone has to keep updating the XML, but it is done regularly so that is fine.

Sorry if my comment was confusing. My main point regarding .neo was is that .neo is a full format that "just works" without external factors needing to be in sync, and it's better as a result for both the user and the developer. Darksoft support is a bonus, and MAME support is only fully possible if decryption were added to the core, which is probably very difficult.

thehughhefner commented 2 years ago

It's good to have the confirmation by Sorg that the main format is zip files from darksoft and romsets.xml. As birdybro mentioned, you need to update romset.xml. Then we have another reason to have mra support.

birdybro commented 2 years ago

But then you would have to update the MRA set which is 158+ separate files, which is more complicated and more work right?

sorgelig commented 2 years ago

I have zero problem with darksoft set. Everything just works.

As birdybro mentioned, you need to update romset.xml. Then we have another reason to have mra support.

Update 158 (actually there are more roms IIRC) files with cryptic format instead of one simple xml file? I don't see logic here.

romsets.xml basically haven't changed several years. There were some recent changes related to display names and other cosmetic data.

birdybro commented 2 years ago

Sorry to derail. The original point of the comment was advice for @blackwine to split the PR into separate ones and to debate the merits of each and to see your points you raised too.

sorgelig commented 2 years ago

I've tried to check what can be taken from this PR.. The only feature i see is pause when OSD is opened. Actually using unibios, you can press select+start to open menu and effectively pause the game.

atrac17 commented 2 years ago

Part of this was working towards adding encryption in the next steps. Not use decrypted ROMs. Actually use physical ROMs dumps. Being able to patch on the fly is great.

NeoGeo first and foremost will always be in arcade platform. The console only has an oscillator and edge connector change from MVS hardware.

It's a vertical wooden crate with a monitor, push buttons, joysticks, and a PCB. That PCB takes "ROM carts". All but two games were designed to run at an MVS refresh rate. Google some SNK docs.

Anyways, we get it... months of work and you don't want it even though it does not break anything that occurs with this repo currently.

sorgelig commented 2 years ago

Why existing loading shouldn't be compatible with encrypted ROMs? Add decryptor and it will be. I don't understand why need to spend a lot of time to re-invent the wheel? NeoGeo is already working fine. Going to subfolder with NeoGeo MRAs to start the game, or launch the core and then start game inside - about the same amount of actions. It's just prefer blue color or red. There is no added value. You spent months for making what is already there, instead of spending time to add decryption.. Just mod for mod. MRA is more complicated and obfuscated comparing to simple way through existing romsets.xml Tomorrow another one will bring another loader. Should it be added too?

atrac17 commented 2 years ago

Come on Sorg, no one else is going to bring you another loader. "Tomorrow another one will bring another loader" is the most asinine comment to date. I'll leave it be, but it's not "more complicated" just a different approach. If I can do it, then it's not complicated.

We'll make sure to check with you before doing anything with a core in MiSTer-devel since it needs your approval. You've made the point you don't see the value in it. This isn't about just playing games it's about fully fleshing out the system for preservation. Which is what this project is sold on. Maybe not by you, but everyone that pushes it on the internet.

sorgelig commented 2 years ago

Don't make from me a monster, ok? Every big project has its view how it develops. Try to add a random big change to Linux kernel (not, MiSTer, but original Linux) github - you will get a hard time. I'm trying to be as flexible as possible but there is common sense. Currently you see only small file romsets.xml which is the only thing you need to load any NeoGeo ROM. You offer tons of MRA files to do the same thing. The choice is obvious. SNES, Genesis -they also basically can be reworked to start a separate rom from MRA. So do it as well just because you can?

If I can do it, then it's not complicated.

it's not about what you can do. If it can be done by more simple way then that way is better. Treat original way as "just plug the cart" while your way is "here we have many chips, let's describe how to connect them then play."

Why not spend the time to fix some problems in core if you have so much free time?

sorgelig commented 2 years ago

Lemme guess how you wanted to implement decryption: apply a an inverted xor mask for the whole rom files. Am i right?

atrac17 commented 2 years ago

I'm not making you a monster sorg, I support you. I have even supported you monetarily and still do just not under this handle. I'm not trying to belittle you, don't do it to me.

I'm regurgitating what's been said here. I understand there is a language barrier. But it's hard to twist what's been said. I'm not giving you grief.

Why would I waste my time making cores better when I have to clear what I am going to do first? Isn't this open source? Yet you'll pull PR's that break things. @birdybro is even saying the same thing. We don't even get along. He contributes to your project left and right. Wouldn't him bringing this up warrant listening to what he has to say as well?

No, I am not going to use an xor mask. I'm going to spend a shit ton of money to have the encryption done properly and reverse engineered. Not something out of mame, not a DS trick, not some terraonion shit. Yes, I am fully aware that everything functions the same either way. It's not always about playing games for some people.

All of this so 400 years from now some clown can play Garou Densetsu...

birdybro commented 2 years ago

Just to clarify, the main thing I'm saying is that MRA loading might have benefits, I think there are good arguments for and against it. I personally am in the middle on the MRA loading, I can see the point of view that it fits with the user experience of Arcade users with cabinets, and I can see the perspective of Sorgelig on how it adds a lot of complexity and it would add more management in the long term.

I'm also saying that HV adjustment should be seriously considered since the games for this system were not typically made for consumer CRTs and weren't designed for the overscan like other console cores, so users cannot see the health bars and other information, and the centering can be off sometimes.

I don't think this pull request is the way to get each of these things done. Compartmentalizing changes so they are easier to test, easier to digest, easier to trace in the git blame, will make a pull request more palatable probably.

The DIP switches personally are something that should be added regardless, they are simple and do the job.

sorgelig commented 2 years ago

I'm going to implement similar to MRA but generalized file which will in theory load roms in any core. It won't require MRA complexity and will reuse current loading methods in cores. It won't require special core version.

pplatoon commented 2 years ago

I'm going to implement similar to MRA but generalized file which will in theory load roms in any core. It won't require MRA complexity and will reuse current loading methods in cores. It won't require special core version.

that file will be able to make a call to an image created for the rom without showing the generic core image? it would be nice to do that for the little i2c. I know that the tty has no problems. Thank you

sorgelig commented 2 years ago

should be

sorgelig commented 2 years ago

github again glitching.. couldn't attach files here. I think discord will have test version soon.