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

Add manual crop #33

Merged
merged 10 commits into from
Jun 14, 2016
Merged

Add manual crop #33

merged 10 commits into from
Jun 14, 2016

Conversation

wata-gh
Copy link
Contributor

@wata-gh wata-gh commented Jun 3, 2016

cf. #11

Manual croppingを追加しました。
Auto croppingとは違いcropの順番を変えたほうが効率が良いので、先にcroppingしてからリサイズするようにしています。
フラグを持つことでAuto croppingの場合は順序を変えていません。

err = engine.Crop(coodinates.CropWidth, coodinates.CropHeight, coodinates.WidthOffset, coodinates.HeightOffset)
if err != nil {
return &ResizeResult{err: logger.ErrorDebug(err)}
}
Copy link
Member

Choose a reason for hiding this comment

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

[IMO] 条件複雑になっちゃうので、全部クロップを最初に倒したほうが良さそうですけどどうでしょう?

@wata-gh
Copy link
Contributor Author

wata-gh commented Jun 6, 2016

@TakatoshiMaeda パラメータを分割しました。

@@ -90,8 +102,60 @@ func ParseGeometry(geo string) (*Geometry, error) {
pos = GEO_AUTO_CROP
if cond[1] == "true" {
needsAutoCrop = true
} else if cond[1] == "manual" {
Copy link
Member

Choose a reason for hiding this comment

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

manualにするのであれば、trueautoにして対にしたいですね。
mc=trueのような形式にしなかったのは理由ありますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

特に無いですね。
別フラグにします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

修正しました!
ed28a69

Copy link
Member

Choose a reason for hiding this comment

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

👍

engine.SetSizeHint(coodinates.ResizeWidth, coodinates.ResizeHeight)
if option.NeedsManualCrop {
// open with source image size.
engine.SetSizeHint(option.SizeHintWidth, option.SizeHintHeight)
Copy link
Member

Choose a reason for hiding this comment

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

この分岐ってなんで必要なんでしたっけ

Copy link
Member

Choose a reason for hiding this comment

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

[IMO]

一段浅いifのblockで

if option.HasSizeHint() && !option.NeedsManualCrop {
  calculator.SetImageSize(option.SizeHintWidth, option.SizeHintHeight)
  coodinates = calculator.Calc(option)
  engine.SetSizeHint(coodinates.ResizeWidth, coodinates.ResizeHeight)
}

こうするのが意図がわかりやすいかなと思いました

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど。
修正しました。
305790f

@takatoshi-maeda
Copy link
Member

👍

@takatoshi-maeda takatoshi-maeda merged commit 9b552f4 into tokubai:master Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants