-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: remove animation canvas entirely | move to animation package #1457
base: main
Are you sure you want to change the base?
Changes from 37 commits
83f7242
0f18c42
87468b7
17e808a
4d8e762
b82ca2d
59f5128
dd937de
47d5fda
f3278bf
0971a29
eed0549
062a304
706ae6c
f9b4942
59cf36e
3b98ea1
bbbdbff
cd60324
b04c009
2327320
00527fc
4aea092
071d12b
a26c9b9
0fd5248
957ccb2
045a732
1d9366b
227040e
d0f913e
77020ad
b27d0af
b1240eb
c2dfff0
268acc6
61c6999
260dfb6
7aea559
ab1c5c9
6c89292
59cc0ad
58dce49
40f5e4a
9f743b1
267fbd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +0,0 @@ | ||
./public/assets/glApp.js | ||
Large diffs are not rendered by default.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,8 +2,8 @@ import { useCallback } from 'react'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useTranslation } from 'react-i18next'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import styled from 'styled-components'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { ToggleSwitch } from '@app/components/elements/ToggleSwitch.tsx'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useAppConfigStore } from '@app/store/useAppConfigStore'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useUIStore } from '@app/store/useUIStore'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { setVisualMode, useAppConfigStore } from '@app/store/useAppConfigStore'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { sidebarTowerOffset, TOWER_CANVAS_ID, useUIStore } from '@app/store/useUIStore'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { Typography } from '@app/components/elements/Typography'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SettingsGroup, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -13,23 +13,51 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SettingsGroupWrapper, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from '@app/containers/floating/Settings/components/SettingsGroup.styles'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { loadTowerAnimation, removeTowerAnimation, setAnimationState } from '@tari-project/tari-tower'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const ErrorTypography = styled(Typography)(({ theme }) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
color: theme.palette.error.main, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function VisualMode() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const visualMode = useAppConfigStore((s) => s.visual_mode); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const setVisualMode = useAppConfigStore((s) => s.setVisualMode); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const visualModeToggleLoading = useAppConfigStore((s) => s.visualModeToggleLoading); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const isWebglNotSupported = useUIStore((s) => s.isWebglNotSupported); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { t } = useTranslation('settings', { useSuspense: false }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const handleDisable = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setVisualMode(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
removeTowerAnimation({ canvasId: TOWER_CANVAS_ID }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.then(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Force garbage collection to clean up WebGL context | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (window.gc) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
window.gc(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.catch((e) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
console.error('Could not disable visual mode. Error at loadTowerAnimation:', e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setVisualMode(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+28
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add timeout to prevent hanging during disable. The disable operation should have a timeout to prevent hanging if the animation cleanup takes too long. const handleDisable = useCallback(() => {
+ const timeoutId = setTimeout(() => {
+ console.error('Tower animation cleanup timed out');
+ setVisualMode(false);
+ }, 5000);
+
setVisualMode(false);
removeTowerAnimation({ canvasId: TOWER_CANVAS_ID })
.then(() => {
// Force garbage collection to clean up WebGL context
if (window.gc) {
window.gc();
}
})
.catch((e) => {
console.error('Could not disable visual mode. Error at loadTowerAnimation:', e);
setVisualMode(true);
})
+ .finally(() => {
+ clearTimeout(timeoutId);
+ });
}, []); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const handleEnable = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.then(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setVisualMode(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setAnimationState('showVisual'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.catch((e) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
console.error('Could not enable visual mode. Error at loadTowerAnimation:', e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setVisualMode(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+42
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Monitor memory usage during animation loading. Based on the PR comments about memory leaks, add memory usage monitoring during animation loading. const handleEnable = useCallback(() => {
+ const initialMemory = window.performance.memory?.usedJSHeapSize;
+
loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset })
.then(() => {
setVisualMode(true);
setAnimationState('showVisual');
+
+ // Monitor memory usage
+ const currentMemory = window.performance.memory?.usedJSHeapSize;
+ if (currentMemory && initialMemory && currentMemory - initialMemory > 500 * 1024 * 1024) {
+ console.warn('High memory usage detected after enabling visual mode');
+ }
})
.catch((e) => {
console.error('Could not enable visual mode. Error at loadTowerAnimation:', e);
setVisualMode(false);
});
}, []); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const handleSwitch = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const canvasElement = document.getElementById('canvas'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (canvasElement) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
canvasElement.style.display = visualMode ? 'none' : 'block'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (visualMode) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
handleDisable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
handleEnable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
setVisualMode(!visualMode); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, [setVisualMode, visualMode]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, [handleDisable, handleEnable, visualMode]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
54
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add debounce to prevent rapid toggling. Based on the PR comments about memory issues, rapid toggling should be prevented. +import { debounce } from 'lodash';
+
-const handleSwitch = useCallback(() => {
+const handleSwitch = useCallback(
+ debounce(() => {
if (visualMode) {
handleDisable();
} else {
handleEnable();
}
- }, [handleDisable, handleEnable, visualMode]);
+ }, 1000),
+ [handleDisable, handleEnable, visualMode],
+); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<SettingsGroupWrapper> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -41,11 +69,14 @@ function VisualMode() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{isWebglNotSupported && <ErrorTypography color="error">{t('webgl-not-supported')}</ErrorTypography>} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</SettingsGroupContent> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<SettingsGroupAction style={{ alignItems: 'center' }}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<ToggleSwitch disabled={isWebglNotSupported} checked={visualMode} onChange={handleSwitch} /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<ToggleSwitch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
disabled={visualModeToggleLoading || isWebglNotSupported} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
checked={visualMode} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onChange={() => handleSwitch()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</SettingsGroupAction> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</SettingsGroup> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</SettingsGroupWrapper> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default VisualMode; |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,57 @@ | ||
import { useAppStateStore } from '@app/store/appStateStore'; | ||
import { useMiningStore } from '@app/store/useMiningStore'; | ||
import { setAnimationState } from '@app/visuals'; | ||
import { useEffect } from 'react'; | ||
import { setAnimationState, animationStatus } from '@tari-project/tari-tower'; | ||
import { useMiningMetricsStore } from '@app/store/useMiningMetricsStore.ts'; | ||
import { useAppConfigStore } from '@app/store/useAppConfigStore.ts'; | ||
|
||
export const useUiMiningStateMachine = () => { | ||
const isMiningInitiated = useMiningStore((s) => s.miningInitiated); | ||
const isChangingMode = useMiningStore((s) => s.isChangingMode); | ||
const cpuIsMining = useMiningMetricsStore((s) => s.cpu_mining_status.is_mining); | ||
const gpuIsMining = useMiningMetricsStore((s) => s.gpu_mining_status.is_mining); | ||
const setupComplete = useAppStateStore((s) => s.setupComplete); | ||
const visualMode = useAppConfigStore((s) => s.visual_mode); | ||
const visualModeToggleLoading = useAppConfigStore((s) => s.visualModeToggleLoading); | ||
const isMining = cpuIsMining || gpuIsMining; | ||
|
||
const statusIndex = window?.glApp?.stateManager?.statusIndex; | ||
const indexTrigger = animationStatus; | ||
|
||
useEffect(() => { | ||
const status = window?.glApp?.stateManager?.status; | ||
const notStarted = !status || status == 'not-started' || status == 'stop'; | ||
let isLatestEffect = true; | ||
if (!visualMode || visualModeToggleLoading) return; | ||
const notStarted = !animationStatus || animationStatus == 'not-started'; | ||
if (isMining && notStarted) { | ||
setAnimationState('start'); | ||
// Debounce animation state changes | ||
const timer = setTimeout(() => { | ||
if (isLatestEffect) { | ||
setAnimationState('start'); | ||
} | ||
}, 300); | ||
return () => { | ||
clearTimeout(timer); | ||
isLatestEffect = false; | ||
}; | ||
} | ||
}, [statusIndex, isMining]); | ||
}, [indexTrigger, isMining, visualMode, visualModeToggleLoading]); | ||
|
||
useEffect(() => { | ||
const notStopped = window?.glApp?.stateManager?.status !== 'not-started'; | ||
let isLatestEffect = true; | ||
if (!visualMode || visualModeToggleLoading) return; | ||
const notStopped = animationStatus !== 'not-started'; | ||
const preventStop = !setupComplete || isMiningInitiated || isChangingMode; | ||
const shouldStop = !isMining && notStopped && !preventStop; | ||
if (shouldStop) { | ||
setAnimationState('stop'); | ||
// Debounce animation state changes | ||
const timer = setTimeout(() => { | ||
if (isLatestEffect) { | ||
setAnimationState('stop'); | ||
} | ||
}, 300); | ||
return () => { | ||
clearTimeout(timer); | ||
isLatestEffect = false; | ||
}; | ||
} | ||
}, [statusIndex, setupComplete, isMiningInitiated, isMining, isChangingMode]); | ||
}, [indexTrigger, setupComplete, isMiningInitiated, isMining, isChangingMode, visualMode, visualModeToggleLoading]); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Ensure proper cleanup of tower-canvas element.
Given the reported memory leak issues when toggling visual mode (memory usage increasing from 800MB to 2GB), ensure that the canvas element is properly disposed of when visual mode is disabled. Consider implementing the following:
To help diagnose the memory leak, consider running this script to analyze canvas cleanup:
🏁 Script executed:
Length of output: 3407
Potential Memory Leak Issue with the
tower-canvas
ElementThe search results show no explicit cleanup for the
tower-canvas
element, particularly for its WebGL context. To avoid memory leaks when toggling visual mode, please ensure that:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai that's the whole point of this PR 😭 - the removal of the canvas is handled in the
@tari-project/tari-tower
packageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shanimal08 I apologize for the redundant suggestion. You're absolutely right - the canvas cleanup is already being handled by the
@tari-project/tari-tower
package, which is precisely what this PR is about. Thank you for the clarification.✏️ Learnings added