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

Add option to take screenshot #43

Closed
wants to merge 3 commits into from

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Jan 10, 2024

@syamajala had asked about a screenshot capability in StanfordLegion/legion#1551 (comment)

This PR adds a screenshot feature to the profiler frontend. Currently it is activated by <ctrl>+Space but open to any alternative suggestions (personally prefer keyboard option to a button, but it would be simple to add one). This approach borrows liberally from the egui plot screenshot example.

Currently just saves the entire window (I think the added context is nice) but image region could be clipped as desired.

foo

cc @manopapad @elliottslaughter

@@ -18,8 +18,11 @@ eframe = { version = "0.22.0", default-features = false, features = [
"default_fonts", # Embed the default egui fonts.
"glow", # Use the glow rendering backend. Alternative: "wgpu".
"persistence", # Enable restoring app state when restarting the app.
"__screenshot", # __screenshot is so we can dump a screenshot using EFRAME_SCREENSHOT_TO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what all the official egui screenshot examples require 🤷

if let Some(screenshot) = frame.screenshot() {
self.screenshot = Some(screenshot);
}
}
Copy link
Contributor Author

@bryevdv bryevdv Jan 10, 2024

Choose a reason for hiding this comment

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

Seems a bit clunky to me, but ref:

/// Request the current frame's pixel data. Needs to be retrieved by calling [`Frame::screenshot`]
/// during [`App::post_rendering`].

src/app.rs Outdated
}

if let Some(screenshot) = self.screenshot.take() {
if let Some(mut path) = rfd::FileDialog::new().save_file() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently always defaults to "Untitled" (.png) but we could set a a different default filename if desired

Choose a reason for hiding this comment

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

Maybe default to the name-of-the-profile.png?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or Screenshot_YYYY-MM-DD-HH-MM-SS.png, or something like that.

I agree a more informative default name is helpful.

Copy link
Contributor Author

@bryevdv bryevdv Jan 11, 2024

Choose a reason for hiding this comment

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

Giver there can be an arbitrary number of source locators I opted for something like @elliottslaughter's datetime suggestion 3be2ff1

Screenshot 2024-01-11 at 12 40 52

I actually wanted Legion Prof 2024-01-11 12:41:27 but annoyingly rfd converts colons to forward slashes 🤷)

@lightsighter
Copy link

Currently it is activated by +Space but open to any alternative suggestions (personally prefer keyboard option to a button, but it would be simple to add one).

Is there a reason not to use the print screen key? 😉

@bryevdv
Copy link
Contributor Author

bryevdv commented Jan 11, 2024

Is there a reason not to use the print screen key?

There is no print screen key on Mac keyboards, e.g. and also no egui::Key code for print screen that I could see.

@lightsighter
Copy link

There is no print screen key on Mac keyboards,

On MAC: print-screen == Ctrl+Cmd+3

also no egui::Key code for print screen that I could see

That's unfortunate.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jan 11, 2024

On MAC: print-screen == Ctrl+Cmd+3

Also Shift Command 4, Shift Command 4 + Space, Shift Command 5... We could make any of them work, but I am not sure there is one obvious choice

@elliottslaughter
Copy link
Contributor

Definitely do not overload the Mac print screen keys. They will work fine as long as we don't mess with them.

I assume Seshu is not on a Mac, so what he needs is for this to be available on other platforms.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jan 11, 2024

@elliottslaughter CI issues seem to be a version incompat

error: package `cfg-expr v0.15.6` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.67.1

@bryevdv
Copy link
Contributor Author

bryevdv commented Jan 16, 2024

@elliottslaughter any thoughts on the CI issue?

@elliottslaughter
Copy link
Contributor

I bumped the Rust version, can you merge and try again? (Note: please throw away your Cargo.lock changes to avoid downgrading anything by accident.)

@bryevdv
Copy link
Contributor Author

bryevdv commented Feb 15, 2024

Unfortunately after merge master (pushed) and regenerating the lockfile, the branch no longer builds for me for any obvious reason at a glance. But I'm about to be OOTO so it will be late next week or the week after before I can get back to look more closely.

@elliottslaughter
Copy link
Contributor

Something you're doing seems to depend on GTK:

The system library `gdk-3.0` required by crate `gdk-sys` was not found.

I'm not sure we want to add additional (non-Rust) dependencies? Is there a way to avoid pulling this in?

@bryevdv
Copy link
Contributor Author

bryevdv commented Feb 15, 2024

no idea offhand, this PR just follows the short and simple official egui screenshot example code here GDK is an image library, though, so perhaps the documented screenshot capability relies on it it somehow.

Edit: That isn't the build error I am seeing, though, FWIW (or maybe they are downstream errors, I have not looked into yet)

base ❯ cargo build --all-features
    Updating crates.io index
   Compiling legion_prof_viewer v0.1.0 (/Users/bryan/work/prof-viewer)
error[E0407]: method `post_rendering` is not a member of trait `eframe::App`
    --> src/app.rs:2440:5
     |
2440 | /     fn post_rendering(&mut self, _screen_size_px: [u32; 2], frame: &eframe::Frame) {
2441 | |         // this is inspired by the Egui screenshot example
2442 | |         if let Some(screenshot) = frame.screenshot() {
2443 | |             self.screenshot = Some(screenshot);
2444 | |         }
2445 | |     }
     | |_____^ not a member of trait `eframe::App`

error[E0599]: no method named `screenshot` found for reference `&eframe::Frame` in the current scope
    --> src/app.rs:2442:41
     |
2442 |         if let Some(screenshot) = frame.screenshot() {
     |                                         ^^^^^^^^^^ method not found in `&Frame`

error[E0599]: no method named `request_screenshot` found for mutable reference `&mut eframe::Frame` in the current scope
    --> src/app.rs:2459:19
     |
2459 |             frame.request_screenshot();
     |                   ^^^^^^^^^^^^^^^^^^ method not found in `&mut Frame`

@bryevdv bryevdv closed this Sep 22, 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.

3 participants