libretro / beetle-pce-fast-libretro

Standalone fork of Mednafen PCE Fast to libretro
GNU General Public License v2.0
29 stars 55 forks source link

Correct pixel aspect ratio and line hiding scaling #40

Closed westonlast closed 7 years ago

Tatsuya79 commented 7 years ago

The scanline menu is working fine now.

Remaining issues: -512 width mode gives a black screen (test: TV sport basketball before a match start) -341 width mode is not centred as it is displayed inside of a 352 width FB

see with R-type part1 (japan version only is in 341 mode): 11 black pixels on the right side.

39 see this modification about it:

vce.dot_clock == 1 is the 341/352 mode Ideally, activate those missing pixels and have an option to remove them.

Those hacks need to be watched too.

Tatsuya79 commented 7 years ago

Just changing 512 by 513 here makes the 512 mode work. It displays this with PAR.

The first 16 pixels on the left (perhaps a bit more?) shouldn't be there. It should start at this white vertical border and show the same kind of white border on the right side. That's 544 pixels wide atm, don't know why.

westonlast commented 7 years ago

Sorry, I'll get to it as soon as I can. Thanks for the feedback. I assumed the emulator was more correct than it is (hey, Ys I + II work :D).

Tatsuya79 commented 7 years ago

I think I managed to fix at least the standard "overscan OFF" usual behaviour. (check here) I need help for the overscan option and I'm pretty sure this could be written in a better way. :)

First time I can see the top menu in Super Darius not being cut off. It wasn't possible in mednafen stand-alone. (let's hope nothing is broken too)

Tatsuya79 commented 7 years ago

Something else you probably didn't notice if you're always using PAR, there is a 128 pixels width kind of display. It gets really ugly when stretched on the whole screen with a display ratio.

Identified offenders are Final Zone II doing this in its cutscenes a lot and Gradius II at least in 1 scene of its intro.

It would be needed to draw black bars on each side / make an empty 256 pixels wide box to paste it in the middle, for it not to get stretched too much. ...and identify the mode too.

I'm looking at this to get hints but not sure atm.

Also Gensou Tairiku Auleria is missing half its intro showing a black screen. But it happens on stand-alone as well.

Tatsuya79 commented 7 years ago

Gradius II displays 112 pixels, so that looks to be variable.

Final Blaster 80 pixels wide during intro.

(that's ok I found a way)

westonlast commented 7 years ago

This is what I mean by "the core is supposed to tell Retroarch the correct aspect ratio for each output resolution." We would have to dig into TurboGrafx-16 pixel clock details to determine accurate ratios. The table of widths appears to be a hack to kind of make this "work" close enough so that no one notices. I'd much prefer doing some math on a TurboGrafx-16 register or something.

Tatsuya79 commented 7 years ago

Should be good, Set_Geometry is constantly called though.

Some annotations about the changes here: https://github.com/westonlast/beetle-pce-fast-libretro/pull/1

andres-asm commented 7 years ago

Constantly called as on every frame? That's bad

Tatsuya79 commented 7 years ago

That's what I thought. Need a check or something but I don't know how to code that.

andres-asm commented 7 years ago

I'll send a PR on your PR

hizzlekizzle commented 7 years ago

Are we all good here? If so, I can merge it all in

Tatsuya79 commented 7 years ago

I just added those 2 commits Radius did. If he thinks that's OK we should be good.

westonlast commented 7 years ago

@fr500, I know calling APIs that we don't actually need to call is "bad" in general (performance; not knowing what the API is actually doing, thus my following curiosity), but I'm curious: What's bad about calling this API in particular? I imagine it assigning the height a width values for the scaler, and that's about it.

andres-asm commented 7 years ago

does it work? I did it blindly

Tatsuya79 commented 7 years ago

Yes that's working great, thanks for that!

andres-asm commented 7 years ago

@Tatsuya79 cool! glad it works. Didn't have idea of what to test...

@westonlast a fronted may (or may not) tear down the driver to adjust to some geometry changes. With RetroArch it's not the case at least, RA actually checks if there are changes so the check could be thought as redundant. Still... it's still better not to call it every frame it take some CPU time even if it's negligible, and it causes it to spam about the function being called every frame in the log.

westonlast commented 7 years ago

I thought the whole point of RETRO_ENVIRONMENT_SET_GEOMETRY was to avoid driver re-initialization, like it says here: https://github.com/libretro/RetroArch/blob/eaed1c96beb1ac134585af8588148eec5d2ba223/libretro-common/include/libretro.h#L862

Tatsuya79 commented 7 years ago

Should we merge?

andres-asm commented 7 years ago

@westonlast indeed, but that doesn't mean frontends handle it correctly, and it certainly doesn't mean it's free. Even logging has a cost. It may be negligible in most platforms but it's not the case everywhere.

No point calling SET_GEOMETRY every frame if you're not actually changing geomtry.

@Tatsuya79 I think it's all good unless @twinaphex doesn't like that I called an internal mednafen function to change horizontal overscan

hizzlekizzle commented 7 years ago

Went ahead and pulled the trigger, so we'll just have to keep our eyes/ears open for trouble reports. In the meantime, high-fives all around. Mission accomplished.

westonlast commented 7 years ago

@fr500, "that doesn't mean frontends handle it correctly"... That's exactly what it means. This is the definition of design by contract.

I never said it was good to call it when it isn't needed, but semantically it's not necessarily bad, except for the logging side effect which isn't documented.

Go team!

westonlast commented 7 years ago

Now to fix every other core, like PSX!

andres-asm commented 7 years ago

It's not just logging. RetroArch actually checks if it needs to do a change and returns if it doesn't. Who knows what phoenix or kodi's retroplayer are doing in that case.

A question regarding this PR. why is the AR 6:5? is that the pixel aspect ratio? I would assume display aspect ratio would have been 4:3?

We have added an option on other cores so the core reports DAR instead of PAR so the picture would not look squished.

Just wondering, I'm not too well versed in aspect ration matters.

Tatsuya79 commented 7 years ago

The PAR depends of the video mod the pc engine uses. It works with Retroarch main settings->video->aspect ratio index->1:1 PAR.

http://forum.fobby.net/index.php?t=msg&goto=2554&

6:5 is "middle of the road" mednafen default = 4:3 + overscan in a lot of games. The majority of games on the system use 256 pixels wide mode and 3-242 scanlines range displays a valid signal in most cases. (unlike the SNES where it's black almost all the time)

It could be 4:3 with a shorter scanline range but judging from videos on Youtube the original system shows different results depending of CRT TVs and games.

I'm not sure what could be done with DAR for this system.

westonlast commented 7 years ago

"Who knows what phoenix or kodi's retroplayer are doing in that case." If they are not doing what the documentation specifies, they are wrong, which is not the concern of cores. The world might end tomorrow; do you want to defend against that in code, too? Design by contract.

https://github.com/libretro/beetle-pce-fast-libretro/pull/40#issuecomment-254103685