Skip to content
This repository was archived by the owner on Oct 28, 2023. It is now read-only.

Add --inpaint-alpha to CLI and API changes to derive mask from channels #60

Closed

Conversation

khskarl
Copy link

@khskarl khskarl commented Oct 22, 2019

Hey, found myself in a Hacktoberfest event this Saturday and it felt like a good opportunity to try my hand on #22 :).

Changes

To derive an inpaint mask from the alpha channel I felt it would be more "idiomatic" according to the lib to do some changes on ImageSource, therefore there are some minor modifications on it.

  • Adds boolean --inpaint-alpha argument to CLI
  • New Mask enum
  • The original ImageSource is now DataSource
  • ImageSource is now a struct with a Mask and a DataSource
Usage
texture-synthesis --out=result.png --inpaint-alpha generate silly_example_with_alpha.png

Example

Example:
brick
Result:
out

(Image source: https://unsplash.com)

Checklist

  • Run tests locally
  • Run all examples locally
  • Run rustfmt
  • Run clippy

There is mild refactor around ImageSource:
- The original ImageSource is now DataSource
- The new ImageSource has a Mask and a DataSource
Copy link
Member

@Jake-Shadle Jake-Shadle 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! I've left some comments. 🙂

cli/src/main.rs Outdated
Comment on lines 142 to 144
/// Flag to extract inpaint from the example's alpha channel
#[structopt(long)]
inpaint_alpha: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I realise #22 specifically mentions the alpha channel, but might want to just name this --inpaint-channel <r, g, b, a> instead (defaulting to alpha) since you already added support for different mask channels.

Copy link
Author

Choose a reason for hiding this comment

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

Not very experienced with structupt, does it support a way to set the default value only if the flag is present?
E.g.
texture-synthesis --inpaint-channel will be texture-synthesis --inpaint-channel=a
texture-synthesis won't be texture-synthesis --inpaint-channel=a

lib/src/utils.rs Outdated
Comment on lines 122 to 123
pub(crate) fn apply_mask(original_image: &image::RgbaImage, mask: &Mask) -> image::RgbaImage {
let mut image = original_image.clone();
Copy link
Member

Choose a reason for hiding this comment

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

This should just take image by &mut, as now there is an unnecessary copy made that gets immediately discarded at the one call site.

}

impl<'a> ImageSource<'a> {
pub fn from_path(path: &'a Path) -> ImageSource<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

ImageSource already has From<AsRef<Path>> that does this.

cli/src/main.rs Outdated
@@ -239,6 +243,16 @@ fn real_main() -> Result<(), Error> {

let mut sb = Session::builder();

if args.inpaint_alpha {
Copy link
Member

Choose a reason for hiding this comment

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

--inpaint and --inpaint-channel should probably be exclusive, structopt doesn't really support that atm, so you can just check manually and error if they are both set.

@khskarl
Copy link
Author

khskarl commented Oct 23, 2019

Just finished most of the changes, but I ran into lifetime issues when trying to do ImageSource::from as in:

path => example.set_sample_method(SampleMethod::Image(ImageSource::from(
                    &std::path::Path::new(path),
                ))),

Error message:

error[E0716]: temporary value dropped while borrowed
   --> cli/src/main.rs:238:22
    |
229 |             if i == examples.len() {
    |                     -------- borrow later used here
...
238 |                     &std::path::Path::new(path),
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
239 |                 ))),
    |                   - temporary value is freed at the end of this statement
    |
    = note: consider using a `let` binding to create a longer lived value

I don't have a good grasp on lifetimes, so I will have to dig into that later as I have to get back to my thesis for now.

@arirawr arirawr requested a review from Jake-Shadle October 31, 2019 11:17
Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature!

@Jake-Shadle Jake-Shadle mentioned this pull request Nov 18, 2019
@Jake-Shadle
Copy link
Member

Closing this in favor of #67 which has all the changes and does some minor cleanup, again, thanks so much for this PR, really sorry it took so long!

@khskarl
Copy link
Author

khskarl commented Nov 18, 2019

No problem, happy to contribute :).

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants