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.

Sign up for GitHub

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

Jump to bottom

lr-mgba.sh: enable lr-mgba for GB and GBC (SGB also supported) #2366

Merged
merged 1 commit into from
Apr 20, 2018
Merged

lr-mgba.sh: enable lr-mgba for GB and GBC (SGB also supported) #2366

merged 1 commit into from
Apr 20, 2018

Conversation

hhromic
Copy link
Member

@hhromic hhromic commented Apr 12, 2018

As mentioned in #2295, lr-mgba is mature enough now to support GB/GBC/SGB as well as GBA.
I've been using it for several weeks now and all works fine. The emulator detects the system type automatically and can use all BIOSes available, even Super Gameboy.
The emulation speed in my standard-configured RPI3 is very satisfactory too.
This is for the master upstream repository of lr-mgba, used when you install it from source.
I strongly suggest the binary version in RetroPie is updated too to make all work out-of-the-box.

The following BIOS files were used for testing each system type:

a860e8c0b6d573d191e4ec7db1b1e4f6  gba_bios.bin
32fbbd84168d3482956eb3c5051637f5  gb_bios.bin
dbfce9db9deaa2567f6a84fde55f9680  gbc_bios.bin
d574d4f9c12f305074798f54c091a8b4  sgb_bios.bin

I kept the original logic used to set the emulator as default, e.g. set it as default in non armv6 platforms.
This should keep users of slow hardware unaffected if they are using lr-gambatte already.

@hhromic
Copy link
Member Author

hhromic commented Apr 12, 2018 •
edited
Loading

If this change gets merged and the binary updated, I will update the Wiki to include BIOS file information too, similar to the PSX Wiki page.

psyke83
psyke83 reviewed Apr 13, 2018
View reviewed changes
scriptmodules/libretrocores/lr-mgba.sh Outdated
local def=1
isPlatform "armv6" && def=0
addEmulator $def "$md_id" "gba" "$md_inst/mgba_libretro.so"
addSystem "gba"
for system in gb gbc gba; do
Copy link
Member

Choose a reason for hiding this comment

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

You should ensure the system variable is local scope

Copy link
Member Author

Choose a reason for hiding this comment

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

oh forgot that, thanks for spotting.

psyke83
psyke83 reviewed Apr 13, 2018
View reviewed changes
scriptmodules/libretrocores/lr-mgba.sh Outdated
for system in gb gbc gba; do
mkRomDir "$system"
ensureSystemretroconfig "$system"
addEmulator "$def" "$md_id" "$system" "$md_inst/mgba_libretro.so"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain if @joolswills wants this patch included at all, but if so, perhaps he can chime in on whether the previous defaults should be preserved, or if all three systems should use lr-mgba as set by this patch (I think not).

@hhromic
Copy link
Member Author

hhromic commented Apr 13, 2018 •
edited
Loading

Perhaps the correct thing to do is to follow the same approach/logic than for lr-genesis-plus-gx and lr-picodrive, two competing emulators for various Sega 8/16 bits consoles:

lr-picodrive is used as -default- for: megadrive mastersystem segacd sega32x
lr-genesis-plus-gx is used as -default- for: gamegear sg-1000
lr-genesis-plus-gx is used as alternative for: megadrive mastersystem segacd

Then I can rework the patch to achieve this style of behaviour for the Game Boy consoles:

lr-gambatte is used as -default- for: gb gbc
lr-mgba is used as -default- in non-armv6 for: gba
lr-mgba is used as alternative for: gb gbc
lr-gpsp is used as -default- in armv6 for: gba
lr-vba-next is used as alternative for: gba
lr-tgbdual is used as alternative for: gb gbc

How does this sound?

Edit:
Just to make it clear, if lr-mgba is accepted as default for all systems in non-armv6 platforms, then the proposed simplified scheme is:

lr-mgba is used as -default- in non-armv6 for: gb gbc gba
lr-gambatte is used as -default- in armv6 for: gb gbc
lr-gpsp is used as -default- in armv6 for: gba
lr-tgbdual is used as alternative for: gb gbc
lr-vba-next is used as alternative for: gba

I've been using lr-mgba upstream for several weeks and it works really well in my vanilla RetroPie RPI3. I think it is safe to recommend it by now for the non-armv6 platform, just make sure the binaries are updated.

@hhromic
Copy link
Member Author

hhromic commented Apr 20, 2018

Hi @psyke83 , @joolswills , did you have any chance to review this PR?
Let me know of your thoughts to keep working on it and get it merged.
If you are not happy with the defaults handling I propose (check the edited comment), we can discard that, but at least I think we should allow lr-mgba to be an alternative for GB/GBC even if it is not their default emulator.
Cheers and thanks for all your work!

@joolswills
Copy link
Member

I will add this as an alternative but I am not going to change any defaults currently.

@hhromic
Copy link
Member Author

hhromic commented Apr 20, 2018

Good stuff. Do you want me to refactor this PR accordingly?

@joolswills
Copy link
Member

If you would like - please squash the commits also. alternatively I am happy to implement it when I next have some time.

@hhromic
Copy link
Member Author

hhromic commented Apr 20, 2018

No problem from my side, I was also planning to squash the commits. Will update shortly. Thanks for the feedback.

@joolswills
Copy link
Member

Thanks.

@hhromic hhromic force-pushed the lr-mgba-enable-gb-gbc branch from d3f91bc to a58f172 Compare April 20, 2018 20:38
@hhromic
Copy link
Member Author

hhromic commented Apr 20, 2018 •
edited
Loading

Update the PR now:

  • Updated the module description and help.
  • Added GB/GBC as alternative but not default.
  • Kept (as originally) GBA as default only in non armv6 platforms.

I will update the Wiki after this is merged too. Thanks!

@joolswills
Copy link
Member

Thanks

@joolswills joolswills merged commit d453c1a into RetroPie:master Apr 20, 2018
@hhromic hhromic deleted the lr-mgba-enable-gb-gbc branch April 20, 2018 20:53
@hhromic
Copy link
Member Author

hhromic commented Apr 20, 2018

@joolswills , before I forget , it is possible to also update the binary for lr-mgba to be up-to-date to its current master? Thanks!

@joolswills
Copy link
Member

Can do. It was updated a few weeks ago btw.

@hhromic
Copy link
Member Author

hhromic commented Apr 21, 2018

Great, should be fine then. When you have time, it won't hurt if you update again just in case.
Thanks a million.

@hhromic
Copy link
Member Author

hhromic commented May 8, 2018

Hi @joolswills , sorry to bring this up again. Looks like users in the forum are needing to update lr-mgba from source to get the latest features enabled recently: https://retropie.org.uk/forum/post/146141
Maybe a refresh of the binary is a good idea? Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psyke83 psyke83 psyke83 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hhromic @joolswills @psyke83