Integrate changes from Playstation Vita port

[lynxlynxlynx]

@Northfear ported GemRB to Vita and is still improving the experience. Here's the comparison to master: https://github.com/gemrb/gemrb/compare/master...Northfear:master#diff-8f0e2fd7d01827ddfa312dad5e1c752aR38

Two trivial bits were already included, but much of the rest will need some form of cleanup. Let's start talking about it here, so the work can progress.

Starting with cmake changes:

• since there are plenty of new files only relevant for vita, I suggest you make a platforms/vita folder to house them and adjust any paths as needed
• CMakeLists.txt:
  • put the new block into a file under the new folder and include it here instead
  • remove the extraneous spaces and commented out code
  • GemRB.cfg.vita will fall out of sync with the main ones quickly, so it's better to generate it from them (I'm a bit sceptical you need to override that many paths too; GamePath and GemRBPath are two crucial ones)
  • you're adding the same compiler flags to both c/c++, so it'd be cleaner if you saved the new ones in a string first
  • similarly for the libraries, extracting out the shared ones
  • USE_SDL/USE_SDL2 have been deprecated, so please remove them
• Find*.cmake: these two files will probably go away #915 , but you can already set an env var to influence the search path. Exporting SDLDIR=$VITASDK/arm-vita-eabi/ should take care of both.
• gemrb/CMakeLists.txt: no libdl on vita? It's odd that cmake would populate CMAKE_DL_LIBS then. I suggest you rather set it to an empty string in the main cmake file, so this change then isn't needed. I see you didn't touch the one user in gemrb/plugins/GUIScript/CMakeLists.txt, so now I'm confused.

[Northfear]

@lynxlynxlynx ok, I think I'm done with cmake changes. I've removed Makefile that was used for VPK creation too and now the final Vita package is generated by cmake only. Pre-filled Vita config is removed too. Generating one would require some regex magic, but I think that few lines of readme that'll explain everything Vita-specific would work too (only CachePath and GemRBPath overrides are needed) https://github.com/gemrb/gemrb/compare/master...Northfear:master

[lynxlynxlynx]

Nice. Here's more feedback:

• is VITA_VERSION deliberately not in our format?
  • Are there any space limitations for the string? Otherwise it'd be more maintainable to just use the cmake var with the version string we construct earlier
• VitaBilinear looks like it's gone away, so the readme needs updating
• the new config options — no need to ifdef them everywhere
• the logging stuff looks fine
  • Logging.h: no need to handle __sgi as well, nobody will run IRIX on a Vita. :D Also, what error does the missing include spawn? It sounds like it might be needed on other platforms
• is the special lookup for GemRB.cfg really needed? We do look into the data dir by default
  • if it is, please move it a bit lower near the other platform ifdefs
• SaveGameIterator.cpp and VFS.cpp changes are misindented
• readonly_mmap is disabled. Is mmap not available? We should detect that in cmake if it's the reason for not using it
• mem.cpp: interesting, why is the include needed?
• GUIScript.cpp: yikes, it'd be good to know which control this breaks on — voice selection?

Left to review: SDLVideo Very hacky: PluginLoader.cpp, MappedFileMemoryStream

[Northfear]

@lynxlynxlynx

is VITA_VERSION deliberately not in our format?

I've tried using GEMRB_VERSION, but it just resulted in broken package. Looks like version format is "XX.XX" and Vita is very strict about it. I can remove it altogether, since it's kinda optional and only shown in information screen in LiveArea

Also, what error does the missing include spawn? It sounds like it might be needed on other platforms

• #include in Logging.h was for va_list

error: 'va_list' has not been declared
78 | GEM_EXPORT void LogVA(log_level level, const char* owner, const char* message, va_list args);

is the special lookup for GemRB.cfg really needed?

Not working w/o the special lookup. Moved it down.

readonly_mmap is disabled. Is mmap not available?

mmap is not available. I've seen some pull request in our newlib, but it doesn't looks like it's going anywhere right now. Not sure how should I proceed with this one

mem.cpp: interesting, why is the include needed?

HAVE_POSIX_MEMALIGN is defined in config.h and it's available on Vita. Otherwise I was getting error: 'memalign' was not declared in this scope

GUIScript.cpp: yikes, it'd be good to know which control this breaks on — voice selection?

Yikes indeed. Every resource control. Voice, portraits, character import. The weirdest thing is that sometimes wi was zero. Other time it was ci. On some rare occasions I was told that it was actually working (tho it was always failing for me). This one is pretty scary, but that's the only place where I've encountered problems with PyArg_ParseTuple. Visible ones at least.

I might try rebuilding Python/GemRB with alternative SDK and see how it goes, but so far both GCC 9.1 and 10.1 with and w/o optimizations were producing the same result

Very hacky: PluginLoader.cpp, MappedFileMemoryStream

Maybe I should just disable them in cmake? Both aren't really supported on Vita and I was just commenting out stuff till it worked.

[lynxlynxlynx]

• VITA_VERSION: ok. This should extract it in the required format: string(REGEX REPLACE "^(.)\.(.)\.(.).*$" "0\\1.\\2\\3" ${VITA_VERSION} ${GEMRB_VERSION})
• cstdarg: interesting; then it defintely doesn't need to be if-defed for vita, just include it.
• mmap: should be checked in cmake with CHECK_FUNCTION_EXISTS like the others and then that used to ifdef things. I'm sure it can be done cleaner.
• memalign: ok.
• python: yeah, worth a shot, since you mention there are two ports.

[Northfear]

All of the changes are in. Also alternative SDK did fixed that weird python parsing bug (and maybe even some others too).

[lynxlynxlynx]

Here are more nits and comments:

• GemRB.cpp changes look like they're indented with spaces
• GmRB.cpp: please add a comment for _newlib_heap_size_user
• Logger/Vita.cpp is also misindented (didn't notice before)
• Logger/Vita.cpp: it looks like sceClibPrintf is fully printf-like, so you could simplify LogInternal to use a single call
• Logger/Vita.h: is the using namespace std; really needed?
• VFS.cpp has some minor style issues (if-blocks have the braces in the same line; strcpy shouldn't have space around its argument list; pointer star should be near the pointer name, not type) — we're slowly unifying code to use consistent style (as we change code, since we avoid pure style commits)
• VFS.h: we create readonly_mmap for windows, so it shouldn't be excluded here
• GUIScript.cpp: Vita is miscapitalized

Considering how many times we check for defined(HAVE_MMAP) || defined(WIN32), it would be cleaner to create a joint define in cmake (in cmake/cmake_config.h.in, I guess), let's say SUPPORTS_MEMSTREAM, and simplify the uses.

Left to review: SDLVideo

[lynxlynxlynx]

• SDL12Video.cpp:
  • can width != VITA_FULLSCREEN_WIDTH || height != VITA_FULLSCREEN_HEIGHT ever not be true?
  • can VITA_FULLSCREEN_WIDTH size be detected somehow? This downscaling would be useful for other tiny screens, so I'm wondering if we can generalize it
  • style as per VFS
  • vitaDestRect looks like it's only used in CreateDisplay, so it could be a local var
  • would anyone ever want to not use fullscreen?
  • ShowSoftKeyboard shows plenty of new state vars. I think it would be cleaner to create a new struct to hold them and then have that as a member. As we generalize the joystick input support, they can be moved out as needed.
    • and another one for the general joystick stuff
• SDL20Video: why are you excluding SDL_WINDOW_OPENGL? And does the resizable bit have any side effects?
• SDLVideo:
  • a lot of the VITA ifdefs could be changed to SDL_NumJoysticks() > 0
  • gGameController doesn't need to be ifdefed and could be called simply gameController, since we don't use prefixes
  • if you move the VITA_SPEED_MOD definition, vitaPointerSpeed adjustment doesn't need to be ifdefed
  • SDL_INIT_JOYSTICK shouldn't be fatal (in general), so please remove the return; also doesn't need to be ifdefed
  • similarly ProcessAxisMotion and the rest of the new functions and (most of?) their uses don't need to be ifdefed
    • GetCurrentKeyValue is the exception; it should be a noop on non-vita for now and special care needs to be taken we don't trigger a callback twice (eg. where it's preceded by OnSpecialKeyPress)
  • you're again sometimes indenting with spaces
  • HandleJoyAxisEvent: if you pad diagonally, that sends separate events per axis?
  • style as per VFS
  • HandleJoyButtonEvent: you can extract the button.state != SDL_PRESSED block one level higher and add a comment that BTN_CROSS/CIRCLE don't set that state
  • SDLVideoDriver::PollMovieEvents: no need to ifdef either

I'm sure we can clean up the text input handling, but let's leave that for later.

[Northfear]

can width != VITA_FULLSCREEN_WIDTH || height != VITA_FULLSCREEN_HEIGHT ever not be true?

It can. You can set video mode to 640x480 and play at native game resolution (either centered on Vita screen or scaled)

can VITA_FULLSCREEN_WIDTH size be detected somehow? This downscaling would be useful for other tiny screens, so I'm wondering if we can generalize it

Not with SDL probably. Our implementation just returns empty list of available resolutions if I recall correctly. And SDL1 scaling is VIta specific too (SDL_SetVideoModeScaling is not available in SDL and it was made due to lack of SDL_RenderCopy and it's destination rect)

vitaDestRect looks like it's only used in CreateDisplay, so it could be a local var

Moving to local var. I was planning to use it for both SDL1 and SDL2 scaling (and calculate it somewhere in SDLVideo), but since SDL2 render isn't functional yet, there's no reason to keep it there.

would anyone ever want to not use fullscreen?

Yeah. Fullscreen scales the game area to fullscreen. fullscreen=off centers it (so it's rendered with native game res). There's no bilinear filtering with fullscreen=off, so image looks sharper (but smaller).

SDL20Video: why are you excluding SDL_WINDOW_OPENGL? And does the resizable bit have any side effects?

It was failing to init with SDL_WINDOW_OPENGL. I'll revert SDL2 changes for now, since it's not really functional ATM

HandleJoyAxisEvent: if you pad diagonally, that sends separate events per axis?

Yeah

Fixed the rest. No more spaces hopefully (It was all VSCode fault. I'm a tab guy really). JoystickControl still needs some work to support at least some basic xbox/dualshock mappings. I'll try adding support for Xbone controller later (since I have one to test with). Doing it with SDL_GameController would be ideal, but it's SDL2 only.

[lynxlynxlynx]

Ok. SDL2 will become the default soon and we never targetted feature parity (eg. SDL1 doesn't have a clipboard and we don't jump through hoops to reimplement it). So SDL_GameController is fine and the future, but since you don't have a working backend, I'm not sure what's the best advice.

The way you did the two new structs, you can simply convert them to classes. It's just about the different default visibility — in a class everything is private by default and you seem to have mostly private members. So classes fit better.

VFS: static analyzers will also complain about strcpy use being unsafe; use strlcpy or strncpy instead

The rest are just style comments:

• VFS, SDL12Video, SDLVideo as noted before
• gemrb/core/CMakeLists.txt has a new misindented )
• case blocks don't need to be in an extra scope, in fact it's actively discouraged by analysers

[Northfear]

Added SDL2 support for gamepads. Working fine with Xbox one controller. SDL1.2 one is Vita/XBone only. Probably not worth bothering beyond that, considering that SDL1.2 won't be supported soon anyway. Fixed the rest, but I can't see whats wrong in SDL12Video and SDLVideo regarding code style.

[lynxlynxlynx]

Thanks, getting close!

Why are SDL_CONTROLLER_AXIS_LEFTX et al defined twice? And since it's a long list, I suggest you extract it to a new header (don't forget to update the year in the license block), and then just include it where needed.

DPadSoftKeyboard and GamepadControl can also go in their own files.

gemrb.6.in deserves a mention of the two new config options (sorry, didn't remember sooner).

Style harmonization:

• if-blocks should have the braces in the same line, eg. https://github.com/gemrb/gemrb/blob/58a0b4c2f606d7ef2e73928a877d882b8bfd455f/gemrb/core/Game.cpp#L162-L168
• and with a space before the starting paren
• even when it results in a single line statement, a braced block is preferred
• pointer star should be near the pointer name, not type, eg. compare the new closedir to gameController
• when using NULL for pointers, use nullptr instead
• uppercase type suffixes, eg. const float JOY_AXIS_SPEEDUP = 1.03f; should have 1.03F (this is a sonar warning just because i and l look similar)

These instructions sometimes conflict with surrounding code, but this is the way we slowly harmonize it to the most used style, with a goal to eventually be able to enforce it (discussion in #161). So I'm being annoyingly strict here, since it's new code and since I was just recently fighting the 186 days of estimated effort to reduce code smells (according to sonar; down to 156 now).

[Northfear]

Done. The only thing I'm not sure about is pointer star in functions. Should it be near the return type or function name? And there's 1 more fix added. swprintf(optNum, sizeof(optNum)/sizeof(optNum[0]), L"%zu. - ", i+1); was resulting in "zu." instead of dialog option number.

[lynxlynxlynx]

zu: just cast i to an int and use %d to avoid the need to special case.

[lynxlynxlynx]

Northfear, can you rebase the remaining changes on current master? Everything but the audio hack is fine for inclusion, but there were some changes made since (eg. from polishing the PR).

[bradallred]

this needs to be rechecked in subviews

[Northfear]

this needs to be rechecked in subviews

I've already tried it a bit and it definitely needs fixing. Out of the major stuff only controller support is broken tho. Quite a lot of changes to the rendering. It may take some time until I'll make something out of it (SDL 2 broke since master (probably due to our SDL implementation), SDL 1.2 seems to be 32bpp only now (which is slower here), GLES is probably the best option, but that would require rewriting shaders in CG (since there's no GLES on Vita) and probably doing some other stuff too)

[bradallred]

SDL 1.2 seems to be 32bpp only now

not intentionally. All the PixelIterator code and all other new bits support it theoretically. What is the issue?

[Northfear]

Broken videos (4 small vids with wrong colors, Like an incorrect depth format) w/o audio and generally lower performance (but the same as 32 bit mode from master).

[bradallred]

I'm not sure what to make of that. You can see that the video surfaces are created using the format of the video file. Everything is done with SDL_BlitSurface so even if the window is 16bits the video should blit just fine AFICT.

[bradallred]

I also don't see anywhere we are forcing a RGBA8888 format outside of the VLC plugin

[Northfear]

Huh, that's weird. I'll try looking into it then

[bradallred]

I just made a change that might help the performance a tad 🤷

realistically, I wouldn't expect much from SDL 1.2 (for me its the same). We sacrificed some SDL 1.2 performance to rearchitect for a sane SDL 2 implementation and to fix rendering bugs or add new features. The SDL 2 backend is completely rewritten from master.

I'm assuming this is a fully optimized build? We have a lot of abstractions now the compiler is expected to boil off during optimization.

[bradallred]

there is a FIXME in RedrawScreenStencil that mentions breakage on 16bit platforms. I haven't ran across another.

[lynxlynxlynx]

@Northfear please double check things are still in order, thanks!

[Northfear]

Oh, they're not right now. I have a branch with some subviews fixes, but I'll only be able to look into it next week