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

Adds ability to display an image in a window using sdl2 #298

Merged
merged 19 commits into from Jan 13, 2019

Conversation

@arthmis
Copy link
Contributor

@arthmis arthmis commented Jan 8, 2019

This is for #277.

This is my first commit to any repo that isn't my own, so criticism is welcome!

@theotherphil
Copy link
Contributor

@theotherphil theotherphil commented Jan 9, 2019

Thanks for this - should be a very useful feature.

I won't have time to review this properly until the weekend, but I did try it out locally and one thing that surprised me was that I could only show one image at a time - is it easy to add support for showing multiple windows at the same time? (So that e.g. I can view the input image, some intermediate, and the result side by side.)

Loading

@arthmis
Copy link
Contributor Author

@arthmis arthmis commented Jan 9, 2019

On a quick check it looks like that is possible, so I'll look into adding it. And btw, the performance of rendering on the window is pretty abysmal with optimization levels 0 and 1(basically debug mode).

Loading

@arthmis
Copy link
Contributor Author

@arthmis arthmis commented Jan 9, 2019

I would prefer to add the ability to create multiple image windows in another pull request. Having a way to display a single image is sufficient for some use cases and multiple windows satisfies another use case.

Also if you have mac testing it on that would be useful because I can only test on ubuntu and windows 10 for the time being.

Loading

@theotherphil
Copy link
Contributor

@theotherphil theotherphil commented Jan 9, 2019

Sure, that’s fine by me. I do have a Mac - I’ll check for any odd behaviour when I review this on Saturday.

Loading

Copy link
Contributor

@theotherphil theotherphil left a comment

I've mentioned a few things that needs resolving and left a couple of other comments.

It would be nice to add something to examples/ showing how to use this function, but I'm happy to merge without an example.

Loading

src/window.rs Show resolved Hide resolved
Loading
src/window.rs Outdated Show resolved Hide resolved
Loading
src/window.rs Show resolved Hide resolved
Loading
src/window.rs Outdated Show resolved Hide resolved
Loading
src/window.rs Outdated Show resolved Hide resolved
Loading
src/window.rs Outdated Show resolved Hide resolved
Loading
src/window.rs Outdated Show resolved Hide resolved
Loading
@arthmis
Copy link
Contributor Author

@arthmis arthmis commented Jan 13, 2019

I'll squash all the commits if everything looks good.

Loading

Copy link
Contributor

@theotherphil theotherphil left a comment

Looks good, thanks!

I've left a comment about another small tweak, but I'll merge as-is and change that later.

Loading

pub fn display_image(title: &str, image: &RgbaImage, window_width: u32, window_height: u32) {
const MIN_WINDOW_DIMENSION: u32 = 150;
// ensures window size is minimum size, so that image resizing calculations for the window are correct
let window_width: u32 = if window_width < MIN_WINDOW_DIMENSION {
Copy link
Contributor

@theotherphil theotherphil Jan 13, 2019

Choose a reason for hiding this comment

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

u32 implements Ord, so this can just be let window_width = window_width.max(MIN_WINDOW_DIMENSION)

Loading

} else {
window_width
};
let window_height: u32 = if window_height < MIN_WINDOW_DIMENSION {
Copy link
Contributor

@theotherphil theotherphil Jan 13, 2019

Choose a reason for hiding this comment

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

Same here

Loading

@theotherphil theotherphil merged commit dac159a into image-rs:master Jan 13, 2019
1 check passed
Loading
@theotherphil
Copy link
Contributor

@theotherphil theotherphil commented Jan 13, 2019

Fixes #277

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants