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

Revert commit a1b4968 (proper fix needs deeper investigation) #552

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

carlosefr
Copy link
Contributor

@carlosefr carlosefr commented Nov 14, 2024

This restores the proper colors in Pinball Fantasies' table selection screen with vgaonly by reverting to the previous (buggy) DOSBox behavior. In other words, it won't be possible to opt out of double-scanning in 320x200 with vgaonly .

After spending some time trying to salvage the intention of commit a1b4968, I've concluded that the actual bug runs deeper and will require some more investigation. One other symptom is how this mode is misreported as 640x200 under SVGA, when it should be reported as 640x480.

Edit: This also restores the proper colors in the bottom control area in Lemmings.

@schellingb
Copy link
Owner

Any idea how other projects originally based on DOSBox got around this? Both DOSBox-X and DOSBox Staging do not seem to have any issues showing the table selection of Pinball Fantasy correctly even on the default SVGA machine. Though both these projects have large parts of the code that is almost unrecognizable from original DOSBox so it might not be easy to spot by looking at the source code of them.

@carlosefr
Copy link
Contributor Author

carlosefr commented Nov 16, 2024

I've looked at those, and their code is indeed different enough that it didn't help much.

Pinball Fantasies' table selection does 640x480, but sets the maximum scan line register to C0, which means it explicitly sets line doubling (SD=1). In vgaonly the SD bit in that register is ignored for vga.mode 2 and 3 — here — which is then compensated by the skip condition that this PR is reverting to.

Ignoring double scanning in the SVGA case just results in corrupted frames (the whole image is drawn on the top half of the buffer, while the bottom half is just garbage).

My suspicion is that Pinball Fantasies is depending on hardware behavior that DOSBox doesn't properly cover, hence these hacks, and now I'm not sure if that can be fixed without a significant intervention (given the references to several demos in the comments, I think the original DOSBox developers might've thought the same).

I haven't given up yet, but I'm doubly conviced that merging this PR is the best course of action for now.

@carlosefr
Copy link
Contributor Author

I haven't given up yet, but I'm doubly conviced that merging this PR is the best course of action for now.

Having said this, I've opened a separate issue for the SVGA case (#553), so that merging this PR doesn't bury that topic.

@carlosefr
Copy link
Contributor Author

carlosefr commented Nov 25, 2024

@schellingb Do you intend to merge this?

Currently it's not just Pinball Fantasies that's affected. There are a few other games and probably demos as well.

@schellingb
Copy link
Owner

Yeah this needs to be merged. Just to be sure, with commit a1b4968 reverted, commit 19876fe which was done afterwards will still remain. Are there any issues with that you could think of?

@carlosefr
Copy link
Contributor Author

carlosefr commented Nov 26, 2024

So far I haven't noticed any issues from 19876fe. Since that only applies to EGA or earlier, I don't expect the same kind of side effects.

What we're seeing with VGA results from (IMHO) DOSBox not implementing the register behavior accurately, instead working around misbehavior later in the process, and those workarounds becoming intertwined over time. There are too many possibilities with custom modes for those workarounds to cover all cases, and these bugs are inevitable.

I might take this as an excuse to go look for some EGA demos in the eXoDemoscene compilation, though. Just to make sure. 🙂

PS: When that if condition was removed in 19876fe, I should've looked for a similar condition elsewhere in the code. That would have revealed that it was half of a two-side workaround.

@schellingb
Copy link
Owner

Ok, thanks, I'll merge it now then.

Maybe we can look into what DOSBox Staging did, it looks like Staging has gotten rid of vgaonly as a machine altogether, having only the SVGA cards above EGA.

@schellingb schellingb merged commit fe6c41b into schellingb:main Nov 26, 2024
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.

2 participants