Skip to content

Conversation

@atomisu0312
Copy link
Contributor

描画に関するStateを状態管理ライブラリに移管(#257)する前準備的な扱いのタスクとなります。
配列のサイズが可変にする必要性が感じられなかったので、配列のサイズを最大値で固定しました。

以下、メリットとデメリットとなります。

メリット:

  • State配列の大きさをnumPixelに応じて動的に変える必要がなくなる分、処理が軽量になる。
  • 常にState配列のサイズが固定されるので、その分の管理が楽になる

デメリット:
Stateに割くメモリのサイズが修正前より大きくなる。
とはいえ、MB単位のサイズで変わるものではない(はず)です

以下、変更点のまとめとなります。

  • State2次元配列のサイズをMAX_NUM_PIXEL x MAX_NUM_PIXELに固定。
  • 永続化するJsonにencodeする際の処理を変更
  • resizeTwoDimensionalArrayを抹消(ここでしか使っていないので、deprecatedではなく普通に消しました)

#261

描画に関するStateを状態管理ライブラリに移管(#257)する前準備的な扱いのタスクとなります。
配列のサイズが可変にする必要性が感じられなかったので、配列のサイズを最大値で固定しました。

以下、メリットとデメリットとなります。

メリット:
- State配列の大きさをnumPixelに応じて動的に変える必要がなくなる分、処理が軽量になる。
-  常にState配列のサイズが固定されるので、その分の管理が楽になる

デメリット:
Stateに割くメモリのサイズが修正前より大きくなる。
とはいえ、MB単位のサイズで変わるものではない(はず)です

以下、変更点のまとめとなります。
- State2次元配列のサイズをMAX_NUM_PIXEL x MAX_NUM_PIXELに固定。
- 永続化するJsonにencodeする際の処理を変更
- resizeTwoDimensionalArrayを抹消(ここでしか使っていないので、deprecatedではなく普通に消しました)
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the size of pixel and circle object state arrays to their maximum values as preparation for migrating drawing-related state to a state management library. The change eliminates the need for dynamic array resizing based on numPixel, simplifying state management at the cost of slightly increased memory usage.

Key changes:

  • Fixed state 2D array sizes to MAX_NUM_PIXEL x MAX_NUM_PIXEL instead of dynamic sizing
  • Updated JSON encoding logic to filter out elements beyond numPixel boundaries during persistence
  • Removed the resizeTwoDimensionalArray utility function that is no longer needed

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
front/src/lib/event/venueedit/encode.ts Added boundary filtering logic to prevent persistence of elements beyond numPixel range
front/src/lib/event/venueedit/decode.ts Changed array initialization to use MAX_NUM_PIXEL instead of dynamic sizing
front/src/lib/event/venueedit/arrayControl.ts Completely removed the resizeTwoDimensionalArray utility function
front/src/hooks/event/venueedit/useDrawingState.ts Removed calls to resizeTwoDimensionalArray and its import

Comment on lines 31 to 41
const flatten = matrix.flatMap((row, y) =>
row.map((value, x) => ({ value, x, y }))
.filter(({ value }) => value !== null) as MatrixElement[]
)

const colorMap = flatten.reduce((acc, element) => {
const color = element?.value?.toString()
if (element.x >= numPixel || element.y >= numPixel) {
// ピクセル数を超える範囲にあるものは永続化しない。
return acc
}
Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The boundary check is performed after flattening and filtering the entire matrix. Consider moving this check earlier in the process by filtering during the flatMap operation to avoid processing elements that will be discarded.

Suggested change
const flatten = matrix.flatMap((row, y) =>
row.map((value, x) => ({ value, x, y }))
.filter(({ value }) => value !== null) as MatrixElement[]
)
const colorMap = flatten.reduce((acc, element) => {
const color = element?.value?.toString()
if (element.x >= numPixel || element.y >= numPixel) {
// ピクセル数を超える範囲にあるものは永続化しない。
return acc
}
const flatten = matrix.flatMap((row, y) =>
row.map((value, x) => ({ value, x, y }))
.filter(({ value, x, y }) => value !== null && x < numPixel && y < numPixel) as MatrixElement[]
)
const colorMap = flatten.reduce((acc, element) => {
const color = element?.value?.toString()

Copilot uses AI. Check for mistakes.
単純に条件を追加するのではなく、既存の条件に追加する形で、num_Pixelに関する処理を追記
@YasunariIguchi
Copy link
Contributor

ありがとうございます!

@YasunariIguchi YasunariIguchi merged commit 9e2bae2 into develop Jul 30, 2025
1 check passed
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.

3 participants