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

fixes jzintv background color to black on pillar boxed 4:3 emulator output and 16:9 displays #3507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gemba
Copy link
Contributor

@Gemba Gemba commented Apr 1, 2022

  • sets 4:3 display format for jzintv automagically (launcher file)
  • fixes a UI freeze bug when a mouse is attached and moved on a RPI3 when jzintv is running

ref: https://retropie.org.uk/forum/topic/32433/jzintv-has-black-border-or-full-screen-color

scriptmodules/emulators/jzintv.sh Outdated Show resolved Hide resolved
@@ -61,17 +70,60 @@ function install_jzintv() {
}

function configure_jzintv() {
mkRomDir "intellivision"
local system="intellivision"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to make system a variable, since it's not changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is more a DRY approach. I don't like to maintain the same information on n locations. But I can remove it or set at least readonly. What is expected the "RetroPie-way"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it as-is then. A variable is used if the emulator supports multiple systems, but it's not the case here and I'd like that intellivision is explicit for both addSystem and addEmulator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I have put it as it was before the PR.

scriptmodules/emulators/jzintv.sh Outdated Show resolved Hide resolved
scriptmodules/emulators/jzintv.sh Show resolved Hide resolved
@Gemba Gemba requested a review from cmitu April 3, 2022 15:13
# ingest additional options
while [[ \$# -gt 1 ]] ; do
options+=(\$1); shift
done
Copy link
Member

Choose a reason for hiding this comment

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

if you have shifted the input params why just do something like options+=("$@")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The jzintv options are "space-free", while the ROM (last parameter) could contain spaces. The last parameter should be quoted to avoid shell interpretation.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how that wouldn't work with my line above - please can you give an example ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I condensed it. Didn't notice the quotes in your suggestion initially.

@@ -27,6 +27,14 @@ function sources_jzintv() {
# jzintv-YYYYMMDD/ --> jzintv/
mv jzintv-[0-9]* jzintv
cd jzintv/src
# archive files have CRLF linendings, patch has CR only
find . -type f \( -iname "*.c" -o -iname "*.h" \) -exec flip -u {} \+
Copy link
Member

@joolswills joolswills Apr 3, 2022

Choose a reason for hiding this comment

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

I'm not sure I like the converting of all the files to LF to apply the patch. Seems overkill. Also do you mean the patches has LF only not CR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes. LF (0x0a) was ment.
Some other options I considered here: Change the source zip archive to LF (hosted at your site, must be remembered when a new source archive is deployed), convert patches to CRLF (awkward as RaspiOS uses UNIX style endings), use --ignore-whitespace in applyPatch (may have knock-on effects).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no success when changing the patch line endings. patch hinted to use either --ignore-whitespace or --binary.
I changed it to dos2unix and limited the conversion to the source file which will be patched.

@@ -44,8 +52,9 @@ function build_jzintv() {
mkdir -p jzintv/bin
cd jzintv/src

isPlatform "rpi" && local extra='EXTRA=-DPLAT_LINUX_RPI'
Copy link
Member

@joolswills joolswills Apr 3, 2022

Choose a reason for hiding this comment

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

You need to declare the variable as local for all platforms not just for the RPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to challenge this. Works also fine when the initial clause is not true.

Copy link
Member

Choose a reason for hiding this comment

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

It may work, but it will mean the variable is used undeclared for non RPI platforms and if the variable had something in it in a parent function that data would be included. So this is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Now I understand. Corrected it.

addEmulator 0 "${md_id}-ecs" "intellivision" "$md_inst/bin/jzintv ${options[*]} %ROM%"
addSystem "intellivision"
addEmulator 1 "$md_id" "$system" "bash '$start_script' %XRES% %YRES% %ROM%"
addEmulator 0 "$md_id-ecs" "$system" "bash '$start_script' %XRES% %YRES% --ecs=1 %ROM%"
Copy link
Member

Choose a reason for hiding this comment

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

The file is marked as executable and lives on a native linux fs so you don't need to use "bash" to run it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Leftover from a sample scriptmodule I peeked on unreflected (dosbox.sh).

Copy link
Member

Choose a reason for hiding this comment

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

That's because the script in dosbox is included in $romdir which may be on a fat32 fs so this is needed in that case. As the script is moved now it's not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

echo "Launching: \$jzintv_bin \${options[*]} \"\$rom\"" >> /dev/shm/runcommand.log

pushd "$romdir/$system" > /dev/null
\$jzintv_bin \${options[*]} "\$rom"
Copy link
Member

Choose a reason for hiding this comment

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

If using my example above this would need to be \${options[@]} btw - Unless I'm missing something this should work ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above / intiial comment.

@joolswills
Copy link
Member

I have made some comments but I'm not sold on including this as it is currently due to the size, and what seems to be unnecessary complexity with the launch script etc.

@@ -18,7 +18,7 @@ rp_module_section="opt"
rp_module_flags="sdl2"

function depends_jzintv() {
getDepends libsdl2-dev libreadline-dev
getDepends libsdl2-dev libreadline-dev flip
Copy link
Member

@joolswills joolswills Apr 3, 2022

Choose a reason for hiding this comment

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

BTW we don't use flip anywhere else. dos2unix is used though so that would be a preference. But you can probably just use sed/tr though to change the files that are needed (or include the patch with CRLF line endings and avoid having to change the source).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using dos2unix now. See above.

@Gemba
Copy link
Contributor Author

Gemba commented Apr 4, 2022

I have made some comments but I'm not sold on including this as it is currently due to the size, and what seems to be unnecessary complexity with the launch script etc.

I guess the launcher size has been reduced already by this review.
A word why I did not patch everything in the source: It would take away the user's option to set a custom resolution and once the border (-b switch in jzintv) function works in jzintv's SDL2 implementation the hard-wiring of a strict maximum (=screen filling) 4:3 jzintv dsiplay may be too limiting.

- sets 4:3 display format for jzintv automagically (launcher file)
- fixes a UI freeze bug when a mouse is attached and moved on a RPI3 when jzintv is running

ref: https://retropie.org.uk/forum/topic/32433/jzintv-has-black-border-or-full-screen-color
@Gemba Gemba force-pushed the fb_jzintv_16_to_9_bg_fix branch from 19e74bb to 1d83718 Compare July 8, 2023 18:15
@Gemba
Copy link
Contributor Author

Gemba commented Jul 8, 2023

Flipped the content of the patch files as the filename was not reflecting the patch content.

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.

3 participants