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

nxengine-evo: init at 2.6.4 #64408

Merged
merged 2 commits into from Jul 20, 2019
Merged

nxengine-evo: init at 2.6.4 #64408

merged 2 commits into from Jul 20, 2019

Conversation

scubed2
Copy link
Contributor

@scubed2 scubed2 commented Jul 7, 2019

Motivation for this change

Add a game.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

So far, I've done:
nix-env -i nxengine
to install it, then started a new game and played a bit. For some reason, the music seems to be going fast, but that's the only issue I've encountered. (So, only a cosmetic bug. It is still fully playable, and the game itself is at the right speed.)

I see that the instructions say there should be a maintainer, but I don't know what to put there.

Note that the game has 2 parts:
the engine, which is opensource
the data, which is freeware and obtained separately
This package only has the engine. So, it is not playable by itself.

Copy link
Member

@mmahut mmahut left a comment

Choose a reason for hiding this comment

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

Fails for me after installation with:

$ nx 
Loading settings...
Couldn't open file settings.dat.
No saved settings; using defaults.
videoinfo: desktop bpp 32
Graphics::SetResolution(2)
Setting scaling 2 and fullscreen=no
SDL_SetVideoMode: 640x480 @ 32bpp
Graphics::FlushAll()
 error << SIFLoader::LoadHeader: failed to open file 'sprites.sif' >> 
 error << Failed to initialize graphics. >> 

pkgs/games/nxengine/default.nix Outdated Show resolved Hide resolved
pkgs/games/nxengine/default.nix Outdated Show resolved Hide resolved
pkgs/games/nxengine/default.nix Outdated Show resolved Hide resolved
@mmahut
Copy link
Member

mmahut commented Jul 7, 2019

@GrahamcOfBorg build nxengine

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 7, 2019

Fails for me after installation with:

Yes. This is intentional as written. I looked around to see what other tools do. I found archlinux does this:
https://aur.archlinux.org/cgit/aur.git/tree/nxengine.sh?h=nxengine
So, there's a wrapper script. It downloads the freeware game, then sets up a directory, and cd's into there. That way, it will be able to find all of its files, and will be sanely laid out.
I'm also not sure how mixing in the freeware would work legally.
So, I took the more conservative approach.

I can set it up with a script like that one, if so desired. Then, it would start the game right away, which is convenient.

Please tell me your preference.

@mmahut
Copy link
Member

mmahut commented Jul 7, 2019

@scubed2 I think it would be nice to have this working out of box. So a kind of script/wrapper to do that, plus moving the package to unfree if required.

@aanderse
Copy link
Member

aanderse commented Jul 8, 2019

It wasn't immediately obviously to me where to get the data from when looking at the homepage. Is the data unfree and not redistributable?

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 13, 2019

Okay, the package should be completely working now.

mmahut:

  • the game assets are now included in share
  • the license is changed to be gpl3, unfreeRedistributable to account for having the assets
  • instead of a wrapper, I sed $out into the binary
    • the assets are released with the binary version, so this seems reasonable to me
  • music plays at the correct speed

aanderse:

The game assets are redistributable.
https://www.cavestory.org/forums/threads/cave-story-freeware-license.13817/
Note that this is Cave Story, not to be confused with Cave Story+, which is fully proprietary and non-redistributable.

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 14, 2019

For reference, the patches have been sent as:
nxengine/nxengine-evo#150
nxengine/nxengine-evo#151

@aanderse
Copy link
Member

@scubed2 I see your changes have been merged upstream. Great work!
Please fetch the patches from upstream so we don't need to maintain them in NixOS.

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 16, 2019

I've downloaded the patches from upstream and put them into pixtone.patch. So, now it matches.

Either way, the patches wouldn't need to be maintained, since the next release would make both patches obsolete.

I also recompiled and ran it for a few minutes to sanity check.

@aanderse
Copy link
Member

@scubed2 I was referring to using fetchpatch so we directly reference the patch upstream instead of keeping a local copy in nixpkgs. If you want to update your PR to use fetchpatch we can merge now, or if there is no rush just wait for the next release and update.

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 17, 2019

I've changed it to use fetchpatch from the github commits of the upstream changes. So, hopefully it should be good now.

For my own curiosity, how different is it to have the patch locally vs. using fetchpatch?

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

We don't like to maintain patches in nixpkgs if they exist elsewhere and can be pulled down. Duplication of code, among other things.

I ran the game and had a few errors like this:

error << pxt->load: file '/nix/store/gds3sl3alkqwzzd9wk47xywldvy224lb-nxengine-evo-2.6.4/share/nxengine/data/pxt/fx08.pxt' not found. >>

The game seemed booted up, so I'm not sure if that is a problem or not.

pkgs/games/nxengine-evo/default.nix Outdated Show resolved Hide resolved
@scubed2
Copy link
Contributor Author

scubed2 commented Jul 18, 2019

I've switched to using nativeBuildInputs. It still builds and runs.

The pxt files are the sound effects. If some are missing, then it just won't play that sound effect (that is, it will be silent). So, missing some would be innocuous. I've played the game and haven't noticed anything missing. So, it seems to be working. I'm not sure how to be certain if sound effect 8 is used. That is, at worst it will cause some sound effects to be missing, but likely it is working as intended.

@aanderse
Copy link
Member

@scubed2 You should add yourself to maintainers/maintainer-list.nix as the first commit to this PR. Then you can amend your second commit to include yourself in the meta.maintainers field.

Aside from that, we're good to go. Thanks!

@scubed2
Copy link
Contributor Author

scubed2 commented Jul 20, 2019

Added myself as a maintainer in a separate commit.

@aanderse
Copy link
Member

@GrahamcOfBorg build nxengine-evo

@aanderse aanderse changed the title nxengine: init at 1.0.0.6 nxengine-evo: init at 2.6.4 Jul 20, 2019
@aanderse aanderse merged commit f5945fc into NixOS:master Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants