-
Notifications
You must be signed in to change notification settings - Fork 84
Conversation
There was a problem hiding this 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 added some comments that I think will make this change a lot smaller and simpler. As far as testing, this is an isolated feature of the CLI, not the library, so I'm not really concerned with it being tested. 🙂
cli/src/main.rs
Outdated
if args.tweaks.enable_flips_and_rotates { | ||
let mut transformed_examples: Vec<_> = vec![]; | ||
for example in &examples { | ||
let mut new_examples: Vec<_> = vec![ | ||
example.clone().with_transformations(vec![Transformation::FlipH]).clone(), | ||
example.clone().with_transformations(vec![Transformation::Rot90]).clone(), | ||
example.clone().with_transformations(vec![Transformation::FlipH, Transformation::Rot90]).clone(), | ||
example.clone().with_transformations(vec![Transformation::Rot180]).clone(), | ||
example.clone().with_transformations(vec![Transformation::FlipH, Transformation::Rot180]).clone(), | ||
example.clone().with_transformations(vec![Transformation::Rot270]).clone(), | ||
example.clone().with_transformations(vec![Transformation::FlipH, Transformation::Rot270]).clone(), | ||
]; | ||
transformed_examples.append(&mut new_examples); | ||
} | ||
examples.append(&mut transformed_examples); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole part can be made a lot simpler by just calling load_image for the example and doing the transforms here. That way, you don't actually need to change anything in the lib crate at all, as the CLI is the only part that needs to know that the example inputs need to be transformed, and ImageSource already supports just passing a raw image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 2d0c83b
cli/src/main.rs
Outdated
/// Flips and rotates traning images | ||
#[structopt(long = "flips-and-rotates")] | ||
enable_flips_and_rotates: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider this "flips and rotates mode" different enough to warrant being in its own subcommand, as I don't really see it making sense to also add inpainting and tiling, as you kind of get a tiling effect anyway (in a way).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider this "flips and rotates mode" different enough to warrant being in its own subcommand
Just to double check (since you mention subcommand), you mean something like?
#[derive(StructOpt)]
enum Subcommand {
/// Transfers the style from an example onto a target guide
#[structopt(name = "transfer-style")]
TransferStyle(TransferStyle),
/// Generates a new image from 1 or more examples
#[structopt(name = "generate")]
Generate(Generate),
/// Generates a new image from 1 or more flipped and rotated examples
#[structopt(name = "flips-and-rotates")]
FlipsAndRotates(FlipsAndRotates),
}
or do you mean something else, like moving enable_flips_and_rotates
to struct Generate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, like that. Not sure about the naming though, but it can be changed later if anyone comes up with a clearer and/or shorter name. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 5fdab54
Oh, and FYI, you might want to install https://github.com/rust-lang/rustfmt#on-the-stable-toolchain and https://github.com/rust-lang/rust-clippy#step-2-install-clippy since we require both of those to pass in CI, and clippy will also help you out writing Rust especially if you are new. 🙂 |
Sorry for the delay, I think I got everything now. There's a |
Yah no worries, published crates can only use other published crates, so that failure is expected due to how this repo currently works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, sorry for the late review but this seems to address the original issue nicely, thanks!
Adds a
--flips-and-rotates
flag. Closes #30Here's the original output of
cargo run --release -- --out /tmp/out.png generate imgs/tom.jpg
:And here's with
--flips-and-rotates
:I'm pretty new with Rust and I'm not sure how to write tests for this/validate that the output is correct. Sorry for any dumb mistake 🙇.