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

Rework "EGA mode with VGA palette" CRT shader auto-switching #3354

Merged
merged 5 commits into from Feb 3, 2024

Conversation

johnnovak
Copy link
Member

@johnnovak johnnovak commented Jan 27, 2024

Description

The auto-switching to VGA shaders for EGA modes with custom VGA palettes on emulated VGA adapters worked when launching games, but kinda by fluke. It was only reliable on text mode to EGA graphics mode transitions, but only by sheer luck... Things then started falling apart when using an image viewer such as QPV.

Definitely review commit by commit; the first commit is just about removing warnings and is very noisy.

It's a bit hard to explain succinctly why the implementation needs to be done in this particular way. I've spent about 5-6 hours just tracing through the code to figure out how the Rube Goldberg mode-switching machinery works... I understand it now, but I don't want to write 10 pages to explain which no one would bother to read anyway... 😝 Please read the comments and the commit messages if you want to understand it deeper what's really going on.

The high-level overview is that I tried to be a bit too clever previously when detecting non-EGA colours; now it just checks the new colour against all possible EGA colours whenever the program changes a VGA DAC palette entry.

I don't really expect anybody to go through my manual testing steps in detail, but it forces me to do a thorough job, and it will be useful for regression testing later if needed.

Could this all be done in a simpler and more straightforward way? Sure. After spending 5k+ hours refactoring the whole VGA code, everything VGA-related should be simpler. See you in 2050 😎 🀘🏻

Related issues

Original PR:

crt-auto should auto-switch to VGA shaders in EGA modes with 18-bit VGA palettes
#2819

Manual testing

General testing

Used the following specially prepared test pics in QPV: test-pics.zip

Grafx2 is great for such manipulations of paletted/bitplane-based graphics.

MONKEY1.IFF

320x200, 16-colour, EGA palette in canonical EGA colour order.
image

image

MONKEY2.IFF

320x200, 16-colour, EGA palette, but with bright cyan missing.
image

image

SQ3.IFF

320x200, 16-colour, EGA palette, but with brown missing and a completely different colour ordering.
image

image

MAGICWLD.IFF

320x200, 16-colour, non-EGA palette.
image

image

SCOOPEX.IFF

320x200, 16-colour, non-EGA palette.
image

image

Make sure QPV is configured for EGA modes. You should have something like this in QPV.CFG:

# 16-colour EGA/VGA modes
 320  200   16     40 $0d 0
 640  200   16     80 $0e 0
 640  350   16     80 $10 0
 640  480   16     80 $12 0

Turn off auto resolution (# key) and select the 320x200 16-colour mode (with the + and - keys).

Test 1

With the default config:

  • Press enter on MONKEY1.IFF β€” EGA shader gets picked.
  • Press Esc to return to the 640x480 interface screen.
  • Press enter on MONKEY2.IFF β€” EGA shader gets picked.
  • Press Esc.
  • Press enter on SQ3.IFF β€” EGA shader gets picked.
  • Press Esc.
  • Press enter on MAGICWLD.IFF β€” VGA shader gets picked.
  • Press Esc.
  • Press enter on SCOOPEX.IFF β€” VGA shader gets picked.

Test 2

With the default config:

  • Press enter on MONKEY1.IFF β€” EGA shader gets picked
  • Press enter to advance to MONKEY2.IFF β€” This involves no scree mode change, but as the image still contains EGA colours only, the EGA shader remains active.
  • Press enter to advance to SCOOPEX.IFF β€” This contains VGA colours, so the VGA shader gets selected.
  • Press enter to advance to SQ3.IFF β€” This has EGA colours, but no screen mode change is involved, so we're stuck with the VGA shader.
  • Press Esc and then Enter to display SQ3.IFF but trigger a screen mode change β€” EGA shader gets picked.

Test 3

Repeated the above tests with different glshader and machine settings:

  • glshader = none & sharp β€” Works as expected.
  • glshader = crt-auto-arcade & crt-auto-arcade-sharp β€” The single-scanned arcade shaders get always picked.
  • glshader = crt-auto-machine & machine = vga β€” Always the VGA shader gets picked.

Test 4

  • Use the default config, then start Deluxe Paint in EGA 640x350 16 colors mode (option "e").
  • EGA shader gets picked.
  • Open the palette editor with the P key, then start dragging the RGB sliders around.
  • DPaint snaps the colours to the 64-colour EGA palette, so no shader switching will take place.
  • Repeat the test in the VGA 640x350 16 colour mode (option "j"). This mode allows setting VGA colours for this EGA screen mode.
  • The EGA shader gets picked initially, but once you start dragging the RGB sliders, the shader auto-switches to VGA.
  • Repeat the steps in the 320x200 16 colour mode on EGA vs VGA.

Test 4

Tried a few games with the default config plus cga_color = colodore:

  • Dark Seed β€” VGA shader gets picked
  • Gods β€” VGA shader gets picked
  • Space Quest 3 β€” EGA shader gets picked & the custom colours are displayed correctly.
  • Spellcasting 201 β€” EGA shader gets picked & the custom colours are displayed correctly.

Test 5

Regression tested Total Eclipse in Hercules, CGA, CGA Mono, and Tandy modes with the appropriate machine settings.

"True EGA"

Confirmed that the below "true EGA" games pick the EGA shader with crt-auto on a VGA adapter:

  • Command Keen: Keen Dreams
  • Drakkhen
  • Space Quest II
  • Wasteland
  • Spellcasting 201 β€” Picks an EGA shader as the game uses the 640x350 16-colour EGA mode (out of a total palette of 64 possible colours).
  • Plus a good selection of the test cases from here

EGA modes with VGA palette

Confirmed that the below games that use EGA modes with custom VGA palettes pick the VGA shader with crt-auto:

  • Gods β€” 320x200 16-colour EGA mode but with a VGA palette. The game switches to an EGA mode at startup, then loads the VGA palette only a few seconds later (the shader switching happens then).
  • Dark Seed – The game uses 640x350 but with a custom 16-colour VGA palette. Before this PR, an EGA shader was picked incorrectly for this game (the difference is very subtle, but still not quite correct).
  • Plus a good selection of the test cases from here

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@johnnovak johnnovak self-assigned this Jan 27, 2024
@johnnovak johnnovak added bug Something isn't working enhancement New feature or enhancement of existing features video Graphics and video related issues shaders Issues related to shaders labels Jan 27, 2024
@johnnovak johnnovak force-pushed the jn/ega-on-vga-fixes branch 7 times, most recently from 4dcd621 to 44816f3 Compare January 27, 2024 12:34
@johnnovak johnnovak changed the title [WIP] Jn/ega on vga fixes [WIP] crt-auto EGA modes with VGA palettes auto-switching rework Jan 27, 2024
@johnnovak johnnovak force-pushed the jn/ega-on-vga-fixes branch 6 times, most recently from c1cd7b7 to ce139aa Compare January 28, 2024 02:37
@johnnovak johnnovak changed the title [WIP] crt-auto EGA modes with VGA palettes auto-switching rework [WIP] EGA modes with VGA palettes auto-switching rework Jan 28, 2024
@johnnovak johnnovak marked this pull request as ready for review January 28, 2024 04:47
@johnnovak johnnovak requested review from weirddan455, FeralChild64 and kcgen and removed request for weirddan455 and FeralChild64 January 28, 2024 04:47
@johnnovak johnnovak changed the title [WIP] EGA modes with VGA palettes auto-switching rework Rework "EGA mode with VGA palette" CRT shader auto-switching Jan 28, 2024
@johnnovak
Copy link
Member Author

johnnovak commented Jan 30, 2024

Ping @FeralChild64 @weirddan455 @kklobe, can any of you review this over the next week or so? I only need one person at least πŸ˜„ Don't be afraid of it, it's short and only reviewing it at the surface-level is better than nothing. I don't expect anyone to become DOSBox VGA code experts overnight (I'm not one either 😏)

The reasons I'm pinging you is because this needs to go into the final.

Copy link
Collaborator

@japsmits japsmits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two code modernisation hints. Approved anyhow, should you decide to ignore them.

src/hardware/vga_dac.cpp Outdated Show resolved Hide resolved
src/hardware/vga_dac.cpp Outdated Show resolved Hide resolved
When setting up an EGA video mode on an emulated VGA card via the 10H
BIOS interrupt, the following used to happen:

1. The 16 Palette Registers that store the indices into the first 64 Color
   Registers were set up first. The Color Registers store the actual
   18-bit RGB values the VGA card outputs when emulating EGA modes (in
   other words, the 16 Color Registers hold indexes into the 256-element
   VGA DAC table, and only the first 64 DAC entries can be addressed).

2. As the second step, the 16 Color Registers were loaded with the
   correct "canonical" EGA RGB colours.

The problem with this approach was that setting the Palette Registers
also triggers reloading the Color Registers. This triggering is probably
necessary and has something to do with murky details of how the actual
VGA hardware workd. But the Color Registers don't necessarily contain
the correct EGA colours when this triggering happens (they still have
whatever DAC colours were set last).

So this is what usually happened with the Color Registers when switching
to an EGA mode:

- 16 Color Register writes triggered by the Palette Register writes,
  often with the wrong non-EGA RGB values.

- Then another 16 Color Register writes when the VGA hardware sets up
  the emulated EGA colours in the DAC entries, now with the correct RGB
  values.

The problem with this approach was that it could often throw off the
"EGA modes with VGA colours" detection logic that listens to Color
Register writes after a mode change is initiated.

The obvious fix is to swap steps 1 and 2 to ensure the 2 x 16 Color
Register writes are all performed using the correct EGA RGB colours. The
exact same set of 16 colours are expected to be set after this swap.

This seems to be a rather safe change, and it's unlikely that it would
cause any side effects. Especially because all this setup stuff happens
in the scope of a BIOS interrupt, so nothing else should be able to muck
around with the VGA registers in the meantime.
All text modes get classified as GraphicsStandard::Vga on an emulated
VGA adapter, which can trip this assert under some circumstances.
@johnnovak johnnovak force-pushed the jn/ega-on-vga-fixes branch 2 times, most recently from c1e4d91 to 10e38c8 Compare February 2, 2024 11:25
src/hardware/vga_dac.cpp Outdated Show resolved Hide resolved
@kklobe
Copy link
Collaborator

kklobe commented Feb 2, 2024

OK, I ran through the manual testing steps to compare against main, nice work!

Copy link
Collaborator

@kklobe kklobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved even sans-const 😁

The previous method only worked by fluke in text mode to EGA mode
transitions.
@johnnovak
Copy link
Member Author

approved even sans-const 😁

Thanks @kklobe . Just force-pushed that minor change into the last commit.

@johnnovak johnnovak merged commit 1bba63c into main Feb 3, 2024
39 checks passed
@johnnovak johnnovak deleted the jn/ega-on-vga-fixes branch February 12, 2024 02:35
@johnnovak johnnovak added backport Important fixes that should be backported and removed backport Important fixes that should be backported labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or enhancement of existing features shaders Issues related to shaders video Graphics and video related issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants