Add Splitwolf: Splitscreen Wolfenstein 3D#2532
Conversation
|
Thanks. I think rather than duplicating a lot of the functionality this could be implemented reusing some of the code from the other module which is what we do elsewhere when modules share similar code. |
|
@joolswills glad to make that change if possible, can you point me to an example? I'll see what can be shared. |
|
eg. amiberry uses some of uae4arm module code. In regards to this module it should be able to reuse gamedata from the wolfsdl module as well as the code there. |
|
Can I help this conversation? Member of SplitWolf team. |
|
@joolswills good call on the game data, I was thinking about that. I will make that change. I will investigate what I can share between wolf4sdl's install script and this one and get back to you shortly. @DoomJedi no worries, I will handle it :) |
|
@joolswills ok, I've pushed a commit that uses the same ROM directory as wolf4sdl ( After investigating to see if there is anything to share between the wolf4sdl install script and this one, I found that the only function we could share directly is downloading game files (even dependencies are different as we're using SDL2 here). However, the existing wolf4sdl setup script does not compile a Spear of Destiny Demo binary, so it doesn't make sense for wolf4sdl to download Spear of Destiny Demo files. Instead of modifying wolf4sdl in this pull request to support Spear of Destiny Demo, or modifying wolf4sdl's setup script to download game files it doesn't use, I propose that I send a subsequent pull request that does the following:
This subsequent pull request will have to come after the holidays, but I will definitely send it. What do you say? |
|
@joolswills, think we can get this in as-is and update Wolf4SDL later? |
|
no sorry. I think the module needs work to less duplicate the functionality of the wolf4sdl module but I don't have time to help you do this currently. I may look into this at some point and re-implement the module myself so it shares functions. |
|
Ok, I have consolidated:
I cannot consolidate:
@joolswills Is this enough? I can't think of anything else that is worth consolidating. |
|
The game data could have parameters so it downloads only required files. There should be no install_bin - there should be sources_ and install_ only. install_bin is for binary only packages which we try and avoid. |
| } | ||
|
|
||
| function add_games_wolf4sdl() { | ||
| eval "declare -A games="${2#*=} |
There was a problem hiding this comment.
there is no need to do this - you can access games from this function if games is set in the parent with local scope.
There was a problem hiding this comment.
Hmm, even in Splitwolf? I need to change the games parameter for Splitwolf, hence this gross little eval.
There was a problem hiding this comment.
I originally moved the declare -A wolf3d_games=(...) for the array up to the top of the splitwolf.sh and tried to use it as a global variable, but add_games_wolf4sdl inside of wolf4sdl.sh could not access it, so instead I passed it in and used this hack to define it because you cannot pass an associative array in bash.
If there is a better way, I'll gladly do it, but I am not aware.
| if [[ ! -f "$romdir/ports/wolf3d/vswap.wl6" && ! -f "$romdir/ports/wolf3d/vswap.wl1" ]]; then | ||
| if [[ ! -f "$romdir/ports/wolf3d/vswap.wl1" && | ||
| ! -f "$romdir/ports/wolf3d/vswap.wl6" | ||
| ]]; then |
There was a problem hiding this comment.
no need to reformat existing code as part of the addition.
We provide binaries because the Splitwolf build takes about 5 minutes on a 3B+ and 45 minutes on a Zero! You want us to remove the option to install from binary? |
Instead of doing this, I'll just modify wolf4sdl to build the Spear of Destiny Demo so people can play it. Otherwise I'll just be adding code I'll remove when I fix wolf4sdl in my next PR. Agreed? |
|
@joolswills just pushed a commit that fixes formatting and updates wolf4sdl to create a Spear of Destiny Demo binary and link. |
| ['vswap.sdm']="Splitwolf - Spear of Destiny Demo" | ||
| ) | ||
|
|
||
| add_games_wolf4sdl "$md_inst/bin/splitwolf.sh %ROM%" "$(declare -p games)" |
There was a problem hiding this comment.
@joolswills this is where I pass games in splitwolf.sh -- if I defined it as local above, add_games_wolf4sdl would not be able to access it.
There was a problem hiding this comment.
it would be able to access it
bash local scope variables are usable in child functions - we use that for the $md_ variables which are passed to the modules - they are local in the parent.
There was a problem hiding this comment.
No, that doesn't work. If i use declare -A at the top of splitwolf.sh, I cannot access it inside of wolf4sdl.sh's functions as it is overridden by wolf4sdl's definition of the variable by the same name.
Honestly, we are way outside of my knowledge when it comes to bash scripting. What you're describing sounds like it should be possible, but I do not know how to accomplish it. If you can tell me precisely the code change you want to see, I'll gladly do it, but I've been reading about this for an hour and I cannot come up with anything better than passing the serialized associative array as a parameter.
Yes please. |
I think I understand - go ahead for now. |
@joolswills I don't understand. You can choose to install Splitwolf from source, it is not a binary-only release.
Many other packages provide binaries, including:
Why shouldn't Splitwolf provide a binary install option in addition to source? It needs to build 6 separate binaries, and 45 minutes is a long time to compile for people on less powerful Raspberry Pis. |
|
Just to clarify re: binary installations, see: https://github.com/RetroPie/RetroPie-Setup/blob/master/scriptmodules/packages.sh#L233 If you remove the custom install_bin_splitwolf function, binary installation can still occur via the generic rp_installBin function (also in that script) which sources binaries that Jools builds from the RetroPie server. Binaries are provided for main and opt packages, so your scriptmodule would be covered. |
|
@psyke83, thanks for clarifying, I had no idea that was how it worked; I thought @joolswills was saying that binary installation would not be supported! I have removed the function. @joolswills How are the binaries generated? I assume they will include the contents of |
|
The binary will include the entire contents of |
|
Apologies for not going into detail. I was short on time. Thanks @psyke83 for clarifying. Regarding a reply I saw in email but I think you removed regarding variable scope. I guess you have it working, but in case it's useful - a local var in bash is available for the function and any child functions called by it. We use it in RetroPie-Setup for the $md_ vars that are usable in modules. They are local to the parent. |
|
Hey @joolswills, please let me know if there is anything else I can do to get this ready to merge. Thanks! |
|
Just the variable scope stuff I mentioned above. |
| rp_module_desc="SplitWolf - 2-4 player split-screen Wolfenstein 3D / Spear of Destiny" | ||
| rp_module_help="Game File Extension: .wl6, .sod, .sd2, .sd3\n\nCopy your game files to $romdir/ports/wolf3d/\n\nIf you add new game files, run: sudo ~/RetroPie-Setup/retropie_packages.sh splitwolf configure" | ||
| rp_module_licence="NONCOM https://bitbucket.org/linuxwolf6/splitwolf/src/master/license-mame.txt" | ||
| rp_module_section="opt" |
| rp_module_help="Game File Extension: .wl6, .sod, .sd2, .sd3\n\nCopy your game files to $romdir/ports/wolf3d/\n\nIf you add new game files, run: sudo ~/RetroPie-Setup/retropie_packages.sh splitwolf configure" | ||
| rp_module_licence="NONCOM https://bitbucket.org/linuxwolf6/splitwolf/src/master/license-mame.txt" | ||
| rp_module_section="opt" | ||
| rp_module_flags="dispmanx !mali !kms" |
There was a problem hiding this comment.
please remove !mali and !kms - as it's sdl2 it should be fine on those targets.
There was a problem hiding this comment.
as it's sdl2 dispmanx should be removed also.
| } | ||
|
|
||
| function build_splitwolf() { | ||
| mkdir "bin" |
| local defs="${opt#* }" | ||
| make clean | ||
| make $defs DATADIR="$romdir/ports/wolf3d/" | ||
| mv $bin "bin/$bin" |
|
|
||
| moveConfigDir "$home/.splitwolf" "$md_conf_root/splitwolf" | ||
|
|
||
| setDispmanx "$md_id" 1 |
There was a problem hiding this comment.
this is an sdl1 only feature.
|
Went over the code again and noticed a few things. Thanks. |
|
@joolswills thanks for the thorough review, I have implemented all of your feedback and tested locally. There was one issue I could not address, the issue of variable scoping for the
The reply you're referencing is here: #2532 (comment) The issue here is that we need the associative array Now, from your replies, I gather that you do not want me to pass the variable around, and instead want the variable to be declared at the top of the file and accessed within a function. So, if I declare it with a unique name, declare -A -g wolf_games=(
['vswap.wl1']="Wolfenstein 3D demo"
['vswap.wl6']="Wolfenstein 3D"
['vswap.sd1']="Wolfenstein 3D - Spear of Destiny Ep 1"
['vswap.sd2']="Wolfenstein 3D - Spear of Destiny Ep 2"
['vswap.sd3']="Wolfenstein 3D - Spear of Destiny Ep 3"
['vswap.sdm']="Wolfenstein 3D - Spear of Destiny Demo"
)Then, I can use it within However, if I then declare declare -A -g wolf_games=(
['vswap.wl1']="Splitwolf - Wolf 3D Demo"
['vswap.wl6']="Splitwolf - Wolf 3D"
['vswap.sod']="Splitwolf - Spear of Destiny Ep 1"
['vswap.sd2']="Splitwolf - Spear of Destiny Ep 2"
['vswap.sd3']="Splitwolf - Spear of Destiny Ep 3"
['vswap.sdm']="Splitwolf - Spear of Destiny Demo"
)It does not override the The solution to this problem is to serialize This is the only way I found to pass associative arrays between functions in Bash (source https://stackoverflow.com/a/8879444). If there is a better way to do this, I'll gladly implement it, but declaring the variable globally will not work. |
|
I think you misunderstood. You declare it inside the parent function, as a local array. Then use it in the child function. As I said above, we use this in retropie for the md_ vars. They are not globals. |
|
Please show me the code change you’d like to see.
… On Dec 30, 2018, at 4:01 PM, Jools Wills ***@***.***> wrote:
I think you misunderstood. You declare it inside the parent function, as a local array. Then use it in the child function.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
diff --git a/scriptmodules/ports/splitwolf.sh b/scriptmodules/ports/splitwolf.sh
index e3eac2d3..4b985482 100644
--- a/scriptmodules/ports/splitwolf.sh
+++ b/scriptmodules/ports/splitwolf.sh
@@ -111,7 +111,7 @@ _EOF_
['vswap.sdm']="Splitwolf - Spear of Destiny Demo"
)
- add_games_wolf4sdl "$md_inst/bin/splitwolf.sh %ROM%" "splitwolf" "$(declare -p games)"
+ add_games_wolf4sdl "$md_inst/bin/splitwolf.sh %ROM%" "splitwolf"
moveConfigDir "$home/.splitwolf" "$md_conf_root/splitwolf"
}
diff --git a/scriptmodules/ports/wolf4sdl.sh b/scriptmodules/ports/wolf4sdl.sh
index 58991531..49b8b122 100644
--- a/scriptmodules/ports/wolf4sdl.sh
+++ b/scriptmodules/ports/wolf4sdl.sh
@@ -32,7 +32,6 @@ function _get_opts_wolf4sdl() {
}
function add_games_wolf4sdl() {
- eval "declare -A games="${3#*=}
local port="$2"
local cmd="$1"
local game
@@ -136,7 +135,7 @@ _EOF_
['vswap.sdm']="Wolfenstein 3D - Spear of Destiny Demo"
)
- add_games_wolf4sdl "$md_inst/bin/wolf4sdl.sh %ROM%" "wolf3d" "$(declare -p games)"
+ add_games_wolf4sdl "$md_inst/bin/wolf4sdl.sh %ROM%" "wolf3d"
moveConfigDir "$home/.wolf4sdl" "$md_conf_root/wolf3d"
|
|
In addition, the add_games functions need to be standalone, so both modules need that function that should contain the array declaration - another _add_games can be created that goes through the array. |
|
Ok, @joolswills, I've made the change. Each script now has its own Is there anything else here for me to correct? |
| # remove obsolete emulator entries | ||
| while read game; do | ||
| delEmulator "${game%% *}" "splitwolf" | ||
| done < <(_get_opts_splitwolf) |
There was a problem hiding this comment.
There was a problem hiding this comment.
If this code is unnecessary I can remove it from both wolf4sdl and splitwolf install scripts, just say the word.
There was a problem hiding this comment.
You don't need it then. It was added to wolf4sdl to remove entries from older versions of the module. Since this is a new module it's not needed.
There was a problem hiding this comment.
@joolswills done deal, I removed it from Splitwolf only.
|
Thanks. BTW you are not defining the array globally but locally. Just that in bash local variables can be accessed from any child functions. I made one more comment about clarification about some code - as I'm not sure it's needed. |
|
Please can you squash this PR and I will merge. Thanks. |
lazd
force-pushed
the
splitwolf
branch
2 times, most recently
from
1d40ea7 to
81d8932
Compare
|
@joolswills squashed and ready to merge. Thanks for your thorough reviews and help getting this into shape. A couple of questions:
|
|
They can choose "update retropie-setup" to just have it appear. Binaries won't be available for exp modules, but I am happy to move it later and build binaries etc. Could open a thread on the forum for user testing and get some feedback. |
|
Merged - thanks for your hard work on this. |
|
Couldn't find SplitWolf in latest retropie...did I miss it? If not, why was it removed? |
|
@DoomJedi It's in the experimental section - but please use the forum for support. |
|
Thank you, glad to hear, will check there. Sorry for bumping this thread. |
4 participants
Splitwolf
Based on the same setup script as Wolf4SDL, this PR adds Splitwolf from Team Raycast, 2-4 player splitscreen Wolfenstein 3D with new game modes and full gamepad support!
That's right, you can frag Nazis with up to 4 of your friends on the big screen!
Notable Features
Installation
Installation works identically to Wolf4SDL, with the same logic to check which game files are available. Binaries were compiled on Raspbian 8 (Jessie), so they work properly on old and new RetroPie setups alike.
Game files should be added to
roms/ports/wolf3d/before installation. If not present, the install script will download the shareware versions of Wolf3D and Spear of Destiny.Support
For RetroPie-specific Splitwolf support and questions (RetroPie installation, game files, etc), post on the RetroPie forums.
For general Splitwolf support and questions (how to build, support for other platforms, map packs, etc), post on the Team Raycast forums.
If you already have Splitwolf up and running and you've found a bug (glitch with the game, crash while playing, etc), please report it using the Splitwolf issue tracker.
If you've found a bug with the RetroPie Setup installer, please report it using the RetroPie-Setup issue tracker.
DO NOT USE ISSUE TRACKERS FOR GENERAL SUPPORT OR QUESTIONS, YOU WILL NOT BE HELPED!
See the Splitwolf wiki for additional details, compilation instructions, game mode instructions, etc.
Credits
None of this would be possible without the hard work of the following people:
Multiplayer framework, new game modes: LinuxWolf
Gamepad support, configuration UI, and RetroPie implementation: lazd
Additional art: DoomJedi, Untrustable, Atina, PSTrooper, ArmanAhmadi
Title Screen: Atina
Title Screen Font: Tormentor667
SDL Port: Moritz "Ripper" Kroll (http://www.chaos-software.de.vu)
Original Wolfenstein 3D: id Software (http://www.idsoftware.com)