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

Add --flips-and-rotates flag #57

Merged
merged 6 commits into from
Oct 28, 2019

Conversation

JD557
Copy link
Contributor

@JD557 JD557 commented Oct 12, 2019

Adds a --flips-and-rotates flag. Closes #30

Here's the original output of cargo run --release -- --out /tmp/out.png generate imgs/tom.jpg:

original

And here's with --flips-and-rotates:

flip-rotate

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

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 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
Comment on lines 219 to 235
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);
}

Copy link
Member

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.

Copy link
Contributor Author

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
Comment on lines 121 to 123
/// Flips and rotates traning images
#[structopt(long = "flips-and-rotates")]
enable_flips_and_rotates: 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 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).

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5fdab54

@Jake-Shadle
Copy link
Member

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

@JD557 JD557 requested a review from Jake-Shadle October 25, 2019 19:37
@JD557
Copy link
Contributor Author

JD557 commented Oct 25, 2019

Sorry for the delay, I think I got everything now.

There's a cargo publish test failing, but I'm not sure how to fix it (nor if it's supposed to be failing, it seems that cargo publish is not loading the library from the local path).

@Jake-Shadle
Copy link
Member

Yah no worries, published crates can only use other published crates, so that failure is expected due to how this repo currently works.

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.

Awesome, sorry for the late review but this seems to address the original issue nicely, thanks!

@Jake-Shadle Jake-Shadle merged commit 01cf506 into EmbarkStudios:master Oct 28, 2019
@Jake-Shadle Jake-Shadle mentioned this pull request Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: CLI option to automatically add mirrorings and rotations of input image
2 participants