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

WIP: Xonotic fix 2 #90282

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

WIP: Xonotic fix 2 #90282

wants to merge 1 commit into from

Conversation

fps
Copy link
Contributor

@fps fps commented Jun 14, 2020

Motivation for this change

The xonotic package missed some features:

  • in-game video encoding
  • the nice xolonium font with unicode support
  • player UID support

Since I'm rather inexperienced with nixpkgs I would welcome changes/additions by the original maintainers before merging.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@fps fps mentioned this pull request Jun 16, 2020
10 tasks
@fps fps changed the title Xonotic fix 2 WIP: Xonotic fix 2 Jun 16, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 02:59
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

You made a lot of changes unrelated to fixing anything. This actually breaks a lot of functionality. Maybe you could try to seperate the additional libraries from other improvements so we can review it more easily.

pkgs/games/xonotic/default.nix Outdated Show resolved Hide resolved
pkgs/games/xonotic/default.nix Outdated Show resolved Hide resolved
pkgs/games/xonotic/default.nix Outdated Show resolved Hide resolved
pkgs/games/xonotic/default.nix Outdated Show resolved Hide resolved
@fps
Copy link
Contributor Author

fps commented Dec 20, 2020

Hi! Thanks for the review. I'm pretty sure you are right on all your raised points. Like I said in the description I'm not an experienced packager. I will give the PR another shot in the next few days..

@fps
Copy link
Contributor Author

fps commented Dec 28, 2020

OK, I'm a little bit like a fish out of the water here, since I'm only used to working with my own solo-repos. I have no idea how to cleanly update my own nixpkgs-fork and then rebasing my changeset on the current master.. Working on it though :)

- fix video encoding from in-game
- fix the missing font
- fix player UID handlin
@fps
Copy link
Contributor Author

fps commented Dec 28, 2020

OK, I tried to redo the patch in a less invasive way now. It still seems to run fine here on my machine (including player UID tracking, video recording and the font)

@ghost
Copy link

ghost commented Dec 28, 2020

Okay. I think your PR doesn't break anything anymore now. Can you help me out with how to test the features you enabled? I have never seen an option for video recording and the font looks exactly the same to me as before.

@fps
Copy link
Contributor Author

fps commented Dec 28, 2020

Does the font look like this for you (that's what it should look like)?

xonotic20201228214259-00

You can render a video (usually used for rendering demos) by opening the console (shift-ESC) and entering cl_capturevideo 1. That starts recording. To end the recording enter cl_capturevideo 0

To test the UID tracking, join the exe .pub | relaxed running | CTS/XDF server and try to finish a map. If you have enabled "Allow player statistics to track your client" and "Allow player statistics to use your nickname" in the profile settings screen and you have these changes in then there will NOT appear a message in the console after finishing the map stating something like "player yada yada scored a new record but sadly it will be lost".

N.B. I'm eof.homs in-game.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. The new diff doesn't break any existing functionality, so that's a big step.
I still find it difficult to fully understand each change. IMO the three new features you mentioned should be split up into different commits, or even into seperate PRs.

Also note this paragraph from CONTRIBUTING.md:

Writing good commit messages

In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand why a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.

"Fix a couple of issues" tells me exactly nothing about what was done or why.

extraPostFetch = ''
cd $out
rm -rf $(ls | grep -v "^data$")
rm -rf $(ls | grep -v "^data$\|^key_0.d0pk$")
Copy link

Choose a reason for hiding this comment

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

What is the purpose of this change?

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 think that's the public key of the UID tracking infrastructure. This change does keep it from getting deleted in the data directory. IIRC this change was necessary for it to work. Possibly I'm conflating this with the required d0_blind_id building, since I was poking the thing from all directions back then in june ;)

Copy link

@ghost ghost Dec 29, 2020

Choose a reason for hiding this comment

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

Thanks for the explanation. If this was also explained in the commit message of one of the commits that would be awesome.

@@ -62,7 +62,7 @@ let
sha256 = "0axxw04fyz6jlfqd0kp7hdrqa0li31sx1pbipf2j5qp9wvqicsay";
};

buildInputs = [ unzip libjpeg zlib libvorbis curl ]
buildInputs = [ unzip libjpeg zlib gmp libvorbis libtheora curl libpng freetype ]
Copy link

Choose a reason for hiding this comment

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

Are freetype, libtheora and so on really needed as buildInput for the dedicated server as well?

Copy link
Contributor Author

@fps fps Dec 29, 2020

Choose a reason for hiding this comment

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

Good question. I suppose not. I was told in the chat yesterday about the nifty -developer option for the executables which tells us what libraries are being attempted to be loaded. Here's the output for the dedicated and sdl-variant (glx doesn't get built on my system):

xonotic-dedicated:

Trying to load library... "libd0_blind_id.so.0" - loaded.
Trying to load library... "libd0_rijndael.so.0" - loaded.
Trying to load library... "libcurl.so.4" - loaded.
Skeletal animation uses SSE code path
Trying to load library... "libode.so.3" "libode.so.2" "libode.so.1" "/home/fps/.nix-profile/bin/libode.so.3" "/home/fps/.nix-profile/bin/libode.so.2" "/home/fps/.nix-profile/bin/libode.so.1" - failed.

xonotic-sdl:

Trying to load library... "libd0_blind_id.so.0" - loaded.
Trying to load library... "libd0_rijndael.so.0" - loaded.
Trying to load library... "libcurl.so.4" - loaded.
Skeletal animation uses SSE code path
Trying to load library... "libode.so.3" "libode.so.2" "libode.so.1" "/home/fps/.nix-profile/bin/libode.so.3" "/home/fps/.nix-profile/bin/libode.so.2" "/home/fps/.nix-profile/bin/libode.so.1" - failed.
Initializing client
Couldn't load gfx/palette.lmp, falling back on internal palette
DPSOFTRAST available (SSE2 instructions detected)
Failed to init SDL joystick subsystem: 
Trying to load library... "libvorbis.so.0" - loaded.
Trying to load library... "libvorbisfile.so.3" - loaded.
Trying to load library... "libogg.so.0" - loaded.
Trying to load library... "libtheora.so.0" - loaded.
Trying to load library... "libvorbis.so.0" - loaded.
Trying to load library... "libvorbisenc.so.2" - loaded.
Trying to load library... "libavw.so.1" "libavw.so" "/home/fps/.nix-profile/bin/libavw.so.1" "/home/fps/.nix-profile/bin/libavw.so" - failed.

It seems I was still missing ODE and AVW which might be required for some features..

@@ -73,7 +73,9 @@ let

dontStrip = target != "release";

buildPhase = lib.optionalString withDedicated ''
buildPhase = ''
(cd ../d0_blind_id && ./configure --prefix="$out" && make -j $NIX_BUILD_CODE -l $NIX_BUILD_CORES)
Copy link

Choose a reason for hiding this comment

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

Can d0_blind_id be built in a seperate derivation to keep things simple? All of the things you are doing here are usually done automatically by stdenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could do that. I figured that there are no other known users of d0_blind_id besides xonotic so it wasn't worth the trouble of doing a separate derivation.

pkgs/games/xonotic/default.nix Show resolved Hide resolved

postFixup =
lib.optionalString withDedicated (''
patchelf '' + commonPatchelfArgs + ''
Copy link

Choose a reason for hiding this comment

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

Nitpicks:
Maybe change the indentation to two spaces more than the previous line?
Maybe use ${commonPatchelfArgs} instead of '' + commonPatchelfArgs + ''?

@fps
Copy link
Contributor Author

fps commented Dec 29, 2020

By the way: can you tell me why the GLX build is optional? For many people the glx variant works better than the SDL variant. Shouldn't we just build both by default?

@ghost
Copy link

ghost commented Dec 29, 2020

By the way: can you tell me why the GLX build is optional? For many people the glx variant works better than the SDL variant. Shouldn't we just build both by default?

Both get build by Hydra and are available in the binary cache. You can choose between glx and sdl by installing the xonotic-sdl or xonotic-glx package. This will install the respective variant as xonotic in your $PATH. You can also install both variants by adding xonotic-sdl and xonotic-glx to your packages, one of them with hiPrio.

I don't want the glx variant even installed because I am running wayland and I have no use for it. I hate it when there are multiple desktop items for one program and I need to choose the correct one every time I launch it.

@fps
Copy link
Contributor Author

fps commented Dec 30, 2020

Ok, thanks for the clarification. I will split up the PR into individual parts in the coming days when I find time to work on it again..

@stale
Copy link

stale bot commented Jun 28, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 28, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
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

2 participants