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

fix: 色変更関数の変数名などをわかりやすいようにした #60

Merged
merged 5 commits into from Sep 29, 2022

Conversation

GoRuGoo
Copy link
Collaborator

@GoRuGoo GoRuGoo commented Sep 28, 2022

Overview

わかりやすいようにしたつもりです

Issue number

Close #

How to check the revision

Points for Review

Remarks

Comment on lines 193 to 202

@classmethod
def get_choice_images_paths(cls):
"""Get the paths to the images for the choices."""
icon_file = cls.ICON_PATH.replace(cls.CHOICE_IMAGES_DIR_PATH, "").replace("/", "")
return [
os.path.join(cls.CHOICE_IMAGES_DIR_PATH, file)
for file in os.listdir(cls.CHOICE_IMAGES_DIR_PATH)
if file.endswith(".png") and not file == icon_file
]
Copy link
Owner

Choose a reason for hiding this comment

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

もとの場所のままでお願いします

@@ -142,23 +142,30 @@ def set_skin_color(self, hue: float, sat: float, val: float) -> None:
def _convert_image_color(
self, image: np.ndarray, hue: float, sat: float, val: float, include_alpha_ch: bool
) -> np.ndarray:
"""アルファチャンネル付きのRGB=(0,0,255)の画像の色を、指定したHSV数値の色に変更する.
"""アルファチャンネル付きのHSV=(240,255,255)の画像の色を、指定したHSV数値の色に変更する.
Copy link
Owner

Choose a reason for hiding this comment

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

元のコメントのままで良いんじゃないかな?

hue (_type_): HSVのHueの数値
sat (_type_): HSVのSaturationの数値
val (_type_): HSVのValueの数値
image (_type_): H:240 S:255 V:255で塗りつぶしたアルファチャンネルを含むメイク素材、1024x1024
Copy link
Owner

Choose a reason for hiding this comment

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

ditto

Comment on lines 149 to 151
hue (_type_): HSVのHueの数値(0~255)
sat (_type_): HSVのSaturationの数値(0~255)
val (_type_): HSVのValueの数値(0~255)
Copy link
Owner

Choose a reason for hiding this comment

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

0~255で制限するならvalidationつけてもいいかも
範囲外でもおかしくならないならばそのままでも大丈夫
(たしかこの前口頭レビューしたところ)

Comment on lines 159 to 161
DETECT_HUE_VAL = 120
DETECT_SAT_VAL = 255
NOT_DETECT_VAL_VAL = 0
Copy link
Owner

Choose a reason for hiding this comment

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

VAL_VAL になっちゃうから、最後のVALは不要です

Copy link
Owner

Choose a reason for hiding this comment

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

RGB=(0,0,255) ⇔ HSV=(120, 255, ?) っていう認識でいいかな?
それならば、

B255_HUE = 120
B255_SAT = 255
NO_CAHNGE_VAL = 0

みたいな感じ??

認識違ったらごめん

image_hsv[:, :, 0] = np.where(image_hsv[:, :, 0] == 120, hue / 2, image_hsv[:, :, 0])
image_hsv[:, :, 1] = np.where(image_hsv[:, :, 1] == 255, sat, image_hsv[:, :, 1])
image_hsv[:, :, 2] = np.where(image_hsv[:, :, 2] != 0, (val * (image_hsv[:, :, 2] / 255)), image_hsv[:, :, 2])
if not ((0.0 <= hue <= 255) & (0.0 <= sat <= 255) & (0.0 <= val <= 255)):
Copy link
Owner

Choose a reason for hiding this comment

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

あんどは記号ではなくローマ字じゃないとダメな気がする

image_hsv[:, :, 1] = np.where(image_hsv[:, :, 1] == 255, sat, image_hsv[:, :, 1])
image_hsv[:, :, 2] = np.where(image_hsv[:, :, 2] != 0, (val * (image_hsv[:, :, 2] / 255)), image_hsv[:, :, 2])
if not ((0.0 <= hue <= 255) & (0.0 <= sat <= 255) & (0.0 <= val <= 255)):
raise ValueError("Defferent range of values")
Copy link
Owner

Choose a reason for hiding this comment

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

つづりが違う気がします

Copy link
Owner

Choose a reason for hiding this comment

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

1個ずつ判定した方が、どの値が範囲外かわかるからいい気がする

@kathmandu777
Copy link
Owner

@GoRuGoo
俺だったらこんな感じにするかな

@GoRuGoo
Copy link
Collaborator Author

GoRuGoo commented Sep 28, 2022

最初のdocstring、ももさんがSとVが255であることを知らずにまちがえて素材作って、エラー出てますって動かしたときに連絡きたのでRGBよりHSVの方が良いと思うの自分だけですかね?
@kathmandu777

@kathmandu777
Copy link
Owner

B255ってh120のs255ではない?

@GoRuGoo
Copy link
Collaborator Author

GoRuGoo commented Sep 28, 2022

一応そうなんですけど、B255で伝えて間違ったものが帰ってきたので…
けど欲しいのはbgraなんだよなぁ…
両方書くのがいいのでしょうか

@kathmandu777
Copy link
Owner

プログラム的には、B255を変換しているので、そのままで行きましょう

@kathmandu777 kathmandu777 merged commit bf4e610 into master Sep 29, 2022
@kathmandu777 kathmandu777 deleted the fix_color_change branch September 29, 2022 14:43
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.

None yet

2 participants