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

Fix Taking Screenshots on Wayland #84

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ProjectSynchro
Copy link

@ProjectSynchro ProjectSynchro commented Sep 11, 2024

This PR implements a new "ScreenShotUtil" class that implements a CreateScreenShot method that will take a screenshot in the typical way using a java.awt.Robot on every desktop platform short of Wayland, this returns a BufferedImage, as expected.

Due to some bugs with how Wayland screenshots are handled in Java proper: https://bugs.openjdk.org/browse/JDK-8269245

We have to call the desktop bus (dbus) to request for the compositor to take a screenshot.

The screenshot is taken as a PNG image and dbus will tell us where that image is. That image is ingested as a BufferedImage and the PNG we took is deleted from the system. The BufferedImage is returned, which is what a Robot would normally do.

I also ran into a fun edge case where writing a BufferedImage with an alpha channel (like one we might get when reading a PNG) to a JPG silently fails, which causes the my-images/ folder to not be populated as expected.

I wrote a small helper method to strip out the alpha channel to avoid this:

// Helper method to convert an image to a JPG-compatible format (no alpha channel)
private static BufferedImage convertToJpgCompatible(BufferedImage image) {
BufferedImage newImage = new BufferedImage(image.getWidth(), image.getHeight(), BufferedImage.TYPE_INT_RGB);
newImage.createGraphics().drawImage(image, 0, 0, null);
return newImage;
}

This PR pulls in the dbus-java library, which allows us to interface with dbus directly instead of talking to a potentially missing executable, which is the most portable way to do this.

This change adds approx 1MiB to the final jarfile.

@ProjectSynchro ProjectSynchro marked this pull request as ready for review September 11, 2024 01:29
@EtienneLamoureux
Copy link
Owner

I'll be moving the companion app to bundle jdk 21, but I doubt that'll fix all the issues...

@ProjectSynchro
Copy link
Author

I'll be moving the companion app to bundle jdk 21, but I doubt that'll fix all the issues...

I can double check if that ends up fixing anything, I had tried building the companion for Java 22 at one point and the issue seemed to still exist.

The main issue is the fact that the prompt for "permission" doesn't seem to work correctly as it's called asynchronously. Definitely not a bug with your code, but a bug with the platform implementation in Java itself.

I went with dbus as a good workaround as that avoids that platform code, as well as the fact that it doesn't require a prompt (it may prompt depending on the compositor's settings, so that's up to user choice)

I can fix up conflicts if you'd still like to merge this. The old Robot implementation is still used as a workaround (I tested in Windows to ensure it still works)

@EtienneLamoureux
Copy link
Owner

EtienneLamoureux commented Nov 29, 2024

We'll be trying to increasingly be moving towards the game log as source of truth, as much as possible, so it will alleviate some of that issue if the data points we wish for make it to the game log.
I'll be taking another look at this PR very soon :)

@ProjectSynchro
Copy link
Author

I can't seem to get this to build properly, so I'll mark this as a draft for now.

Feel free to comment on any changes though 😄 using the game log is definitely a more consistent solution (especially considering all the dirt that is on Pyro commodity screens)

@ProjectSynchro ProjectSynchro marked this pull request as draft November 29, 2024 20:48
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.

2 participants