Enable all Framebuffer NTSC / PAL screen modes

[hornc]

This PR:

• Fixes NTSC 256x240 mode, which previously displayed as 205x240 (in Mednafen emulator at least)
• Enables NTSC 512x240 and NTSC 640x240 modes, which previous paniced on InvalidY from an over-PackedVector
• Autoselects appropriate interlace mode based on Framebuffer height for both PAL and NTSC, previously 480 and 512 heights displayed at the non-interlaced resolutions (240 and 256)
• Enables all PAL mode Framebuffers, by adding them as valid Vres, and fixing an off-by-one error on lower_right corners which does not cause errors in NTSC modes, but does for PAL modes in the 512 height VRAM
• Changes the horizontal width 368 to 384. This is a strange mode, it seems theoretical? I was looking at Yaroze documentation which consistently claims 384 is the correct value, happy to revert this if there is a reason why 368 is correct?

Unfortunately I still don't quite understand the range of values that can be passed to
GP1(06h) - Horizontal Display range (on Screen), or what the units are.

I think the exact values are (mostly?) independent of the desired display resolution and have more to do with the analog video output signal and the range of analog display devices (CRTs), and depend on VSYNC and HSYNC timings of PAL/NTSC signals and so on... The documentation I have seen isn't very clear, and all explanations I have found make sense for NTSC 320x240, but it's difficult to apply to other modes. I could also be missing something.

Supporting my theory that the value is independent: https://github.com/Lameguy64/PSn00bSDK/blob/702bb601fb5712e2ae962a34b89204c646fe98f5/libpsn00b/psxgpu/env.c#L149 SetDefDispEnv appears to set all the screen values to 0 regardless of the supplied x, y, w, h, and that causes identical values to what psx-sdk-rs previously calculated for 320x240 to be sent to GP1(06h) -- so the values are effectively hard coded. I have not disassembled PsyQ to figure out what it uses.

The current psx-sdk-rs calculated values for widths 512 and 640 overflow the PackedVertex, and panic with InvalidY, so the current formula is not correct for those resolutions. The calculated value for 256 x 240 also truncated the displayed screen to 205x240, so the formula also seems incorrect for lower widths too. This PR's hardcoded value gives expected results for all resolutions (... except 384, but maybe that is expected, or another PSX issue?)

I have currently only tested this in one emulator (Mednafen); testing non-interlaced and interlaced double buffering modes for NTSC and PAL with text display and PolyF4 drawing.

I should be able to test the PAL modes on a real PSX (hopefully GP1(06h) won't damage my stuff!)

Sorry for the description verbosity, there are only a few lines of code changed here, but testing all ~20 mode combinations has taken some time to research and write test examples for. I hope the problems I'm fixing are clear enough!

If anyone has any comments or tips for testing, please comment below!

tagging @Kapouett because you said you were interested in PAL modes (a while ago now!)
@ayrtonm, I'd love to get your feedback on this, you've probably read all the same slightly unclear docs I did.

Testing:

non-interlaced modes, tested in emulator (Mednafen)
Vertically stacked disp/draw: Framebuffer::new((0, 0), (0, y), (x, y), VideoMode::NTSC)

<style type="text/css"></style>

mode (NTSC)	BEFORE	AFTER
NTSC 256 x 240	Truncated width, 205 pixels wide, 240 high	WORKING (256 x 240)
NTSC 320 x 240	WORKING	WORKING
NTSC 512 x 240	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via horizontal_range()	WORKING
NTSC 640 x 240	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via horizontal_range()	WORKING
NTSC 368 x 240	WORKING (368 wdith)	InvalidX @ psx/hw/gpu/gp1 display_mode()
NTSC 384 x 240	InvalidX @ psx/hw/gpu/gp1 display_mode()	Truncated width = 364 pixels displayed on screen, (disp and draw are 384)
mode (PAL)	BEFORE	AFTER
PAL 256 x 256	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via psx/gpu/mod lower_right: PackedVertex<3, 10, 9>	WORKING, 256x256
PAL 320 x 256	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via psx/gpu/mod lower_right: PackedVertex<3, 10, 9>	WORKING, 320x256
PAL 512 x 256	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via psx/gpu/mod lower_right: PackedVertex<3, 10, 9>	WORKING, 512x256
PAL 640 x 256	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via psx/gpu/mod lower_right: PackedVertex<3, 10, 9>	WORKING, 640x256
PAL 368 x 256	InvalidY @ psx/gpu/vertex PackedVertex::const_try_from() via psx/gpu/mod lower_right: PackedVertex<3, 10, 9>	InvalidX @ psx/hw/gpu/gp1 display_mode()
PAL 384 x 256	InvalidX @ psx/hw/gpu/gp1 display_mode()	Truncated width = 364 pixels displayed on screen, (disp and draw are 384)

It looks like maybe I should revert the 368 -> 384 change for now as it seemed more sensible before.

I also tested Interlaced Display / Draw Framebuffers (side-by-side). They all use the non-interlaced screen height for display since the interlaced mode flag is currently hardcoded. I should be able to fix this too. UPDATE: Interlaced modes are now enabled in 221c2c6

TODO later, not in this PR:

• Looks like pixel mode is also hardcoded to Depth::Bits15. Not sure how to select Depth::Bits24, "24-bit true-color mode"

  psx-sdk-rs/psx/src/framebuffer.rs

  Line 80 in fe4e0ec

  .display_mode(res, video_mode, Depth::Bits15, false)?

• Interlaced PAL (screen height 512) doesn't have the background fill.
  • bg_color_cmd: 0x02, - Fill Rectangle in VRAM, 3rd arg: Width+Height (YsizXsizh) ;Xsiz counted in halfwords, steps of 10h , via https://problemkaputt.de/psx-spx.htm#gpumemorytransfercommands
  • this does not have an off-by-one error: it really is the Y size (not lower right corner), but its max value is 0x1ff (511) so it cannot fully fill a 512 high region, the best it can do is fill 511 and leave one pixel row blank.
  • Suggested FIX/workaround: in psx/src/gpu/mod.rs accept all values for bg_size, unless bg_size.1 == 512, then change it to 511, or min(bg_size.1, 511) . This seems a problem with the PSX GPU command, not the sdk.

Also, I don't know why the last test failed on CI -- it appears unrelated to my changes, and I don't get the error locally.

[ayrtonm]

wow thanks @hornc. Unfortunately I don't have much bandwidth at the moment so I can only help out a bit and very sporadically, especially for the next two months or so.

Sorry for the description verbosity

Not a problem, it's actually very helpful (even if I can't respond to every point immediately)

Looks like pixel mode is also hardcoded to Depth::Bits15. Not sure how to select Depth::Bits24, "24-bit true-color mode"

The simplest possible thing would be to add a depth: Depth (or maybe Option<Depth>) argument to Framebuffer::new. It's starting to have a lot of arguments so maybe a Framebuffer::new with a minimal number of args/sane defaults and another constructor using some variation of the rust builder pattern would be more sensible. Honestly it's easy to get stuck on making the "perfect" API so if you just wanna have something that lets you make demos I'd go for the first option. Either way you're not locked into an option. There's relatively few users (there was at least one recently on the rust-lang discord) so some API churn is fine.

Also, I don't know why the last test failed on CI -- it appears unrelated to my changes, and I don't get the error locally.

CI tests nightly so it's probably because you weren't using a recent enough nightly. I was able to reproduce locally by running rustup update nightly today. I fixed CI in #33 but seems that rerunning the test doesn't automatically rebase. If you push to the PR it should rerun and be fixed, I think.

nit: thanks for keeping some level of consistency by using $TOPIC: $TITLE for commit titles. I'm not super picky about contributors doing this and you've been working on it more than I have recently so it's really up to you to decide how consistent you want to be. If you want to rewrite commit messages/squash the fixup commits I find git rebase -i HEAD~7 works well. My usual workflow on github is to push new commits for changes then squash/reword things at the end before the rebase and merge, but again totally up to you.

[ayrtonm]

I took brief a look and I'd say mostly LGTM. I'd need to think a little about the changes to gpu/mod.rs (the off-by-one thing) and the 368 vs 384 thing.

[hornc]

Thanks for merging and your review earlier, @ayrtonm I re-wrote the commit messages, hopefully as you suggested. I was originally trying to adhere to the naming convention, but I had so much temporary debug commits, I got lazy. I appreciated the nitpick :)

The other thing I did was back out the 368 to 384 width change. I agree it needs more research, and it can be done independently of these changes. The code, for whatever reason, seems to work better with the current 368 value.

I meant to write a follow up response after reviewing my change again, but time slipped away.

I'm pretty sure the off-by-one thing is correct, it's the difference between using the rectangle size to get the point for an adjacent rectangle in VRAM, and referring to a corner point within the first rectangle, which is what the lower_right value needs to be. I wasn't completely happy with the code, but I couldn't find a more compact way to subtract (1, 1) , hence

        let lower_right = PackedVertex::try_from(offset + size - Vertex::new((1, 1)))?;

[ayrtonm]

The other thing I did was back out the 368 to 384 width change

yep I noticed that, I think that was the right call for now.

I wasn't completely happy with the code, but I couldn't find a more compact way to subtract (1, 1) , hence

yeah the sdk's API for this isn't great but it looks fine. I think in an earlier version I was passing around (i16, i16) instead of Vertexs but I think I ended up changing it because of issues around rust's orphan rule. Basically impl Trait for Type requires that either Trait or Type be defined in the crate with the impl. So the sdk crate can't impl traits that override the + or - operators like core::ops::{Add, Sub} for (i16, i16) which is why I added the Vertex type. I'm not totally happy with it since I agree x - Vertex::new((1, 1)) is kinda ugly compared to x - (1, 1), but just giving you some background on why it's the way it is.