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.

Sign up for GitHub

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

ルーラー描画の高速化 #1067

Merged
merged 3 commits into from
Oct 6, 2019
Merged

ルーラー描画の高速化 #1067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
410e842
ルーラー描画の高速化
Oct 6, 2019
10e8913
レビュー指摘対応
beru Oct 6, 2019
1a8fb17
レビュー指摘対応
beru Oct 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 50 additions & 32 deletions sakura_core/view/CRuler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@
CRuler::CRuler(const CEditView* pEditView, const CEditDoc* pEditDoc)
: m_pEditView(pEditView)
, m_pEditDoc(pEditDoc)
, m_hFont(NULL)
, m_nRulerHeight(0)
{
m_nOldRulerDrawX = 0; // 前回描画したルーラーのキャレット位置 2002.02.25 Add By KK
m_nOldRulerWidth = 0; // 前回描画したルーラーのキャレット幅 2002.02.25 Add By KK
}

CRuler::~CRuler()
{
if (m_hFont) {
::DeleteObject( m_hFont );
}
}

//2007.08.26 kobake UNICODE用にX位置を変更
Expand Down Expand Up @@ -88,26 +93,31 @@ void CRuler::DrawRulerBg(CGraphics& gr)
CTypeSupport cRulerType(m_pEditView,COLORIDX_RULER);

// フォント設定 (ルーラー上の数字用)
LOGFONT lf;
HFONT hFont;
HFONT hFontOld;
memset_raw( &lf, 0, sizeof(lf) );
lf.lfHeight = 1 - pCommon->m_sWindow.m_nRulerHeight; // 2002/05/13 ai
lf.lfWidth = 0;
lf.lfEscapement = 0;
lf.lfOrientation = 0;
lf.lfWeight = 400;
lf.lfItalic = 0;
lf.lfUnderline = 0;
lf.lfStrikeOut = 0;
lf.lfCharSet = 0;
lf.lfOutPrecision = 3;
lf.lfClipPrecision = 2;
lf.lfQuality = 1;
lf.lfPitchAndFamily = 34;
wcscpy( lf.lfFaceName, L"Arial" );
hFont = ::CreateFontIndirect( &lf );
hFontOld = (HFONT)::SelectObject( gr, hFont );
if (m_hFont && m_nRulerHeight != pCommon->m_sWindow.m_nRulerHeight) {
::DeleteObject( m_hFont );
m_hFont = NULL;
}
if (m_hFont == NULL) {
LOGFONT lf = {0};
lf.lfHeight = 1 - pCommon->m_sWindow.m_nRulerHeight; // 2002/05/13 ai
Copy link
Contributor

Choose a reason for hiding this comment

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

lfHeight メンバに負数を渡すと、ピクセル単位のフォントサイズを指定したことになる、
というようなLOGFONT構造体の仕様があった気がします。

「1 - ルーラー高さ」はおそらく負数になるので、たぶんこれはDPI対応の対象になるような気がします。

もちろん、別件で構わないと思っとりますが、そのうちやりたい。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

共通設定のウィンドウに「ルーラーの高さ」の設定がありますが単位がドットですね。指定されたドット数の高さにするのか、それとも表示倍率に応じて調整するのか、どちらが正しい仕様なのかは謎です（そもそも明文化された仕様は無いですけど）。

#505 でもここらへんに関わる会話がありました。

ルーラーのフォントの単位をエディタのフォントと同じポイントにして、エディタのフォントサイズ変更に追従するのが挙動としては良いと思いますが、そうするとルーラーの高さを設定で持つ事との互換性が無くなると思います。ルーラーのフォントサイズが大きくなると、ルーラーの高さを超えてしまう為です。

lf.lfWidth = 0;
lf.lfEscapement = 0;
lf.lfOrientation = 0;
lf.lfWeight = 400;
lf.lfItalic = 0;
lf.lfUnderline = 0;
lf.lfStrikeOut = 0;
lf.lfCharSet = 0;
lf.lfOutPrecision = 3;
lf.lfClipPrecision = 2;
lf.lfQuality = 1;
lf.lfPitchAndFamily = 34;
wcscpy_s( lf.lfFaceName, L"Arial" );
m_hFont = ::CreateFontIndirect( &lf );
m_nRulerHeight = pCommon->m_sWindow.m_nRulerHeight;
}
assert(m_hFont != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

このassertがfailするときの挙動を、いつか真面目に考えないといかんと思っとります。
たぶん、ガチスルーでよいはず。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

基本的には起こりえないエラーについては Debug ビルドで軽くみておくぐらいで良いんじゃないかなと思います。フリーソフトのテキストエディタにミッションクリティカルを求めるのも変な話なので。

Copy link
Contributor

Choose a reason for hiding this comment

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

ガチスルーの意図

if (!m_hFont) return; //後続の処理を実行しない

です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ガチスルーとはそういう事でしたか。話題自体をスルーする事かと思ってました。

フォント作成が失敗するような状況だと return 後の処理が正常に動く保証も無いような気がしますが、エラーチェックを入れないより大分良い気がしてきました。

HFONT hFontOld = (HFONT)::SelectObject( gr, m_hFont );
Copy link
Contributor

Choose a reason for hiding this comment

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

感想です。

gr.PushMyFont(m_hFont) としても良いような気がします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

メソッド名に My が入っているのが嫌です。

::SetBkMode( gr, TRANSPARENT );

//背景塗りつぶし
Expand All @@ -126,7 +136,6 @@ void CRuler::DrawRulerBg(CGraphics& gr)
int nX = m_pEditView->GetTextArea().GetAreaLeft();
int nY = m_pEditView->GetTextArea().GetRulerHeight() - 2;

// 下線 (ルーラーと本文の境界)
berryzplus marked this conversation as resolved.
Show resolved Hide resolved
// Aug. 14, 2005 genta 折り返し幅をLayoutMgrから取得するように
// 2005.11.10 Moca 1dot足りない
CLayoutXInt nMaxLineColum = m_pEditDoc->m_cLayoutMgr.GetMaxLineLayout();
Expand All @@ -135,8 +144,6 @@ void CRuler::DrawRulerBg(CGraphics& gr)
if( nToX > m_pEditView->GetTextArea().GetAreaRight() ){
nToX = m_pEditView->GetTextArea().GetAreaRight();
}
::MoveToEx( gr, m_pEditView->GetTextArea().GetAreaLeft(), nY + 1, NULL );
::LineTo( gr, nToX, nY + 1 );

//目盛を描画
const int oneColumn = (Int)m_pEditView->GetTextMetrics().GetLayoutXDefault();
Expand All @@ -150,44 +157,55 @@ void CRuler::DrawRulerBg(CGraphics& gr)
i += CLayoutXInt(oneColumn - pxOffset); // CLayoutXInt == pixel
++keta;
}

// 目盛り線を1本ずつ描画するのではなく後述する PolyPolyline でまとめて描画を行う
const int nWidth = (Int)(m_pEditView->GetTextArea().GetRightCol() - i);
const size_t nLinesToDraw = 1 + std::min<int>((nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

この式の確からしさを検証できるドキュメントが欲しいです。

左辺の変数名は「ドキュメントが要らないコード」の要件を満たしている気がします。
右辺の数式がほぼほぼ意味不明なので判断がつかんとです。

Copy link
Contributor Author

@beru beru Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

while 文が回る回数を計算してます。

Copy link
Contributor

Choose a reason for hiding this comment

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

ええと、左辺の変数名から利用方法は理解していて、わからんのはその数式が正しいといえる根拠です。

const size_t nLinesToDraw = 1 + std::min<int>(
    (nWidth + 1 + 1 + oneColumn - 1) / oneColumn,
     nMaxLineKetas - keta + 1);
  • nLinesToDraw (縦線を引く本数＝ループの回数)　＝　1 + 以下A,B のうち小さいほうの値
    • A = （nWidth(描画範囲の桁数) + 1 + 1 + oneColumn(1桁あたりのピクセル数)）÷　oneColumn(1桁あたりのピクセル数)
    • B = nMaxLineKetas(折り返し桁数) - keta(テキストエリアの左端桁数を1桁あたりのピクセル数で割ったもの) + 1

日本語にして見ると なんでやねん！ の嵐です。

とりあえず、「既存と同じです」なら「ま、いっか」となるんですけど、なんで？のもっともらしい理由づけが欲しいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

元の実装の記述の意味合いや厳密性については考えていませんが、while 文での扱いと同じ結果になるように合わせた式にしています。

Copy link
Contributor Author

@beru beru Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

std::min の左側の 1 + は、下線 (ルーラーと本文の境界) の分です。

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

Choose a reason for hiding this comment

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

もちょっと細かく解説した方が良いでしょうか？

冗談でなく、本当に理解できてないです。

改めてコードを眺めた感じの見解が以下です。

  • nWidth + 1　＝　描画領域の右端桁位置　－　描画領域の左端桁位置　＝　描画範囲の桁数
    • + 1 は見切れ分を考慮した保険的な増分だと思っています
  • + 1(2個目のやつ)　＝　ルーラーの区切り線（コメント中は「下線」と表現されている）を引くために必要な値。
  • oneColumn - 1　＝　謎の値。動作を見る限り、oneColumn == 1 っぽいので無視してよさげ。
  • / oneColumn　＝　謎の値。動作を見る限り、oneColumn == 1 っぽいので無視してよさげ。

この見解で合ってるとすると、スジが通ってると思います。
ただ、oneColumn - 1と/ oneColumnは不要なので分かりやすさを担保するために削除したいっす。

Copy link
Contributor

Choose a reason for hiding this comment

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

ただ、oneColumn - 1と/ oneColumnは不要なので分かりやすさを担保するために削除したいっす。

このコメント取消。
害はないと思うので CI ビルドの完了を待って approve 予定です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const size_t nLinesToDraw = 1 + std::min<int>((nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);

は

nLinesToDraw（何本描画するか）= 
  1（下線(ルーラーと本文の境界)の1本分）
    + 
  後述する A と B のうちの小さい値

で、A と B から小さい値を選ぶ理由は while 文の条件式で && が有って両方の条件を満たさないとループが中断されるからです。

A

nWidth = m_pEditView->GetTextArea().GetRightCol() - i

なので、

A = (m_pEditView->GetTextArea().GetRightCol() - i + 1 + 1 + oneColumn - 1) / oneColumn

ですが、nWidth を別変数にしたのは CLayoutInt を int に暗黙的な変換が出来なくてコンパイルエラーが起きたためです（両方とも Int にキャストすれば回避出来るのを知らなかった）。

さて、while 文の条件式は下記のようになっていて、

i <= m_pEditView->GetTextArea().GetRightCol() + 1

ループ内では

i  += oneColumn

となっています。なので変数 i に oneColumn を何回足したら m_pEditView->GetTextArea().GetRightCol() + 1 を超えた値になるか、を考えます。

この場合に除算が使えます。余りが出た場合に +1 する為に 割る数 - 1 を足してから割るという慣用的な記述を使っています。

+ 1 が何故2つ有るのかについては、元々の条件式にある + 1 の分と 2個目の + 1 については元々の条件式で <= が使われているからです。

元々の条件式に +1 が有るのは何かの保険の為でしょうか？そこについては理解してません。

B

B = 折り返し桁数 - 目盛りを描く最初の桁数 + 1

+ 1 するのは、while 文の条件式で <= としているからです。

auto& apt = m_apt;
auto& asz = m_asz;
apt.resize(nLinesToDraw * 2);
asz.resize(nLinesToDraw, 2);
// 下線 (ルーラーと本文の境界)
apt[0] = POINT{m_pEditView->GetTextArea().GetAreaLeft(), nY + 1};
apt[1] = POINT{nToX, nY + 1};
size_t idx = 1;
while(i <= m_pEditView->GetTextArea().GetRightCol() + 1 && keta <= nMaxLineKetas)
{
apt[idx * 2 + 0] = POINT{nX, nY};
//ルーラー終端の区切り(大)
if( keta == nMaxLineKetas ){
::MoveToEx( gr, nX, nY, NULL );
::LineTo( gr, nX, 0 );
apt[idx * 2 + 1] = POINT{nX, 0};
}
//10目盛おきの区切り(大)と数字
else if( 0 == keta % 10 ){
wchar_t szColumn[32];
::MoveToEx( gr, nX, nY, NULL );
::LineTo( gr, nX, 0 );
apt[idx * 2 + 1] = POINT{nX, 0};
_itow( ((Int)keta) / 10, szColumn, 10 );
::TextOut( gr, nX + 2 + 0, -1 + 0, szColumn, wcslen( szColumn ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここって TextOut を複数回呼び出す代わりに PolyTextOut で一括で行うようにしたら処理負荷って下がるんでしょうか？ちょっと実験してみます。

Copy link
Contributor

Choose a reason for hiding this comment

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

そればっかりはやって見ないと分からん感じです。
Poly系の関数はデバイスが該当描画処理を標準装備している場合に効果を発揮するはずです。
複数の線分はイケる可能性が高いですが、テキストはどうか分からんですw

}
//5目盛おきの区切り(中)
else if( 0 == keta % 5 ){
::MoveToEx( gr, nX, nY, NULL );
::LineTo( gr, nX, nY - 6 );
apt[idx * 2 + 1] = POINT{nX, nY - 6};
}
//毎目盛の区切り(小)
else{
::MoveToEx( gr, nX, nY, NULL );
::LineTo( gr, nX, nY - 3 );
apt[idx * 2 + 1] = POINT{nX, nY - 3};
}
++idx;
assert(idx <= nLinesToDraw);

nX += dx;
i += oneColumn;
keta++;
}
::PolyPolyline(gr, &apt[0], &asz[0], (DWORD)nLinesToDraw);
Copy link
Contributor

Choose a reason for hiding this comment

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

感想です。

趣味ですが、最後の引数で渡す値は asz.size() と一致していたほうが分かりやすい気がします。
std::vector は確保済み配列サイズと有効データ個数を別に管理できます。

reserveで確保サイズを決定し、
処理の冒頭で resize(0) して push_back していく構成にすれば、累積した｛点，点｝の追加を取り違えるリスクを排除できます。

たぶん、PolyPolyLineの呼出直前に線分の数を計算するのが有効なはず。

	asz.assign(nLinesToDraw, 2);

で、nLinesToDraw の実態は apt.size() / 2 だと思うので、こうなるはず。

	asz.assign(apt.size() / 2, 2);
    ::PolyPolyline(gr, apt.data(), asz.data(), static_cast<DWORD>(asz.size()));

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

Choose a reason for hiding this comment

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

出たとこ勝負で push_back する方式のメリットは、
処理前に正確なメモリ使用量の見積もりを行う必要がないことにあると思います。

ルーラーで表示できる桁目盛りの数なんて高が知れてるので、全部動的に追加確保しても問題ないような気もします。まぁ、このコメントは感想なので、具体的になにか対応してほしいわけでもなく、指摘として言ってみてるだけなんで気にしないでください 😄

Copy link
Contributor Author

@beru beru Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

人によっては1行80桁の制限を頑なに守り続けるかもしれませんが、将来的にモニタの解像度が異常に上がって頭髪の平均密度を越す勢いでルータールーラーの桁目盛りが描画される時代が来るかもしれません。


//色戻す
gr.PopTextForeColor();
gr.PopPen();

//フォント戻す
::SelectObject( gr, hFontOld );
berryzplus marked this conversation as resolved.
Show resolved Hide resolved
::DeleteObject( hFont );
}

/*! ルーラー描画
Expand Down
5 changes: 5 additions & 0 deletions sakura_core/view/CRuler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ class CRuler{
bool m_bRedrawRuler; // ルーラー全体を描き直す時 = true 2002.02.25 Add By KK
int m_nOldRulerDrawX; // 前回描画したルーラーのキャレット位置 2002.02.25 Add By KK 2007.08.26 kobake 名前変更
int m_nOldRulerWidth; // 前回描画したルーラーのキャレット幅 2002.02.25 Add By KK 2007.08.26 kobake 名前変更

HFONT m_hFont; // ルーラー上の数字用フォント
int m_nRulerHeight;
std::vector<POINT> m_apt;
berryzplus marked this conversation as resolved.
Show resolved Hide resolved
std::vector<DWORD> m_asz;
};

#endif /* SAKURA_CRULER_CF213704_1CF6_427E_AD78_D628D2D1F9029_H_ */
Expand Down