fix: listener removal (+ cleaning up all resources in full render exa…

…mple) (#34) Co-authored-by: Marc Rousavy <me@mrousavy.com>
margelo · Mar 11, 2024 · 1671087 · 1671087
1 parent 786d3fb
commit 1671087
Show file tree

Hide file tree

Showing 20 changed files with 124 additions and 106 deletions.
diff --git a/output.mp4 b/output.mp4
diff --git a/package/cpp/Choreographer.cpp b/package/cpp/Choreographer.cpp
@@ -8,14 +8,13 @@
 namespace margelo {
 
 std::shared_ptr<Listener> Choreographer::addOnFrameListener(Choreographer::OnFrameCallback onFrameCallback) {
-  auto listener = _listeners.add(std::move(onFrameCallback));
-  return std::make_shared<Listener>(std::move(listener));
+  return _listeners->add(std::move(onFrameCallback));
 }
 
 void Choreographer::onFrame(double timestamp) {
-  for (const auto& listener : _listeners.getListeners()) {
+  for (const auto& listener : _listeners->getListeners()) {
     listener(timestamp);
   }
 }
 
-} // namespace margelo
+} // namespace margelo
diff --git a/package/cpp/Choreographer.h b/package/cpp/Choreographer.h
@@ -24,7 +24,7 @@ class Choreographer {
   void onFrame(double timestamp);
 
 private:
-  ListenerManager<OnFrameCallback> _listeners;
+  std::shared_ptr<ListenerManager<OnFrameCallback>> _listeners = ListenerManager<OnFrameCallback>::create();
 };
 
 } // namespace margelo
diff --git a/package/cpp/Listener.cpp b/package/cpp/Listener.cpp
@@ -3,12 +3,16 @@
 //
 
 #include "Listener.h"
+#include "Logger.h"
 
 namespace margelo {
 
-Listener::Listener(const std::function<void()>& remove) : HybridObject("Listener"), _remove(remove), _isRemoved(false) {}
+Listener::Listener(const std::function<void()>& remove) : _remove(remove), _isRemoved(false) {
+  Logger::log(TAG, "Creating Listener...");
+}
 
 Listener::~Listener() {
+  Logger::log(TAG, "Destroying Listener...");
   remove();
 }
 
@@ -20,8 +24,8 @@ void Listener::remove() {
   _isRemoved = true;
 }
 
-void Listener::loadHybridMethods() {
-  registerHybridMethod("remove", &Listener::remove, this);
+std::shared_ptr<Listener> Listener::create(margelo::Listener::ListenerRemover remover) {
+  return std::shared_ptr<Listener>(new Listener(std::move(remover)));
 }
 
 } // namespace margelo
diff --git a/package/cpp/Listener.h b/package/cpp/Listener.h
@@ -6,21 +6,25 @@
 
 #include "jsi/HybridObject.h"
 #include <functional>
+#include <memory>
 
 namespace margelo {
 
-class Listener : public HybridObject {
+class Listener {
 public:
-  Listener(Listener&& listener) : HybridObject("Listener"), _remove(std::move(listener._remove)), _isRemoved(listener._isRemoved) {}
-  explicit Listener(const std::function<void()>& remove);
+  using ListenerRemover = std::function<void()>;
+
+  static std::shared_ptr<Listener> create(ListenerRemover remover);
   ~Listener();
   void remove();
 
-  void loadHybridMethods() override;
+private:
+  explicit Listener(const std::function<void()>& remove);
 
 private:
   std::function<void()> _remove;
   bool _isRemoved;
+  static constexpr auto TAG = "Listener";
 };
 
 } // namespace margelo
diff --git a/package/cpp/ListenerManager.h b/package/cpp/ListenerManager.h
@@ -14,31 +14,37 @@ namespace margelo {
 
 template <typename Callback> class ListenerManager : public std::enable_shared_from_this<ListenerManager<Callback>> {
 private:
-  using TSelf = ListenerManager<Callback>;
   std::list<Callback> _listeners;
 
-private:
-  std::shared_ptr<TSelf> shared() {
-    return this->shared_from_this();
-  }
-
 public:
-  Listener add(Callback listener) {
+  std::shared_ptr<Listener> add(Callback listener) {
     _listeners.push_back(std::move(listener));
-    // TODO(Marc): fix this to not cause a bad_weak_ptr
-    //    auto id = --_listeners.end();
-    //    auto weakThis = std::weak_ptr<TSelf>(shared());
-    return Listener([]() {
-      //      auto sharedThis = weakThis.lock();
-      //      if (sharedThis) {
-      //        sharedThis->_listeners.erase(id);
-      //      }
+    auto id = --_listeners.end();
+
+    auto weakThis = std::weak_ptr<ListenerManager<Callback>>(shared());
+    return Listener::create([id, weakThis] {
+      auto sharedThis = weakThis.lock();
+      if (sharedThis) {
+        sharedThis->_listeners.erase(id);
+      }
     });
   }
 
   const std::list<Callback>& getListeners() {
     return _listeners;
   }
+
+private:
+  explicit ListenerManager() {}
+
+  std::shared_ptr<ListenerManager<Callback>> shared() {
+    return this->shared_from_this();
+  }
+
+public:
+  static std::shared_ptr<ListenerManager<Callback>> create() {
+    return std::shared_ptr<ListenerManager<Callback>>(new ListenerManager());
+  }
 };
 
 } // namespace margelo
diff --git a/package/cpp/SurfaceProvider.cpp b/package/cpp/SurfaceProvider.cpp
@@ -11,32 +11,32 @@ void SurfaceProvider::loadHybridMethods() {
   registerHybridMethod("getSurface", &SurfaceProvider::getSurfaceOrNull, this);
 }
 
-Listener SurfaceProvider::addOnSurfaceChangedListener(SurfaceProvider::Callback callback) {
+std::shared_ptr<Listener> SurfaceProvider::addOnSurfaceChangedListener(SurfaceProvider::Callback callback) {
   std::unique_lock lock(_mutex);
 
-  return _listeners.add(std::move(callback));
+  return _listeners->add(std::move(callback));
 }
 
 void SurfaceProvider::onSurfaceCreated(std::shared_ptr<Surface> surface) {
   Logger::log(TAG, "Surface created!");
   std::unique_lock lock(_mutex);
-  for (const auto& listener : _listeners.getListeners()) {
+  for (const auto& listener : _listeners->getListeners()) {
     listener.onSurfaceCreated(surface);
   }
 }
 
 void SurfaceProvider::onSurfaceChanged(std::shared_ptr<Surface> surface, int width, int height) {
   Logger::log(TAG, "Surface resized to %i x %i!", width, height);
   std::unique_lock lock(_mutex);
-  for (const auto& listener : _listeners.getListeners()) {
+  for (const auto& listener : _listeners->getListeners()) {
     listener.onSurfaceSizeChanged(surface, width, height);
   }
 }
 
 void SurfaceProvider::onSurfaceDestroyed(std::shared_ptr<Surface> surface) {
   Logger::log(TAG, "Surface destroyed!");
   std::unique_lock lock(_mutex);
-  for (const auto& listener : _listeners.getListeners()) {
+  for (const auto& listener : _listeners->getListeners()) {
     listener.onSurfaceDestroyed(surface);
   }
 }

diff --git a/package/cpp/SurfaceProvider.h b/package/cpp/SurfaceProvider.h
@@ -31,7 +31,7 @@ class SurfaceProvider : public HybridObject {
   explicit SurfaceProvider() : HybridObject("SurfaceProvider") {}
 
 public:
-  Listener addOnSurfaceChangedListener(Callback callback);
+  std::shared_ptr<Listener> addOnSurfaceChangedListener(Callback callback);
 
   virtual std::shared_ptr<Surface> getSurfaceOrNull() = 0;
 
@@ -43,7 +43,7 @@ class SurfaceProvider : public HybridObject {
   void onSurfaceDestroyed(std::shared_ptr<Surface> surface);
 
 private:
-  ListenerManager<Callback> _listeners;
+  std::shared_ptr<ListenerManager<Callback>> _listeners = ListenerManager<Callback>::create();
   std::mutex _mutex;
 
 private:

diff --git a/package/cpp/core/EngineWrapper.cpp b/package/cpp/core/EngineWrapper.cpp
@@ -141,14 +141,14 @@ void EngineWrapper::setSurfaceProvider(std::shared_ptr<SurfaceProvider> surfaceP
                                          },
                                      .onSurfaceDestroyed =
                                          [=](std::shared_ptr<Surface> surface) {
-                                           // TODO: Properly delete swapchain
-                                           //                                           queue->runOnJSAndWait([=]() {
-                                           //                                             Logger::log(TAG, "Destroying surface...");
-                                           //                                             sharedThis->destroySurface();
-                                           //                                           });
+                                           // TODO(Marc): When compiling in debug mode we get an assertion error here, because we are
+                                           // destroying the surface from the wrong thread.
+                                           queue->runOnJSAndWait([=]() {
+                                             Logger::log(TAG, "Destroying surface...");
+                                             sharedThis->destroySurface();
+                                           });
                                          }};
-  Listener listener = surfaceProvider->addOnSurfaceChangedListener(callback);
-  _listener = std::make_shared<Listener>(std::move(listener));
+  _surfaceListener = surfaceProvider->addOnSurfaceChangedListener(callback);
 }
 
 void EngineWrapper::setSurface(std::shared_ptr<Surface> surface) {
@@ -171,6 +171,7 @@ void EngineWrapper::setSurface(std::shared_ptr<Surface> surface) {
       sharedThis->renderFrame(timestamp);
     }
   });
+
   // Start the rendering
   _choreographer->start();
 }
@@ -187,7 +188,9 @@ void EngineWrapper::surfaceSizeChanged(int width, int height) {
 void EngineWrapper::destroySurface() {
   _choreographer->stop();
   _choreographerListener->remove();
+  _renderCallback = std::nullopt;
   _swapChain = nullptr;
+  _surfaceListener->remove();
 }
 
 void EngineWrapper::setRenderCallback(std::optional<RenderCallback> callback) {
@@ -262,9 +265,11 @@ std::shared_ptr<SwapChainWrapper> EngineWrapper::createSwapChain(std::shared_ptr
 }
 
 std::shared_ptr<CameraWrapper> EngineWrapper::createCamera() {
-  std::shared_ptr<Camera> camera = References<Camera>::adoptEngineRef(
-      _engine, _engine->createCamera(_engine->getEntityManager().create()),
-      [](const std::shared_ptr<Engine>& engine, Camera* camera) { engine->destroyCameraComponent(camera->getEntity()); });
+  std::shared_ptr<Camera> camera = References<Camera>::adoptEngineRef(_engine, _engine->createCamera(_engine->getEntityManager().create()),
+                                                                      [](const std::shared_ptr<Engine>& engine, Camera* camera) {
+                                                                        EntityManager::get().destroy(camera->getEntity());
+                                                                        engine->destroyCameraComponent(camera->getEntity());
+                                                                      });
 
   const float aperture = 16.0f;
   const float shutterSpeed = 1.0f / 125.0f;

diff --git a/package/cpp/core/EngineWrapper.h b/package/cpp/core/EngineWrapper.h
@@ -83,7 +83,7 @@ class EngineWrapper : public HybridObject {
   std::shared_ptr<JSDispatchQueue> _jsDispatchQueue;
   std::shared_ptr<Engine> _engine;
   std::shared_ptr<SurfaceProvider> _surfaceProvider;
-  std::shared_ptr<Listener> _listener;
+  std::shared_ptr<Listener> _surfaceListener;
   std::optional<RenderCallback> _renderCallback;
   std::function<std::shared_ptr<FilamentBuffer>(std::string)> _getAssetBytes;
   std::shared_ptr<Choreographer> _choreographer;

diff --git a/package/example/ios/Podfile.lock b/package/example/ios/Podfile.lock
@@ -929,7 +929,7 @@ PODS:
   - React-Mapbuffer (0.73.4):
     - glog
     - React-debug
-  - react-native-filament (0.2.0):
+  - react-native-filament (0.3.0):
     - Filament
     - glog
     - RCT-Folly (= 2022.05.16.00)
@@ -1297,7 +1297,7 @@ SPEC CHECKSUMS:
   React-jsinspector: 9ac353eccf6ab54d1e0a33862ba91221d1e88460
   React-logger: 0a57b68dd2aec7ff738195f081f0520724b35dab
   React-Mapbuffer: 63913773ed7f96b814a2521e13e6d010282096ad
-  react-native-filament: e97e7da21e3cf4879a2122b887a0bfe0ea13394f
+  react-native-filament: 73ac0fa14f9b6c7df7fe748f4a12ed82706364dc
   React-nativeconfig: d7af5bae6da70fa15ce44f045621cf99ed24087c
   React-NativeModulesApple: 0123905d5699853ac68519607555a9a4f5c7b3ac
   React-perflogger: 8a1e1af5733004bdd91258dcefbde21e0d1faccd

diff --git a/package/example/src/App.tsx b/package/example/src/App.tsx
@@ -1,8 +1,8 @@
 import * as React from 'react'
-import { useEffect, useMemo, useRef } from 'react'
+import { useCallback, useEffect, useMemo, useRef } from 'react'
 
 import { Platform, StyleSheet } from 'react-native'
-import { FilamentProxy, Filament, useEngine, Float3 } from 'react-native-filament'
+import { FilamentProxy, Filament, useEngine, Float3, useRenderCallback, RenderCallback } from 'react-native-filament'
 
 const penguModelPath = Platform.select({
   android: 'custom/pengu.glb',
@@ -23,8 +23,20 @@ const near = 0.1
 const far = 1000
 
 export default function App() {
-  const engine = useEngine({
-    onFrame: (_timestamp, _startTime, passedSeconds) => {
+  const engine = useEngine()
+
+  const [_pengu, penguAnimator] = useMemo(() => {
+    const modelBuffer = FilamentProxy.getAssetByteBuffer(penguModelPath)
+    const asset = engine.loadAsset(modelBuffer)
+    const animator = asset.getAnimator()
+    asset.releaseSourceData()
+
+    return [asset, animator]
+  }, [engine])
+
+  const prevAspectRatio = useRef(0)
+  const renderCallback: RenderCallback = useCallback(
+    (_timestamp, _startTime, passedSeconds) => {
       const view = engine.getView()
       const aspectRatio = view.aspectRatio
       if (prevAspectRatio.current !== aspectRatio) {
@@ -39,18 +51,9 @@ export default function App() {
 
       engine.getCamera().lookAt(cameraPosition, cameraTarget, cameraUp)
     },
-  })
-
-  const [_pengu, penguAnimator] = useMemo(() => {
-    const modelBuffer = FilamentProxy.getAssetByteBuffer(penguModelPath)
-    const asset = engine.loadAsset(modelBuffer)
-    const animator = asset.getAnimator()
-    asset.releaseSourceData()
-
-    return [asset, animator]
-  }, [engine])
-
-  const prevAspectRatio = useRef(0)
+    [engine, penguAnimator]
+  )
+  useRenderCallback(engine, renderCallback)
 
   // Setup the 3D scene:
   useEffect(() => {

diff --git a/package/ios/src/FilamentMetalView.h b/package/ios/src/FilamentMetalView.h
@@ -7,8 +7,8 @@
 
 #pragma once
 
-#import <UIKit/UIKit.h>
 #import <React/RCTViewManager.h>
+#import <UIKit/UIKit.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)layoutSubviews;
 
-@property (nonatomic, strong) RCTDirectEventBlock onViewReady;
+@property(nonatomic, strong) RCTDirectEventBlock onViewReady;
 
 - (void)didMoveToWindow;
 

diff --git a/package/ios/src/FilamentMetalView.mm b/package/ios/src/FilamentMetalView.mm
@@ -1,10 +1,10 @@
 #import "FilamentMetalView.h"
 #import <Metal/Metal.h>
-#import <UIKit/UIKit.h>
 #import <React/RCTViewManager.h>
+#import <UIKit/UIKit.h>
 
 @implementation FilamentMetalView {
-    bool isMounted;
+  bool isMounted;
 }
 
 + (Class)layerClass {
@@ -27,7 +27,7 @@ - (instancetype)init {
 
 - (void)didMoveToWindow {
   [super didMoveToWindow];
-    
+
   if (self.window != nil && !isMounted) {
     isMounted = true;
     self.onViewReady(@{});

diff --git a/package/package.json b/package/package.json
@@ -1,6 +1,6 @@
 {
   "name": "react-native-filament",
-  "version": "0.2.0",
+  "version": "0.3.0",
   "description": "A real-time physically based 3D rendering engine for React Native",
   "main": "lib/commonjs/index",
   "module": "lib/module/index",

diff --git a/package/src/Filament.tsx b/package/src/Filament.tsx
@@ -37,7 +37,6 @@ export class Filament extends React.PureComponent<FilamentProps> {
     if (this.view == null) {
       throw new Error(`Failed to find FilamentView #${handle}!`)
     }
-
     const surfaceProvider = this.view.getSurfaceProvider()
     // Link the surface with the engine:
     this.props.engine.setSurfaceProvider(surfaceProvider)