Skip to content
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

video/mc6845.cpp: Restore support for zero active width/height configuration #12007

Merged
merged 2 commits into from Mar 22, 2024

Conversation

mgarlanger
Copy link
Contributor

@mgarlanger mgarlanger commented Feb 4, 2024

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).

@mgarlanger mgarlanger marked this pull request as ready for review February 4, 2024 05:25
@mgarlanger
Copy link
Contributor Author

This should address the crashes seen in #11756, @Tafoid @Robbbert if you guys have time, could you test this build to verify no crashes are seen on those systems.

@rb6502 rb6502 changed the title Revert "Revert "video/mc6845.cpp: Support zero active width/height configuration video/mc6845.cpp: Restore support for zero active width/height configuration Mar 22, 2024
@rb6502 rb6502 merged commit cb56249 into mamedev:master Mar 22, 2024
5 checks passed
@cuavas
Copy link
Member

cuavas commented Mar 22, 2024

This breaks everything that sets "show borders".

@rb6502
Copy link
Contributor

rb6502 commented Mar 22, 2024

It fixes 2 dozen-ish drivers that just crash.

@cuavas
Copy link
Member

cuavas commented Mar 22, 2024

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.

@rb6502
Copy link
Contributor

rb6502 commented Mar 22, 2024

Then you should've noted that on this PR and closed it.

@cuavas
Copy link
Member

cuavas commented Mar 22, 2024

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: 5e1b719

Since the issues weren’t fixed two months later, it was reverted before 0.262: 064d676

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.

@rb6502
Copy link
Contributor

rb6502 commented Mar 22, 2024

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.

@angelosa
Copy link
Member

angelosa commented Mar 22, 2024

fwiw I commented on those two setters later on.

// HACK: show the full screen_device htotal/vtotal if true (regulate with UI slider controls)
void set_show_border_area(bool show) { m_show_border_area = show; }
// HACK: a static alternative of above, potentially unsafe.
void set_visarea_adjust(int min_x, int max_x, int min_y, int max_y)

@mgarlanger
Copy link
Contributor Author

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 - #11504 that I haven't been able to make any progress on, I'll just close that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants