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

Jump to bottom

feat: 499 better memory management #606

Merged
merged 12 commits into from
Apr 24, 2024
Merged

feat: 499 better memory management #606

merged 12 commits into from
Apr 24, 2024

Conversation

alvarosabu
Copy link
Member

@alvarosabu alvarosabu commented Mar 29, 2024 •
edited
Loading

Hopefully closes #449

  • Recursive removal of nodes dispose (GPU) of all three disposable properties and removes refs (CPU) if the node is not primitive
  • If it is a primitive, the user needs to handle the disposal manually by using a dispose function

Todo

  • Docs

@alvarosabu alvarosabu requested a review from andretchen0 March 29, 2024 11:54
@alvarosabu alvarosabu mentioned this pull request Mar 29, 2024
Closed
@alvarosabu alvarosabu marked this pull request as draft March 29, 2024 11:54
@andretchen0
Copy link
Contributor

Sure thing! I'll have a look.

Tests?

I feel like the answer to "Should this be disposed?" is hard to explain. And that means the implementation will be easy to break in the future.

Tests would probably help both in explaining the functionality to maintainers and in ensuring future changes don't accidentally break the functionality.

At first glance, I don't see any tests about resource disposal. Would you like me to add some tests?

@alvarosabu
Copy link
Member Author

Before the tests @andretchen0 , I'm struggling to dispose the whole scene on the unmount hook of the TresCanvas when changing a route, the GPU is clear but not the CPU memory allocation 🥲. I will push the latest code I used to try to make it work

@alvarosabu alvarosabu added dx p3-significant High-priority enhancement (priority) labels Mar 31, 2024
@andretchen0
Copy link
Contributor

Before the tests @andretchen0 , I'm struggling to dispose the whole scene on the unmount hook of the TresCanvas when changing a route, the GPU is clear but not the CPU memory allocation 🥲. I will push the latest code I used to try to make it work

Sure thing.

Can you walk me through the steps you're taking – in the Chrome dev tools? – so that we're looking at the same data?

@alvarosabu
Copy link
Member Author

Hi @andretchen0 here is a video with the reproduction steps

video-memory-management.mp4

@alvarosabu alvarosabu self-assigned this Apr 1, 2024
@alvarosabu
Copy link
Member Author

I think I found the reason @andretchen0, the refs kept on the CPU are entirely related to the devtools. This doesn't occur in production build.

@alvarosabu alvarosabu marked this pull request as ready for review April 1, 2024 15:30
@andretchen0
Copy link
Contributor

I think I found the reason @andretchen0, the refs kept on the CPU are entirely related to the devtools. This doesn't occur in production build.

I came here to post this. Lol. 👍

@alvarosabu
Copy link
Member Author

Hey buddy @andretchen0 can you take a look to this one, I think I would prefer to tackle the test in the #631 task

@alvarosabu alvarosabu merged commit e98ca6d into v4 Apr 24, 2024
3 checks passed
@alvarosabu alvarosabu deleted the feat/499-better-memory-management branch April 24, 2024 09:08
alvarosabu added a commit that referenced this pull request May 30, 2024
@alvarosabu @andretchen0
@garrlker @Tinoooo @JaimeTorrealba
feat: release v4 (#490)
1ba17ee 
* feat: 474 vue chrome devtools plugin (#479)

* feat: vue chrome devtools

* feat: editable scenes from devtools

* chore(lint): fix lint errors

* feat: highlight material

* chore(lint): fix

* chore: release v4.0.0-next.0

* feat: update to three `v160` and vue `v3.4` (#488)

* fix(types): added `Object3DEventMap` to `Object3D` generics for point event handling (#491)

* feat: 140 on demand rendering (#497)

* feat: conditional rendering

* chore: remove subscribe system

* feat: on-demand automatic invalidation with prop changes

* feat: invalidate once first when is `renderMode !== 'always'`

* docs: performance page, on-demand rendering

* chore: fix windowsize issue

* chore(lint): fix maximum line length issues

* feat: invalidate on-demand on window resize

* feat: add advance method for manual mode

* feat: fix manual first render with advance

* docs: performance manual mode

* docs: add badge with version

* chore: correct typos and PR suggestions

* chore: tell dont ask fix

* feat: render state instead of internal

* feat: remove default camera warning (#499)

* feat: remove annoying defautl camera warning

* chore: remove logWarning

* feat: 492 set tone mapping default to acesfilmictonemapping (#498)

* feat: set ACESFilmicToneMapping as default toneMapping

* chore: usage of nullish coealescing operator instead of ternaries

* feat: 516 localstate for custom renderer node instances instead of userdata (#522)

* feat: conditional rendering

* chore: remove subscribe system

* feat: on-demand automatic invalidation with prop changes

* feat: invalidate once first when is `renderMode !== 'always'`

* docs: performance page, on-demand rendering

* chore: fix windowsize issue

* chore(lint): fix maximum line length issues

* feat: invalidate on-demand on window resize

* feat: add advance method for manual mode

* feat: fix manual first render with advance

* docs: performance manual mode

* docs: add badge with version

* chore: correct typos and PR suggestions

* chore: tell dont ask fix

* feat: render state instead of internal

* feat: add __tres local state to nodeOps instances

* feat: add context to root on instances localstate

* feat: camera registration ops from node local state ctx

* feat: event handling registration from localState of nodes

* feature: disposable flag on node localstate

* feat: remove userData from types

* chore: remove unused import

* fix(test): fake localstate `.__tres` on tests

* fix(types): fix nodeOps instances localstate type

* fix: camera aspect

* Update orthographic camera aspect when screen size updates
* Give user a "manual" flag to keep Tres from updating camera

* feat: 503 conditional rendering of primitives (#514)

* feat(nodeOps): switch instance logic for reactive `object` prop

* chore: playground primitives with models

* chore: fix linter

* chore: fix tests and linters, primitive object is now reactive

* chore: refactor instance swaping logic to overwrite set and copy properties

* chore: tests

* chore: remove console.log

* chore: remove unused import watch

* feat: add primitive conditional to patch object prop

* fix: `nodeOps` is now a function (#579)

* fix: `nodeOps` is now a function

* chore(test): updated tests for `nodeOps`

* chore: next package json version

* chore: release v4.0.0-next.1

* fix: refactor nodeOps to return methods at the end of the function (#602)

* fix: refactor nodeOps to return methods at the end of the function

* chore: fix lint

* chore: internal playground organisation (#601)

* chore: new internal playground org and testing pages

* chore: fix lint

* chore: better styling of playground landing page

* chore: lint

* chore: deps update

* chore: internal primitive model test playground

* chore: fix lint

* chore: release v4.0.0-next.2

* chore: misc routes

* fix: do not change pierced props case (#608)

* chore: lint fix

* chore: problem with package version

* chore: fix lint

* chore: rebuild pnpm-lock

* test(nodeOps): clean up tests

* test(nodeOps): organize tests

* test: add coverage plugin

* test: add coverage to package.json script

* test(nodeOps): improve test coverage

* feat: devtools renderer improvements (#614)

* feat: renderer programs when selecting scene on devtools

* feat: renderer.info

* chore: fix lint

* docs: devtools update

* chore: fix lint issues

* feat(events)!: pointerevents manager and state (#529)

* new file:   playground/src/components/Box.vue
	new file:   playground/src/pages/raycaster/Propogation.vue
	  * Started work on interactive Event Propogation playground example
	modified:   src/components/TresCanvas.vue
	  * Import and use `useEventStore`
	  * defineEmits for all expected pointer events so we may emit propogated events off of the canvasa
	modified:   src/composables/index.ts
	new file:   src/composables/useEventStore/index.ts
	  * Started work on an event store. I'm not sure this counts as a store just yet
	  * Wired up majority of pointer events
	  * Added event propogation
	  * Does not require using userData scene props or nodeOps for registering objects to scene
	modified:   src/composables/useRaycaster/index.ts
	  * Added new event listeners to power newly supported pointer events. We now check whole scene/children when calling intersectObjects.
	  * Created new EventHooks for new events
	  * Added `forceUpdate` function that allows for pointer-move events to work without mouth movement (good for when camera is moving but mouse is not)

	modified:   src/core/nodeOps.ts
	  * Added supported events to array so they don't get received as props
	  * (temporarily) unhook current pointer event solution to iterate on useEventStore
	modified:   src/utils/index.ts
	  * Added Camel-to-kebab case util

* Support multiple event listeners, add support for .stop event modifier

* Set stopProgation variable to false by default, whoops

* fix typo

* fix: remove `createGlobalState` from `useEventStore`, allowing events to work while multiple TresCanvas' are being used

* fix(perf): remove extraneous intersectObjects/getIntersects calls by moving intersects into a ref that is updated on pointer-move

* chore(lint): fix lint issues

* feat: enhance events manager to include duplicates checking, pointer-missed support, and forced updating

Per file changelog:
	modified:   playground/src/components/Box.vue
	  * Added a pointer-missed handler for testing
	modified:   playground/src/pages/TheBasic.vue
	  * uses forceUpdate from EventManager to fire events even when the mouse hasn't moved
	modified:   playground/src/pages/raycaster/Propagation.vue
	  * Didn't mean to undo the lint changes, adds a pointer-missed event on the canvas 		for extra testing
	modified:   src/components/TresCanvas.vue
	  * Adds `pointer-missed` as possible event for canvas emits
	modified:   src/composables/index.ts
	  * Update export
	deleted:    src/composables/useEventStore/index.ts
	  * Rename `useEventStore` to `useTresEventManager`
	modified:   src/composables/useRaycaster/index.ts
	  * Check for empty intersects on hit test, wire up pointerMissed events eventHook
	  * Fix forceUpdate to call onPointerMove instead of triggering an EventHook
	modified:   src/composables/useTresContextProvider/index.ts
	  * Add TresEventManager type
	new file:   src/composables/useTresEventManager/index.ts
	  * add onPointerMissed
	  * create (de)registerPointerMissedObj methods so we can track objects in the scene listening to this event
	  * Note: These are passed to nodeOps via TresContext
	  * Implement duplicates checking for eventPropogation
	modified:   src/core/nodeOps.ts
	  * register/deregister pointerMissed objects

* chore: lint

* docs: new event docs

* chore: fix lint

* feat: enhance event object details and use in Box example to change material color. Add ability to force event system updates even when mouse hasn't moved. Enhance pointer-enter/leave events. Update types

  Box.vue
    * Added pointer-missed handler
    * set the materials flash color using the object coming off of the event instead of a ref
  UseRaycaster
    * Flesh out event details to include
      * all mouse event properties
      * intersections
      * tres camera
      * camera raycaster
      * source event
      * mouse position delta
      * stopPropagating stub
      * and unprojectedPoint (this needs work, cant get the math to work)
  UseTresContextProvider
    * Add TresEventManager type to TresContext
  useTresEventManager
    * Add forceUpdate method to allow apps to force an event system update even when the mouse hasnt moved
    * Add pointerMissed event
    * Properly implement pointer-enter/pointer-leave events
      * Before now, pointer-enter | leave were only called on first object in intersection, now we execute the events for all entered/left objects
    * Use stopPropagating property included on event object

* chore: lint

* chore: fix lint issues

---------

Co-authored-by: alvarosabu <alvaro.saburido@gmail.com>

* feat: 499 better memory management (#606)

* chore: memory management playground

* feat: recursively free cpu and gpu memory allocation on remove

* chore: clumsy attempt to dispose on unmount

* chore: lint fix

* feat: remove scene root on disposal

* chore: fix lint

* docs: added disposal guide on `performance` docs

* chore: fix lint

* chore: type issues (#663)

* fix: fix some internal types

* chore: fix linters

* fix: typescript issues on event manager

* chore: release v4.0.0-rc.0

* fix: make on* callbacks settable (#672)

* fix: make on- callbacks settable

* test: test setting not calling

* feat: 633 use loop  (#673)

* feat: createRenderLoop unique to context

* feat: onLoop returns current state

* feat: ensuring callback excecution with index order

* feat: take control of render loop logic

* docs: updated composable docs

* feat: change error to deprecation warning towards v5

* chore: add link to new composable docs on deprecation warning

* chore: remove depcreation warning of existing useRenderLoop

* feat: `useFrame` and `useRender` instead of `onLoop`

* chore: fix lint

* feat: applied useFrame to directives

* chore: fix lint

* feat: `useUpdate` instead of `useFrame` and useRender pausing.

* chore: testing fbo

* feat: reserve index 1 for late-updates

* chore: fix lint

* feat: useLoop composable for the win

* chore: change onLoop name for register

* chore: unit tests for loop

* chore: change order for registration to make index optional

* chore: fix lint

* feat: pauseRender and resumeRender

* docs: useLoop guide

* docs: updated basic animations recipe to `useLoop`

* docs: correct pause render methods on docs

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* chore: refactor subscribers to `priorityEventHooks`

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* feat: just return `off` on the loop registration methods

* docs: update docs to add `off` unregister callback method

* feat: remove `v-rotate`

* docs: added context warning for `v-always-look-at`

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* Update docs/api/composables.md

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* chore: remove leftover of isntance.provide

* chore: remove subscribers from context

* chore: abstract `wrapCallback`  and move render loop register to `useRender`

* chore: fix lint

* chore: testing off

* Revert "chore: abstract `wrapCallback`  and move render loop register to `useRender`"

This reverts commit 24cec65.

* chore: return bound `off` method and use createPriorityEvent for render with defaultFn fallback

* feat: deprecate and remove `vAlwaysLookAt` and `vRotate`

BREAKING_CHANGE: Directives `vAlwaysLookAt` and `vRotate` due incompatibility with new `useLoop` and the refactor of the render loop logic.

* feat: set context to loop to avoid wrapping the callbacks

* feat: dispose render hook before taking over

---------

Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>

* chore(playground): adding missing import and removing the directives that were deprecated

* chore(playground): use new composable on animations

* fix(utils): reorder object disposal to avoid issue with Helper `dispose` methods (#683)

* chore: updated deps

* chore: release v4.0.0-rc.1

* fix: manual rendering blank (#685)

* fix: increate time to advance on manual mode

* chore: correct playground

* fix: 686 useloop callback state missing controls (#687)

* fix(loop): take plain snapshots of ctx

* fix: types for useloop

* chore: lint

* docs: add RectAreaLightHelper to vLightHelper docs

* chore(deps): update deps 24-0-2024

* chore: release v4.0.0-rc.2

* fix: start loop if user calls useRenderLoop (#695)

* docs: change motivation

* chore(deps): last update before release

---------

Co-authored-by: Peter <petermoldovia@yahoo.ca>
Co-authored-by: Garrett Walker <garbwalk@gmail.com>
Co-authored-by: Tino Koch <17991193+Tinoooo@users.noreply.github.com>
Co-authored-by: Jaime Torrealba <solucionesinformaticasjtc@gmail.com>
Co-authored-by: Jaime A Torrealba C <63722373+JaimeTorrealba@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andretchen0 andretchen0 Awaiting requested review from andretchen0

Assignees

@alvarosabu alvarosabu

Labels
dx needs-documentation needs-test p3-significant High-priority enhancement (priority) v4
Projects
Team Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alvarosabu @andretchen0