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

feat: support hyprcursor #18

Closed
wants to merge 5 commits into from
Closed

Conversation

trowgundam
Copy link
Contributor

I've added support for hyprcursor.

@trowgundam trowgundam mentioned this pull request Apr 28, 2024
build.sh Outdated
name = $theme_name
description = ${theme_comment#Comment=}
version = $(git show -s --format=%cd --date=format:%Y%m%d HEAD)
cursor_directory = hyprcursors
Copy link

Choose a reason for hiding this comment

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

It should be cursors_directory instead of cursor_directory
https://github.com/hyprwm/hyprcursor/blob/main/docs/MAKING_THEMES.md#manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it should. I've fixed.

@sgoudham sgoudham self-assigned this May 15, 2024
@sgoudham sgoudham removed their assignment May 22, 2024
@sgoudham sgoudham self-requested a review May 22, 2024 23:25
Copy link
Contributor

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

Hey 👋
Thanks for the PR!

This repository currently has no active maintainers against it so it definitely needs some love! I really dislike that the zips are in source control and wouldn't be comfortable releasing another version that way again.

I don't use Hyprland myself but I'm happy to merge this as long as others can confirm that it works or if you can provide screenshots of the cursors being applied on Hyprland.

@trowgundam
Copy link
Contributor Author

trowgundam commented May 25, 2024

In the process of getting a screenshot, I came to the realization that there is an extra step necessary. It isn't explicitly stated in the hyprcursor documentation, but there is an "compilation" step that needs to be executed using hyprcursor-util. I've modified the build script to do this. This does introduce a new build dependency though, hyprcursor itself. Here is a screenshot with the cursor in use on Hyprland, and to illustrate it is a hyprcursor instead of the xcursor, I set the size to 128 and left the screenshot uncropped.
screenshot

@sgoudham
Copy link
Contributor

This does introduce a new build dependency though, hyprcursor itself.

I feel like building cursors for hyprcursor should be extracted out to another file or come under an optional parameter as I'd imagine we don't want to force building cursors for hyprland if people aren't on hyprland.

This removes the build dependency on hyprcursor for those who do not use
hyprcursor. Which at this time would be anyone not using Hyprland.
@trowgundam
Copy link
Contributor Author

Yes, I agree. It's something I thought about doing after implementing it. I've separated it out to a separate build command. Technically you don't really need the xcursor portion on Hyprland, but it's usually best to have them for things that don't support it on your system, like SDDM. That said, both the xcursor and hyprcursor can co-exist in the same folder, so I would suggest any release artifacts just include them both. Since hyprcursor uses the SVG, the hyprcursor theme is much smaller than its xcursor equivalent.

@AxerTheAxe
Copy link

AxerTheAxe commented May 26, 2024

The cursors built just fine and are working great. However, it was a bit tricky figuring out if 'Catppuccin-Variant-Color' or 'theme_Catppuccin-Variant-Color' was the right folder for the theme. After checking the contents, it was clear which one to use, but I think the naming could be clearer or the extra files could be cleaned up at the end of the build script.

Edit: The Hyprcursor seems to be blurrier and less detailed at lower sizes than the Xcursor equivalent at those same sizes. I could not figure out if this is the fault of the theme or Hyprcursor itself.

2024-05-25-204953_hyprshot

@trowgundam
Copy link
Contributor Author

Edit: The Hyprcursor seems to be blurrier and less detailed at lower sizes than the Xcursor equivalent at those same sizes. I could not figure out if this is the fault of the theme or Hyprcursor itself.

Make sure you are copying the directories from the "dist" folder after building and NOT from the build folder. The "theme_Catppuccin-Variant-Color" folder is the compiled hyprcursor theme and ends up merged with the xcursor theme in the dist folder. The name should be the name of the end folder minus the "-Cursors" on the end, as seen in the "manifest.hl" file.

@nonetrix
Copy link

I use the Nix package so I am somewhat curious how support will be there once and if this is merged

@AxerTheAxe
Copy link

Edit: The Hyprcursor seems to be blurrier and less detailed at lower sizes than the Xcursor equivalent at those same sizes. I could not figure out if this is the fault of the theme or Hyprcursor itself.

Make sure you are copying the directories from the "dist" folder after building and NOT from the build folder. The "theme_Catppuccin-Variant-Color" folder is the compiled hyprcursor theme and ends up merged with the xcursor theme in the dist folder. The name should be the name of the end folder minus the "-Cursors" on the end, as seen in the "manifest.hl" file.

Thanks for the info. The Hyprcursor is still blurry but after testing with some other cursors I think that is an issue with Hyprcursor itself.

@nonetrix
Copy link

nonetrix commented May 26, 2024

Any issue or PR or something for that on Hyprland or Hyprcursor repo? Likely on Hyprlands side since it's rendering thing

@trowgundam
Copy link
Contributor Author

trowgundam commented May 26, 2024

Edit: The Hyprcursor seems to be blurrier and less detailed at lower sizes than the Xcursor equivalent at those same sizes. I could not figure out if this is the fault of the theme or Hyprcursor itself.

Make sure you are copying the directories from the "dist" folder after building and NOT from the build folder. The "theme_Catppuccin-Variant-Color" folder is the compiled hyprcursor theme and ends up merged with the xcursor theme in the dist folder. The name should be the name of the end folder minus the "-Cursors" on the end, as seen in the "manifest.hl" file.

Thanks for the info. The Hyprcursor is still blurry but after testing with some other cursors I think that is an issue with Hyprcursor itself.

How are you setting your cursor. This is how I have it in my Hyprland config:

env = HYPRCURSOR_THEME,Catppuccin-Mocha-Blue
env = HYPRCURSOR_SIZE,24

If you are only using the XCURSOR equivalents you are using the XCursor version of the cursor instead. Also for testing you can do the following to try different cursors:

hyprctl setcursor Catppuccin-Mocha-Blue 24

@AxerTheAxe
Copy link

Edit: The Hyprcursor seems to be blurrier and less detailed at lower sizes than the Xcursor equivalent at those same sizes. I could not figure out if this is the fault of the theme or Hyprcursor itself.

Make sure you are copying the directories from the "dist" folder after building and NOT from the build folder. The "theme_Catppuccin-Variant-Color" folder is the compiled hyprcursor theme and ends up merged with the xcursor theme in the dist folder. The name should be the name of the end folder minus the "-Cursors" on the end, as seen in the "manifest.hl" file.

Thanks for the info. The Hyprcursor is still blurry but after testing with some other cursors I think that is an issue with Hyprcursor itself.

How are you setting your cursor. This is how I have it in my Hyprland config:

env = HYPRCURSOR_THEME,Catppuccin-Mocha-Blue
env = HYPRCURSOR_SIZE,24

If you are only using the XCURSOR equivalents you are using the XCursor version of the cursor instead. Also for testing you can do the following to try different cursors:

hyprctl setcursor Catppuccin-Mocha-Blue 24

I am setting my Hyprcursor in that exact same way. Here is a crude example showcasing the Hyprcursor (left), and the XCursor (right), both sized at 24. The difference may seem subtle from the screenshot, but the Hyprcursor is noticeably duller than the XCursor counterpart when viewed from my monitor.

comp

Just for completeness, here is how I had my cursor set for both the Hyprcursor and the XCursor.

env = HYPRCURSOR_THEME,Catppuccin-Mocha-Dark-Cursors
env = HYPRCURSOR_SIZE,24
env = XCURSOR_THEME,Catppuccin-Mocha-Dark-Cursors
env = XCURSOR_SIZE,24

@trowgundam
Copy link
Contributor Author

For the record you need to remove the "-Cursors" part from the HYPRCURSOR_THEME environment variable. Hyprland, or hpyrcursor in general, actually does match against the name in the manifest only and not the folder name. So it's probably not finding the theme to use. Also at such small sizes it's really hard to tell the different between the cursors. Likely what is happening is a difference between the inkscape rendered PNG that is used for the Xcursor version and Hyprland's rendering of the SVG. The difference in luminance could be attributed to just being a thinner border, thus looking "dimmer". It would be better to compare it a larger size like at 128. You would likely never use the cursor at this size, but it would highlight the difference best since xcursor does not provide cursors larger than 64, so Xcursor would appear pixelated while hyprcursor should still appear smooth, It would actually be even better to use something like 200 or some other value that isn't a multiple of any of the Xcursor provide sizes that way some interpolation has to occur to get the requested size.

@sgoudham
Copy link
Contributor

sgoudham commented May 26, 2024

Sorry to derail this PR a little bit but I'm currently carrying out some changes to parallelise the build process so that it doesn't take forever to build and possibly reworking the makefiles/build scripts.

I'm looking to merge my changes first so I can be confident/happy with the time it takes to build and the release process. Hope that's okay if this PR sits for a while longer while I figure out the repository 👍

@sgoudham sgoudham changed the title Add hyprcurors support feat: support hyprcursor May 26, 2024
@trowgundam
Copy link
Contributor Author

That's fine. I'm not sure how much more you could do. As you might have guessed, I've run the build for this several times, and it fully utilizes all the cores on my System. I've monitored the process through htop, and I can see multiple inkscape processes. My CPU also sits pegged at 100% on all cores (a Ryzen 7 7840HS, so 16 cores) with only a nominal dip between each cursor variant.

@sgoudham
Copy link
Contributor

When I say parallelise, I'm honestly just talking about adding support for building each zip individually so you can run them as bash background jobs and we can take advantage of GitHub Actions matrices to build in parallel too.

As far as I can see, we're not writing to the same shared directory (it's always at least namespaced by flavour / accent) so we should have no conflicts.

I'm more focused on other matters in the organisation at the minute but will definitely look to tidy this repository up so it's not a huge pain to build / develop on.

Sorry that it tanks your CPU and thank you for continuing to work on it!

@sgoudham
Copy link
Contributor

sgoudham commented May 27, 2024

So... exciting news! I realised that we were calling inkscape around 8800 times as a separate process per generation of one single flavour. I've been able to generate the commands to pass to its new shell mode which exponentially improves the batch processing by generating all the pngs in one inkscape process.

I've now been able to fully build one single flavour in under a minute on my machine!

image

Hoping to land this work in #19 soon!

@isabelroses
Copy link
Member

I use the Nix package so I am somewhat curious how support will be there once and if this is merged

Something similar to https://github.com/NotAShelf/hyprcursor-catppuccin, but on https://github.com/catppuccin/nix I assume.

@trowgundam
Copy link
Contributor Author

I have reimplemented this based on the recent changes to the repository with PR #23.

@sgoudham
Copy link
Contributor

Hi, meant to comment on this PR - sorry for giving you additional work!
Closing as superceded by PR mentioned in above comment

@sgoudham sgoudham closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants