mamedev
saturn.cpp: device-ify VDP1/2, greatly rewrite QA notes at top, addresses for good #8915
Branch is just a courtesy to checkout CI and eventually not piss off anyone else (I definitely am if you ask me).
It’s worse doing weird class splits across files. If you’re not prepared to actually turn it into devices, please just leave it alone.
Also, it’s not acceptable to call things “code smells” if you can’t say why the code “smells” in your opinion.
Shut up.
You’ve had years to actually improve the situation with this driver, but you’ve chosen to spend your time on other things. You can’t just turn around and act like you’re “taking ownership” or something.
Weird class splits across files are an anti-pattern, or a “code smell” if you prefer. It makes the code harder to navigate. In this case you’re trying to give the illusion that there’s a level of encapsulation or separation of functionality that isn’t there.
If you’re not going to actually split it up into separate classes, that’s fine. Just don’t split class up across files arbitrarily.
Maybe you don't understand the context of my shut up. I don't accept a review from you, since you don't have the slightest idea about how Sega Saturn works out. It's not in your league, it's easy to bitch now about "haha stupid design" nowadays, but where were you back when this was first written? for example: where's your awesome LLE CD-Block device that will magically make everything to work 100% even on potato PCs? You are not even slightly serious from the first place, and this whole stunt about blocking the XMas MAME release out of something that was there since forever is a clear sign you're losing it out.
As for the code smell: it's a double nested if conditional branch that complements each out. And yes, shadow effects are known to glitch out in some places, imagine my surprise.
I don't accept a review from you, since you don't have the slightest idea about how Sega Saturn works out. It's not in your league, it's easy to bitch now about "haha stupid design" nowadays, but where were you back when this was first written?
That’s completely irrelevant. Those files were moved from src/mame to src/emu in 2013 (1a0c7eceeaa5838eff7ad6c1956c7ffb0d60a447) as preparatory work for a future change to allow Saturn to be moved to the MESS target, and presumably to decouple the state classes completely. This was after conversion to C++ and after device_t had been introduced, but before src/devices was a thing. In eight years, the decoupling didn’t happen, and these files were never made independent of the saturn_state driver state class. Eight years is long enough to wait.
You are not even slightly serious from the first place, and this whole stunt about blocking the XMas MAME release out of something that was there since forever is a clear sign you're losing it out.
No, it’s a sign that I’m losing my patience at constantly being the one to clean up messes I didn’t make. Things aren’t going to improve if people don’t improve them. Having this class split across libraries was embarrassingly bad, and no-one’s stepped up to fix it in years.
People tell me to delegate and not work myself into the ground, but for the most part people just want to leave things to rot.
As for the code smell: it's a double nested if conditional branch that complements each out.
So put a comment that actually says that. Simply writing “code smell” is completely unhelpful to the next person reading the code. Comments are for humans to read – if the person reading the code could telepathically determine why you think the code “smells”, they wouldn’t need the comment.
No, but it needs patience to sort everything out. "8 years" remark is laughable, have you instead read my remarks at top of VDP1/2? I don't blame anyone not picking this up doing this simple refactoring I'm doing and leaving in current state, since doing this actually right (and more interesting as a user standpoint) is something that is tricky as hell even if MAME would be an actual professional company instead.
And, since you don't pay me a single dime let's put this clear:
- I don't care if I'll break anything, can't be bothered in QA testing for the time being. It's also incomplete anyway, 'J' letter iirc;
- I don't care about refactoring except for stuff I catch up and are quick to fix;
No, but it needs patience to sort everything out. "8 years" remark is laughable, have you instead read my remarks at top of VDP1/2? I don't blame anyone not picking this up doing this simple refactoring I'm doing and leaving in current state, since doing this actually right (and more interesting as a user standpoint) is something that is tricky as hell even if MAME would be an actual professional company instead.
I’m not complaining about no-one improving the emulation of the VDPs in eight years. I realise that’s difficult. I also realise there’s relatively little interest given there are other Saturn emulators (Mednafen and derivatives, Yabause and derivatives, and so on).
I’m only objecting to the half-arsed change from eight years ago never being completed or rolled back. It’s great if the VDPs can be devicified easily, but failing that, putting the stuff into src/mame/video/saturn.cpp is a big improvement over having the class split across libraries and portions of the source tree.
I don't care if I'll break anything, can't be bothered in QA testing for the time being. It's also incomplete anyway, 'J' letter iirc;
I’m not asking you to re-test anything or expecting you to do so. It’s just the embarrassingly bad class split that I’d finally had enough of.
I don't care about refactoring except for stuff I catch up and are quick to fix;
Once again, I was never asking for the thing to be refactored to some ideal state, just for the stupid class split across libraries/circular dependency to be fixed, i.e. to be more honest about the lack of separation/encapsulation and not have parts of a class hidden off in a remote part of the source tree.
Once again, I was never asking for the thing to be refactored to some ideal state, just for the stupid class split across libraries/circular dependency to be fixed, i.e. to be more honest about the lack of separation/encapsulation and not have parts of a class hidden off in a remote part of the source tree.
Which means the User Story is done, once that the CI completes you or anyone really can pull the trigger and eventually pickup from there.
Which means the User Story is done, once that the CI completes you or anyone really can pull the trigger and eventually pickup from there.
Not really given you’ve done things we wouldn’t accept from an external contributor, and team members have a responsibility to set a good example. For example:
- Global namespace pollution with macros in a header. Given most of them are used to access members, even if they need to be macros, they shouldn’t be in the header.
- Creating devices with parent references in the path (the VDP2’s
m_paletteandm_gfxdecode). This is something thatmachine_configshould reject outright.