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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

glfw-wayland-minecraft: update to glfw 3.4 #306264

Closed

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Apr 23, 2024

Description of changes

The set-cursor patch is taken from: Admicos/minecraft-wayland#56

And other changes including fractional scaling is already upstreamed in 3.4 thus not needed anymore. I override the patch on the original glfw to de-duplicate the packaging code.

I tested with prismlauncher.override { withWaylandGLFW = true; }, Minecraft 1.20.2, Java 17, on KDE 6 wayland session with a 125% scaled display. The game is indeed a native wayland window (checked in KWin debug console), and the cursor setting on opening game GUI (#293296 (comment)) is working correctly instead of crashing like the unpatched glfw.

The set-cursor patch itself seems quite reasonable and probably is suitable for upstreaming? CC @DanShaders. Also I'm not sure if it's a good idea to patch unconditionally or conditionally on the glfw package, rather than a separated glfw-wayland-minecraft.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

The set-cursor patch is taken from:
<Admicos/minecraft-wayland#56>

And other changes including fractional scaling is already upstreamed in
3.4 thus not needed anymore.
@DanShaders
Copy link

The set-cursor patch itself seems quite reasonable and probably is suitable for upstreaming?

I mean, it felt like a horrible hack when I was writing it, but you can try upstreaming it, I wouldn't mind

@getchoo
Copy link
Member

getchoo commented Apr 23, 2024

Also I'm not sure if it's a good idea to patch unconditionally or conditionally on the glfw package, rather than a separated glfw-wayland-minecraft

+1. with glfw-wayland being dropped in #293296 and prismlauncher being the only real consumer of this, it'd make perfect sense imo

edit: using this in the standard glfw package would also allow for us to drop the withWaylandGLFW override from prismlauncher entirely, making wayland support a default

@oxalica
Copy link
Contributor Author

oxalica commented Apr 24, 2024

using this in the standard glfw package would also allow for us to drop the withWaylandGLFW override from prismlauncher entirely, making wayland support a default

Pros:

  1. This is not a breaking change and it makes more fail cases pass. Though the implementation is kinda not ideal, it's a best effort one.
  2. Makes the wayland backend usable by default, which benefits all downstream packages other than Minecraft.
  3. No user overriding or recompilation is required.

Cons:

  1. The author of the patch @DanShaders thinks the patch is too hacky. I'm not sure if it could be upstreamed or the functionality would be "correctly" implemented in the future. I'm not an expert on GLFW and/or wayland.
  2. We need to maintain the patch in-tree in case of future GLFW updates, because they need to be updated together. I don't think I'm competent for it. At least we need another maintainer for the package and/or patch in nixpkgs.

Use the patch in the main package but disabled by default is another approach, but it will requires end-users to override and recompile GLFW (though, it does not take too long).

@Aleksanaa
Copy link
Member

Aleksanaa commented Apr 26, 2024

This is in conflict with #306827, which drops glfw-wayland-minecraft

@oxalica oxalica mentioned this pull request Apr 26, 2024
13 tasks
Comment on lines +1 to +2
{ glfw }:
glfw.overrideAttrs (old: {
Copy link
Member

Choose a reason for hiding this comment

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

This is generally not a recommended pattern as a user could have something like the following overlay which would cause an infinite recursion:

final: prev: {
  glfw = final.glfw-wayland-minecraft
}

What you could do instead is have some kind of common.nix that you can import and use in both derivations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you could do instead is have some kind of common.nix that you can import and use in both derivations.

So do you imply that we should to keep glfw-wayland-minecraft in parallel with the original glfw, rather than making it a package option?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess making it a package option of the original glfw is also a good way to do this!

@Scrumplex
Copy link
Member

Could you add me as a maintainer to the glfw (or common) derivation? I wasn't pinged by ofborg for this PR :(

@SuperSandro2000
Copy link
Member

Closing in favor of #310073

@oxalica oxalica deleted the pkg/glfw-wayland-minecraft-update branch May 29, 2024 08:07
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

6 participants