Skip to content

Support 2d image source#207

Merged
smikitky merged 18 commits intomasterfrom
support-2d-image
Dec 13, 2021
Merged

Support 2d image source#207
smikitky merged 18 commits intomasterfrom
support-2d-image

Conversation

@doramari
Copy link
Collaborator

@doramari doramari requested review from genkn and smikitky November 26, 2021 02:38
@doramari doramari marked this pull request as ready for review November 26, 2021 02:38
@smikitky
Copy link
Member

smikitky commented Nov 29, 2021

胸部単純写真などでのウィンドウ処理が想定外に遅かったので d506396 でウィンドウ処理を少しでも高速化する処理を入れました。

  • ImageData のインスタンスに直接書き込むことでメモリコピーを削減
  • ウィンドウ処理の四則演算を少しでも削減
  • Math.round や 0-255 範囲外の処理をスキップ(どうせ Uint8ClumpedArray が同じ処理をするので)

1枚の処理が 300ms → 100msくらいにはなりました(まだ思ったよりだいぶ遅いですが)

Copy link
Member

@smikitky smikitky left a comment

Choose a reason for hiding this comment

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

  • プライマリシリーズが 2D なケースを開くと、layout がとりあえず空っぽの 2x2 で始まって、メタデータを呼んだ後に 1x1 にリセットされている感じの挙動をしていますが、見た目がよくないので、metadata の呼び出しが終わってから ViewerCluster を呼び出すように変更をお願いします。
  • ラベルセレクタ下部にある "Add series" でシリーズを追加/削除した場合の動作が不安定です。恐らく 3D モードと 2D モードが混在している時に起きている気がします。SeriesInfo コンポーネントがエラーを出しており、直接的な原因は metadata が正しく渡っていないのが理由のようです。
  • カラー画像でウィンドウツールを使おうとすると、"You cannot change the window of color images" のようなダイアログが無限に出てきて操作不能になってしまいます。

@@ -0,0 +1,26 @@
import { ViewState } from 'browser';

const determineColor = (
Copy link
Member

Choose a reason for hiding this comment

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

  • 命名が曖昧すぎるので determineLineColor で。
  • viewState の型によって第3・第4引数を無視するかどうかが決まるというシグネチャは気持ち悪いです。そもそも viewState を渡す必要がないのでは。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ご指摘いただいた関数は以前に同じような判定が複数個所にあるので集約をというご指示を頂き作成した関数でしたが、今回のご指摘と合わせて加納に相談したところ、無理に集約するのはやめたほうが良いという結果になりました。各アノテーションで判定をするように戻し、コードを再度見直した上で push いたしましたのでご確認ください。

  • 判断の基準となる distance に該当する値の取得方法がアノテーションごとに異なる。
  • 判断の基準となる アノテーションのプロパティ値 の名称も、アノテーションによって異なる。
  • アノテーションによっては、線だけではなく、塗りつぶし色 の判定も必要。
  • (1) つまり無理に1つの関数にしようとすると、大量の引数が必要になり、しかも場合によっては引数にundefinedを渡したりといったコードになりそうです。
  • (2) 別の方針としては抽象クラスを間に挟み、しかもその抽象クラスは子クラスに対して判定に使用する値を返すための protected な getter の定義を強制する形となります。
  • (1),(2)ともに残念なので、現時点では概念としてシンプルになる (state: ViewState) => 必要な色 を各箇所で定義する形が一番マシだと判断しました。

Comment on lines +124 to +132
useEffect(() => {
setResults(
series.map(() => ({
metadata: undefined,
composition: undefined,
volumeLoaded: false
}))
);
}, [series]);
Copy link
Member

Choose a reason for hiding this comment

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

これは何のために存在しているのでしょうか。

ラベルセレクタの下部にある "Add Series" ボタンでシリーズを追加した場合、恐らくここで既存のデータがリセットされてしまうためにエラーが起きています。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

引数の series が変更になった時の results の初期化をしています。デバッグで確認してみましたがここはエラーの原因ではないように思います。

@doramari
Copy link
Collaborator Author

  • カラー画像でウィンドウツールを使おうとすると、"You cannot change the window of color images" のようなダイアログが無限に出てきて操作不能になってしまいます。

alert は dragHandler ではなく dragStartHandler で表示させるよう修正しました。

@doramari
Copy link
Collaborator Author

doramari commented Dec 2, 2021

  • プライマリシリーズが 2D なケースを開くと、layout がとりあえず空っぽの 2x2 で始まって、メタデータを呼んだ後に 1x1 にリセットされている感じの挙動をしていますが、見た目がよくないので、metadata の呼び出しが終わってから ViewerCluster を呼び出すように変更をお願いします。

アクティブシリーズのメタデータのロードが済んでから、ViewerGrid を描画するように修正しました。

@doramari
Copy link
Collaborator Author

doramari commented Dec 2, 2021

  • ラベルセレクタ下部にある "Add series" でシリーズを追加/削除した場合の動作が不安定です。恐らく 3D モードと 2D モードが混在している時に起きている気がします。SeriesInfo コンポーネントがエラーを出しており、直接的な原因は metadata が正しく渡っていないのが理由のようです。

master ブランチですでに発現しているエラーのようです。一度 master ブランチで "Add series" でシリーズを追加し、不安定だと感じた動作についてご確認いただけませんでしょうか?

@doramari doramari requested a review from smikitky December 2, 2021 03:31
@smikitky smikitky merged commit d14eabb into master Dec 13, 2021
@smikitky smikitky deleted the support-2d-image branch December 13, 2021 06:52
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