mame
mame copied to clipboard
mamedev
Revert "Revert "video/mc6845.cpp: Support zero active width/height configuration
Verified with the build artifacts on this PR that the DE check prevents the crash in Windows (tested with a load prior to the original revert and saw the crash to verify my setup).
This should address the crashes seen in https://github.com/mamedev/mame/pull/11756, @Tafoid @Robbbert if you guys have time, could you test this build to verify no crashes are seen on those systems.
No, those drivers don't crash because I backed out the problematic change before it made it into a release. This re-applies the problematic change and then attempts to hack around it.
The problematic change was merged at the beginning of the 0.262 development cycle (end of novermber or early December), to allow plenty of time for testing and fixing issues that surfaced: 5e1b719acada7c7003514aadab372d7bbedf9583
Since the issues weren’t fixed two months later, it was reverted before 0.262: 064d676e94ecc60f1b73ca59d19e8359b86e1d99
There was no release including the problematic change, and breaking systems that draw to the border area right before release isn’t a good look.
Then you should've noted that on this PR and closed it.
I thought it was obvious from the content of the second commit what the effect of the PR would be.
It wasn't obvious, and PRs with known problems should have those problems noted on them so that they then are obvious.
That also allows us to auto-close abandoned PRs after some amount of time so we don't have 120-odd open that will probably never be applied.
fwiw I commented on those two setters later on. https://github.com/mamedev/mame/blob/1af40e3a1c828e8394d51b8f3b611d39a2b4bb5e/src/devices/video/mc6845.h#L65-L68
Better communication would definitely be appreciated. Since there was no comments for 7 weeks and a few other PRs I opened were reviewed, I assumed there was an issue, but an actual comment would have helped.
Due to the potentially unsafe methods pointed out by angelosa, it seems testing of all systems using 6845 would be needed to verify this change (or similar change). Since this change was only needed to enable the screen saver(blank) effect on a replacement ROM for the H19, it doesn't seem worth it to devote more time to it. I have an alternative idea that should have less of an impact on other systems, that I may try in the future, but for now blanking the screen will just not work.
It would be nice if someone more experienced took the time to replace/rework the 6845 to get the 2 interlace modes working, and make it safer for future changes. I have another PR for some interlace functionality - https://github.com/mamedev/mame/pull/11504 that I haven't been able to make any progress on, I'll just close that one.