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

Projects
None yet
2 participants
@lazypassion
Copy link
Contributor

lazypassion 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

This comment has been minimized.

Copy link
Member

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.)

@lazypassion

This comment has been minimized.

Copy link
Contributor Author

lazypassion 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).

@lazypassion

This comment has been minimized.

Copy link
Contributor Author

lazypassion 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.

@theotherphil

This comment has been minimized.

Copy link
Member

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.

lazypassion added some commits Jan 10, 2019

@theotherphil
Copy link
Member

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.

Show resolved Hide resolved src/window.rs
Show resolved Hide resolved src/window.rs Outdated
Show resolved Hide resolved src/window.rs
Show resolved Hide resolved src/window.rs Outdated
Show resolved Hide resolved src/window.rs Outdated
Show resolved Hide resolved src/window.rs Outdated
Show resolved Hide resolved src/window.rs Outdated

lazypassion added some commits Jan 13, 2019

@lazypassion

This comment has been minimized.

Copy link
Contributor Author

lazypassion commented Jan 13, 2019

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

@theotherphil
Copy link
Member

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.

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 {

This comment has been minimized.

@theotherphil

theotherphil Jan 13, 2019

Member

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

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

This comment has been minimized.

@theotherphil

theotherphil Jan 13, 2019

Member

Same here

@theotherphil theotherphil merged commit dac159a into PistonDevelopers:master Jan 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@theotherphil

This comment has been minimized.

Copy link
Member

theotherphil commented Jan 13, 2019

Fixes #277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment