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

🎉 お絵かきキャンバス実装 #8

Merged
merged 8 commits into from
May 31, 2024
Merged

🎉 お絵かきキャンバス実装 #8

merged 8 commits into from
May 31, 2024

Conversation

SSlime-s
Copy link
Member

ベースは v1 のときのやつ
ガッツリリファクタしたから原型はあんまない
塗りつぶし部分はほぼまんま

端っこで途切れないようにいい感じにした

localhost:5173/tmp2 でぐりぐりできます

@SSlime-s SSlime-s requested review from ras0q and toshi-pono May 16, 2024 13:34
@SSlime-s SSlime-s self-assigned this May 16, 2024
@SSlime-s SSlime-s force-pushed the SSlime/add-canvas branch from 99ab13f to afac853 Compare May 17, 2024 09:15
Copy link

cloudflare-workers-and-pages bot commented May 17, 2024

Deploying nascalay-v2-preview with  Cloudflare Pages  Cloudflare Pages

Latest commit: 79f2108
Status: ✅  Deploy successful!
Preview URL: https://d7a46fbd.nascalay-v2-preview.pages.dev
Branch Preview URL: https://sslime-add-canvas.nascalay-v2-preview.pages.dev

View logs

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

とりあえずfill

apps/client/app/features/artboard/bucketFill.ts Outdated Show resolved Hide resolved
apps/client/app/features/artboard/bucketFill.ts Outdated Show resolved Hide resolved
}, []);

const undoHistory = useCallback((currentData: Entry) => {
if (history.current.undoList.length === 0) return null;
Copy link
Member

Choose a reason for hiding this comment

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

実質これと同じだけどcanUndoを使うのがよさそう
他のとこも

Copy link
Member Author

Choose a reason for hiding this comment

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

こっちのほうがコストが低くて安全なので

Copy link
Member

Choose a reason for hiding this comment

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

コスト周りあまり詳しくないけど例えばcanUndo/canRedoをなくしてhistoryだけをstateで管理するのとかはどう

Copy link
Member Author

Choose a reason for hiding this comment

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

正直書いたの前だからあんまり覚えてないんだけど
history を ref で持ってるのは破壊的変更したほうがコピーのコスト削減できる & 読みやすいから state じゃなくて ref で持って破壊的変更してるはず
リレンダリング的な話だと history の途中 (最初でも最新でもないところ) でリレンダリング走らないからちょっとだけ得
変更し忘れ的な話をしたいんだと思うけど、この hooks 自体が小さいしシンプルだからこっちのほうが良くないかなと思うけど

Copy link
Member

Choose a reason for hiding this comment

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

多重管理してるのがやっぱり気になるけどちょっとでもパフォーマンスいいならこれでもいいです

Copy link
Member Author

Choose a reason for hiding this comment

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

ちょっとでもパフォーマンスがいいのは確かだよ (上にも理由書いてるけど)

Comment on lines +72 to +73
const next = undoHistory(now);
if (next === null) return;
Copy link
Member

Choose a reason for hiding this comment

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

時間的にnextなのは分かるがprevだろみたいな気もする

Copy link
Member Author

Choose a reason for hiding this comment

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

次に適用するって意味で next ね

apps/client/app/utils/unreachable.ts Show resolved Hide resolved
@SSlime-s SSlime-s force-pushed the SSlime/add-canvas branch from 40fa14c to 79f2108 Compare May 20, 2024 22:45
Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

大体見たつもり

* @returns 右端の x 座標
*/
function drawToRight(
data: Color[][],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data: Color[][],
colorMap: Color[][],

とかpixelDataとかだと苦なく読める

Copy link
Member Author

Choose a reason for hiding this comment

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

PR 出してるし前言ったけど bucketFill は修正しないね

向こうでも data 使ってるからそっちに書いてくれ

apps/client/app/utils/unreachable.ts Show resolved Hide resolved
}, []);

const undoHistory = useCallback((currentData: Entry) => {
if (history.current.undoList.length === 0) return null;
Copy link
Member

Choose a reason for hiding this comment

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

コスト周りあまり詳しくないけど例えばcanUndo/canRedoをなくしてhistoryだけをstateで管理するのとかはどう

}, []);

const undoHistory = useCallback((currentData: Entry) => {
if (history.current.undoList.length === 0) return null;
Copy link
Member

Choose a reason for hiding this comment

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

多重管理してるのがやっぱり気になるけどちょっとでもパフォーマンスいいならこれでもいいです

@SSlime-s SSlime-s merged commit c372b77 into main May 31, 2024
4 checks passed
@SSlime-s SSlime-s deleted the SSlime/add-canvas branch May 31, 2024 09:15
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