背景塗りつぶしにPatBltを使う Part2

[berryzplus]

目的

背景塗りつぶしに使うAPIを変更します。

#765 の展開として FillRect 関数の独自版(内部的にPatBltを使用)を用意し、全数を置き換えます。

注意

• sakura_core\dlg\CDlgAbout.cpp にある FillRect は 背景塗りつぶしにPatBltを使う Part1 #765 で修正予定です。
• sakura_core\prop\CPropComToolbar.cpp にある FillRect は利用状況の都合で ExtTextOut に変更します。

変更で導入する独自関数

この変更により新たに独自関数を導入します。
既存FillRectとなるべく近い使い勝手となるように3オーバーロードを用意します。
速度影響をなるべく少なくしたいので全関数inline指定です。

FillRectとの主な相違点は実行速度と第二引数ですが、だいたい似た感じに使えるように作っています。

int WINAPI FillRect( HDC hDC, RECT* lpRc, HBRUSH hBrush );
inline bool MyFillRect( const HDC hDC, const RECT &rc, HBRUSH hBrush ) noexcept;
inline bool MyFillRect( const HDC hDC, const RECT &rc, int sysColor ) noexcept;
inline bool MyFillRect( const HDC hDC, const RECT &rc, COLORREF color ) noexcept;

関数名	最後の引数	説明
MyFillRect	HBRUSH	指定したブラシをSelectObjectしてPatBltします。基本形です。
MyFillRect	int	指定したカラーインデックスでGetSysColorBrushしてMyFillRectします。使い終わったブラシを削除する必要はないのでそのまま戻ります。FillRectの第3引数の仕様に合わせ、カラーインデックスを受けつけるようにしています。
MyFillRect	COLORREF	指定した色でCreateSolidBrushしてMyFillRectします。使い終わったブラシは削除してから戻ります。サクラエディタではRGB指定で色を渡すケースが多いので独自実装を用意します。

PRの評価方法について

このPRの目的は、FillRect関数の置換です。
実際の速度改善は目的ではないので、置換が正しく行われているか（明らかな置き換えミスがないか）に納得がいけば問題なしとしてよいと考えております。

[beru]

細かくは見てませんが、ざっと動作確認した感じでは問題無いように思います。

[beru]

MyFillRect 関数群のエラー処理に問題があるように見えます。

[beru]

問題無いと思います。

[ds14050]

相変わらず目的を説明しませんね。FillRect を FillRectA に置換します、FillRectA を FillRectB に置換します、というような、横方向へスライドするだけの「目的」だけが示されて、一体どこへ向かっているのか、進んでいるつもりなのか、まるで説明しませんね。

[ds14050]

一つ前のコメントは #765 (comment) より前に書かれました。現在の理解は一つ前のコメント時点より多少進んでいますが、この PR の概要に不備があるとの意見は同じです。

[berryzplus]

レビューありがとうございます。いれてしまいます。

[berryzplus]

補足しておくと「体裁が整っていない」はマージ阻害要因じゃないと思っています。

コメントかコードかどちらかで意図を汲み取れるなら、それを持って進めてよいと思っています。

[ds14050]

不誠実な人間ですね。誤解の元だ。

[berryzplus]

GitHub でやったらあかん発言ですな。

[ds14050]

言葉を狩られても痛くも痒くもありません。自分は @berryzplus さんの言動を批判しています。それが根拠のない名誉毀損にあたるというのなら真摯に受け止めます。

[berryzplus]

過去発言見てもらうと分かると思います。

内容の説明を求めたことはありますが、本文を訂正しろとかドキュメントを書けと要求したことはありません。

逆に、明らかに説明足りてないコミットをいいんじゃない？と何度か通してきたはずです。説明ちゃんと書けよーといちゃもんつけながら。

やってること変わってないんだから不誠実とか言われるのは心外。

アフォなんじゃないか？と言われたら
何故分かった！
と叫んどきますが。

[ds14050]

@berryzplus さんの過去の発言を以て berryzplus さんの PR を認めることができないと指摘しました。

[ds14050]

コメントかコードかどちらかで意図を汲み取れるなら

目的のない PR に対して、その手段であるコードを吟味する理由がありません。その手段の妥当性、目的の達成を判断する基準が存在しません。目的さえ明確なら手段はどうとでもなります。奇しくもその点は同意できるようです。

[ds14050]

わざとなのか読解力・言語能力に問題があるのか、ハンロンの剃刀を適用すればまずは無能であると判断すべきようですが、@berryzplus さんは他人(ds14050)の発言に正面から受け答えすることがあまりありません。文章と内容が理解できないから応じられないのでしょうか。そこに悪意を見出せば不誠実だという評価につながります。さっきは応じるつもりがないと開き直られたので、こちらも不誠実だと断じました。

[berryzplus]

過去発言見て「今回だけ違う」と思う読解力ってどうなの？
コミットコード見て、目的を判断できない読解力ってどうなの？

他人を批判すれば、それはそのまま自分に跳ね返ってくるものだと思います。
まぁ、ぼくは無能で構わないですけど 😄

わざとなのか読解力・言語能力に問題があるのか、ハンロンの剃刀を適用すればまずは無能であると判断すべきようですが、@berryzplus さんは他人(ds14050)の発言に正面から受け答えすることがあまりありません。

コミュニケーションはお互いの歩み寄りによって成り立つものです。
ぼくの理解力が足りないのか、君の言語能力が足りないのか、どっちでもいいけど、相手を責めてもなにも進展はないと思います。

文章と内容が理解できないから応じられないのでしょうか。

PR本文を書きなおしてほしいの意味だと判断して「不要と考えます」と答えました。
自分がレビューする立場で、似たケースにあったとき修正を要求するかどうかを考えました。
おそらく、自分はPR本文の修正を要求しないです。
前に一度ありましたが「no description provided」でも良いです。

「方向性がおかしくなったら止めてください」とお願いしてましたけど、
PR本文については本体と合わせて目的を理解できるようになっていればいいと思います。

そこに悪意を見出せば不誠実だという評価につながります。

悪意はありません。

今回の要望を受け入れないことと @ds14050 さんに今後も突っ込みをお願いしたいこととの間に関連はありません。

[ds14050]

これは part1 への最初のコメントです。「速度を目的とした変更なら、アプリケーションに適用した場合のベンチマークが知りたいです。」

仮定を元にして意見を述べています。@berryzplus さんが明言しないために発生したこのコミュニケーションコストを以後もずっと払い続けていました。

berryzplus さんの発言です。「実動作に合わせたベンチマークは用意してません。速度改善効果の確認がしたいわけじゃないからです。」 これに対する自分の解釈を訂正する発言も berryzplus さんからは出てきませんでした。この発言を元にした意見は完全なロスでした。

コメントかコードかどちらかで意図を汲み取れるなら、それを持って進めてよいと思っています。

意見を聞く耳はないでしょうね。書きません。

悪意はありません。

行動が改まらなければ同じことです。

[ds14050]

過去発言見て「今回だけ違う」と思う読解力ってどうなの？
コミットコード見て、目的を判断できない読解力ってどうなの？

他人を批判すれば、それはそのまま自分に跳ね返ってくるものだと思います。
まぁ、ぼくは無能で構わないですけど 😄

読解力がないとしてもそれを責めてはいません。昔の Issue ですが自分の考えを理解してもらうために #116 (comment) に至るまでに何度も何度も何度も自分自身はとうに得ていた理解を繰り返しました。

同じように、berryzplus さんに自分を理解させる努力を求めています。明言すること、誤解を正すこと、「コメントかコードかどちらかで意図を汲み取れるなら」と甘えて結果的に読解できない人間を置き去りにすることがないこと。これが歩み寄りではありませんか。

まぁ、ぼくは無能で構わないですけど 😄

ブラフですよ。謙虚に教えを請う姿勢が見られません。発言を真実だとして行動と考え合わせると ds14050 は berryzplus さん以上の無能で学ぶところなどないと見られているということです。

この一連の PR を通して自分は berryzplus さんにでたらめを改めて理を通すことを求めてきました。でたらめに見えるんです。ただ手を動かして何かした気になりたいだけの人間に見えるんです。

---

あらためてこの PR の概要を読み直しましたが、ますます混乱してきました。
MyFillRect の説明コメントには「 * @brief API関数FillRectの高速版(色指定用)」とありますが、PR の説明文では「このPRの目的は、FillRect関数の置換です。実際の速度改善は目的ではないので、置換が正しく行われているか（明らかな置き換えミスがないか）に納得がいけば問題なしとしてよいと考えております。」

これは矛盾です。FillRect を MyFillRect に置き換える理由が説明できません。

既存FillRectとなるべく近い使い勝手となるように3オーバーロードを用意します。

３つのオーバーロードにより色を指定する型を選ばず使い勝手の向上を目指すのが目的というのでもなく、「既存FillRectとなるべく近い使い勝手となるように」と同じ水準を目指しているだけです。

誰の読解力に問題があり、誰の言語能力に問題があるのか追及することに意味はありません。しかし PR を出した当人である berryzplus さんがこれらの疑問について説明することを拒むなら、やはり不誠実な行動だと自分は判断します。

[beru]

この PR は #765 の展開なので、やっている事は塗りつぶし用のGDI関数を直接呼ぶ形からオーバーロードされた MyFillRect 関数群を経由する形に変えるものですね。

自分が気になるのは「ここからどうするんだろう？」という事です。
作業の流れは #767 に書かれており、整理PRをこの後作成するとなっていますが、そうする事によって最終的に達成出来るのは何か？ですね。

berryzplus さんの元の目論見 では、ExtTextOut より PatBlt の方が処理速度が速いという情報があるので検証資材を作成して、そうらしい事が確認出来たので入れ替え、という流れだと思います。

自分の予想ですが、結局は ExtTextOut から PatBlt に置き換えて本当に塗りつぶしに掛かる時間が減るのかはかなりグレーで、条件依存、環境依存、ではないかと思います。

ただ仮に処理速度を上げる目標をこのアプローチで達成出来なかったとしても無意味と捉えるのではなく、より成功に近づいた、と考えるのが建設的で良いと思います。

[ds14050]

この PR は #765 の展開なので、

#765 を開けばわかりますが、part1, part2 を通して #765 への参照は @beru さんが行ったものが最初です。それがどういうことかはもうくどくど書きたくありません。たぶん自分に慈愛の心が足りなかったんです。

より成功に近づいた、と考えるのが建設的で良いと思います。

目指す「成功」と複数の価値基準について。

「書かれなかったコード」に軍配を上げたがるのが自分の傾向です。機械に優しくではなく人間に優しくです。だからこそどれだけ機械の負荷が減っているのかにも厳しい目を向けます。本当に負荷が減っているかどうかではありません。コードを費やすことによって失うものがあり、自分が重視する価値からは離れていきます。手を加えただけ前に後ろに進んでいるという単純なことではないのです。

サクラエディタのコードは玉石混淆なので、本質的な無駄を取り除くことにより人間に優しいコードと機械を酷使しないコードがあちこちで簡単に両立できるはずです。優先順位を考えてもらいたいですし、これと同様の最適化を進めるにも、先人からくどいほど伝えられてきた鉄則がいくつかあります。

• 「早すぎる最適化は諸悪の根源」について - Qiita
• 最適化 (情報工学) - Wikipedia

@berryzplus さんはそれらを踏襲していません。守破離の守をとうに卒業して遙かな高みへ行ってしまったからその行動を裏打ちする正しさが自分にはわからないのだと思います。周りのレベルに合わせる優しさを期待したいですね。

ただ仮に処理速度を上げる目標をこのアプローチで達成出来なかったとしても無意味と捉えるのではなく、より成功に近づいた、と考えるのが建設的で良いと思います。

確実に前に進んでいることを確かめながらであれば、作業の積み重ねも無駄ではないでしょうね。これは皮肉ではなく要求です。

[ds14050]

自己レスです。

#765 への言及は #767 への間違いでした。

これは矛盾です。FillRect を MyFillRect に置き換える理由が説明できません。

(自分で)説明します。「このPRの目的は、FillRect関数の置換です。実際の速度改善は目的ではないので」というのは厳密には「このPRの目的は速度の改善です。FillRect の代わりに PatBlt を使ったオリジナルの MyFillRect を使うようにします」という意味です。

これが理解できた今初めてこの PR の内容が頭に入ってきました。

#765 を見れば FillRect はどういう場合にも遅いらしいので、置き換えに意味があると考える理由はあります。ExtTextOut の置き換えとは事情が違うみたいです。

---

ことここに至ればこれまでのことはくだらない言葉の行き違いだったとわかります。

ペンと紙が置かれたテーブルと係の人を前にして「お名前宜しいですか？」と声をかけられたときに、(宜しいですか？ 良い？ 悪い？ 名前が？ それで？) とフリーズしてしまった人間ならではのつまづきだったと思います。係の人が言い直してくれるまで固まっていました。

@berryzplus さんに謝りはしません。不備は不備ですし、対応にも誠実さが欠けました。しかし自分の無理・矛盾への対応力のなさもちょっとどうかと思います。「ダブルバインド - Wikipedia」に対応できない人工知能みたいだ。

[beru]

#765 を開けばわかりますが、part1, part2 を通して #765 への参照は @beru さんが行ったものが最初です。それがどういうことかはもうくどくど書きたくありません。たぶん自分に慈愛の心が足りなかったんです。

PRの #765 と #766 の最初のコメントに #767 への参照が無いから分かりづらかったって事でしょうか？

PRの体裁が整っていなくてレビューしにくいと感じたら、それはコメントで伝えればよいのではないかと思います。あとこのままではマージして欲しくないと思ったら、Review changes から Request changes でコメント付きで変更要求すれば良いかなと思います。マージされるまで多少時間は有ったと思いますので静観してないで早めにアクションを取るのが良いと思います。マージされてから「なっとらん」って言うより、やれやれと思うかもしれませんが事前に指摘する方が良いのではないかなと。

目指す「成功」と複数の価値基準について。

「書かれなかったコード」に軍配を上げたがるのが自分の傾向です。機械に優しくではなく人間に優しくです。だからこそどれだけ機械の負荷が減っているのかにも厳しい目を向けます。本当に負荷が減っているかどうかではありません。コードを費やすことによって失うものがあり、自分が重視する価値からは離れていきます。手を加えただけ前に後ろに進んでいるという単純なことではないのです。

手書きのロジックの量を増やすとその分メンテコストが増大するので簡素な記述になるに越したことは無いですね。

速度の最適化するとしたら、計測した数字の上でもちゃんと高速化のパーセンテージが大きくて、あと体感的にも処理時間が短くなったというのが感じられないと、アドバンテージは無いかもしれないですね。

サクラエディタのコードは玉石混淆なので、本質的な無駄を取り除くことにより人間に優しいコードと機械を酷使しないコードがあちこちで簡単に両立できるはずです。優先順位を考えてもらいたいですし、これと同様の最適化を進めるにも、先人からくどいほど伝えられてきた鉄則がいくつかあります。

• 「早すぎる最適化は諸悪の根源」について - Qiita
• 最適化 (情報工学) - Wikipedia

簡単に両立できるのかはやってみないと自分にはわかりませんが、全体のコード量も多いしそんなに楽じゃないと思います。能力のある人であれば朝飯前なのかもしれませんが。

優先順位に関しては主観によるバイアスが大きいと思うので共通認識を持つのは難しいんじゃないかと思います。というのを言い訳にして全員があらぬ方向を向いてて協調しないのもまずいですが…。

リンクの紹介ありがとうございます。「早過ぎる最適化は諸悪の根源」っていうのは昔指摘された事あります。「ﾜﾙｶｺｲｲ!」という事で構わないんじゃないか…と心の中で思いましたが口にはしませんでした。まぁミクロな最適化ばっかしてて全体のコードのリファクタしないのは技術的負債を積み重ねてるのと紙一重なのはその通りなので、楽しいからと言ってチューニングばかりしてたらまずい事は確かですね。まぁ脳内麻薬との格闘ですね。

確実に前に進んでいることを確かめながらであれば、作業の積み重ねも無駄ではないでしょうね。これは皮肉ではなく要求です。

定量的に効果を確認するのと、的外れな方向に進んでないか見直しが必要って事でしょうか？
ベンチマーク関係の整備が必要ですね。

しかし同じ塗りつぶしを行うAPIが大きな速度差が出ないように実装されていれば良かったですね。そうも出来ない裏事情とかあるのかもしれませんが、ドキュメントで解説されてないし分かりづらいです…。

[berryzplus]

どこにコメントすべきか迷いましたがここに書きます。

この次のアクションは、整理コミットの作成だと思っています。
タイトル「背景塗りつぶしにPatBltを使う」の #767 の続き作業です。
続き作業と分かるようにしたいので名称は ～ Part3 とするつもりです。
今日はちょっと時間がもう遅いので、PRは明日以降になります。

PR作るときに issue #767 のタイトルを変更して追加の説明を入れようかなと思ってます。

今回やりたかったことは、塗りつぶしに使うAPIの見直しです。
「正確さ」を気にするなら「PatBltにする」じゃなくて「再考する」のが目的だと言えます。
そのあたりは @ds14050 さんが指摘してきた通りだと思ってます。
ぼくが「やー」と言ってるのはマージ済みPRの本文変更なのです。
レビュー指摘として要求されたなら変更したと思います。

根拠としてあげてきた「速度」の話は、さきに回答したとおり主因ではありません。
追加した関数「MyFillRect」は「RectをFillするMy関数」であると一目で分かります。
中身を見るまでもなく「たぶんそうだ」と実装を推測できる名前だと思います。
分かりやすさの感じ方は人それぞれなので、あえて前面には出しませんでした。

ぼく個人としては、分かりやすさの改善はとても大事なことだと思っています。

[beru]

MyFillRect っていう関数名ってあんまり好きじゃないです。「お前のものは俺のもの」的なジャイアニズムを感じる…とかいうと言い方がきついかもしれないので別の言い方をすると「俺様用しおり」的な、、、あれいっしょか？

じゃあ他に良い名前があるのかというと FillRectEx とか、FillRect2 とかも考えられますが、なんか微妙ですね。辞書で違う言語を調べて MonFillRect, MioFillRect とかにするのも良くないだろうし。

みんながみんな好き勝手な名前を付けだしてそのうち早い者勝ちルールに嫌気がさした人がGUIDを付け始めたら末期症状ですね。

まぁもちょっと描画ルーチンのバリエーションが増えたらネーミングスキームを考えて適用するのが良いのかな…。数が増えたら体系的にまとめていきたいですね。

[beru]

根拠としてあげてきた「速度」の話は、さきに回答したとおり主因ではありません。
追加した関数「MyFillRect」は「RectをFillするMy関数」であると一目で分かります。
中身を見るまでもなく「たぶんそうだ」と実装を推測できる名前だと思います。
分かりやすさの感じ方は人それぞれなので、あえて前面には出しませんでした。

ぼく個人としては、分かりやすさの改善はとても大事なことだと思っています。

「速度」が主因ではないとして、元のAPI の FillRect だと分かりやすさには何か問題があったんでしょうか？

なお速度に関していえば、FillRect が本当にこのアプリの実仕様のケースで遅いのかは確認した方が良かったと思います。「デスクトップウィンドウのDCではなくて、自プロセスのウィンドウのDCの場合にどうなのか？」をです。

まぁ MyFillRect 関数群の層が作られた事で処理時間の確認とかもしやすくなっていると思うので、ぼちぼち進めていきましょう。

[ds14050]

細かなところだけ。

PRの体裁が整っていなくてレビューしにくいと感じたら、それはコメントで伝えればよいのではないかと思います。

「レビューしにくい」ではなくレビューができない状態でした。リンクされていない issue をレビューの対象に含めることは不可能です。書かれていないために仮定した目的は否定されました。そして berryzplus さんとは話が通じないことがわかっているため、改善を求めることを諦めてしまいました。

今も berryzplus さんの追加の書き込みによって疑問が膨らんでいます。

• 「再考する」では情報が足りません。どこに問題意識を持って再考する必要があると考えたのか、まで共有してもらわなければ。
• 「速度が主因ではない」について。そこにしか価値はないと思うのでここで混ぜっ返す意味がわかりません。
• MyFillRect のわかりやすさについて。実装が仮定できる Windows API の FillRect のわかりやすさと、中でどんな変なことをしているか確かめずにはいられないサクラエディタのコードベースに含まれる MyFillRect のわかりやすさでは、Windows API に軍配を上げます。

(最適化の)優先順位に関しては主観によるバイアスが大きいと思うので共通認識を持つのは難しいんじゃないかと思います。

期待できる効果のオーダーが異なることから、小手先の改善よりロジックやアルゴリズムの改善を優先するべきだということです。これは共通認識でなければ困ります。サクラエディタのコードベースであればアルゴリズムだなんて大仰に構えることもなく、何でこんな無駄なことをやってるんだ、というような箇所がいくらでも見つかると思います。しかしどのレベルに取り組むかは個人の能力・嗜好によるところもあるでしょう。

[berryzplus]

名前は「嫌い」です。嫌いだけど、誤解されないこと、FillRectで検索してヒットすることの2点でこれにしました。FillRectCustomとかApiWrap::FillRectとか適切っぽい名前の候補はありました。

[berryzplus]

767のリンクに気付かなかったのはブラウザ依存ですかね？

ieとiPhoneで見た場合issueにPR番号を書くとPR側に参照リンクが表示されて見えます。

[ds14050]

ああ、逆リンクですか(初めて気がついた)>#767
PR本文の解釈エラーによりそこまでたどり着きませんでした。

[ds14050]

それに、バックリンクはトラックバックなどと同じく参照元が主体であり重きを置いていませんでした。が、手がかりではあり、「不可能」というのは間違いでしたね。