From ad29b9c70a3bf6e59fdcb22eca84a183fddb97cb Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 15 Aug 2024 00:18:56 -0700 Subject: [PATCH] fix several issue with the debug datasource in View - the last View created was always overriding previous View's datasource - because of lazy registering of the data source it was possible that the registering lambda was called after the view was destroyed, leading to crashes - all view would share the same PID parameters and these would be initialized to default value instead of the user provided value. so debug build would behave differently. With this change we improve things: - now only the first view gets to publish its data source. it's still not ideal, but works for our use case with gltf_Viewer - the view can now unregister itself when it's destroyed - only the view that successfully registered uses the debug PID values and publishes its data source. - the normal parameters are used until we query the datasource (from imgui), so by default the behavior is now identical to release builds This fixes a crash in gltf_viewer when opening the Debug panel. --- filament/src/details/DebugRegistry.cpp | 21 ++++++-- filament/src/details/DebugRegistry.h | 6 ++- filament/src/details/View.cpp | 66 ++++++++++++++++++-------- filament/src/details/View.h | 7 ++- 4 files changed, 74 insertions(+), 26 deletions(-) diff --git a/filament/src/details/DebugRegistry.cpp b/filament/src/details/DebugRegistry.cpp index ad1a54df74..c2f5d3d3fb 100644 --- a/filament/src/details/DebugRegistry.cpp +++ b/filament/src/details/DebugRegistry.cpp @@ -118,19 +118,32 @@ template bool FDebugRegistry::getProperty(const char* name, float2* v) c template bool FDebugRegistry::getProperty(const char* name, float3* v) const noexcept; template bool FDebugRegistry::getProperty(const char* name, float4* v) const noexcept; -void FDebugRegistry::registerDataSource(std::string_view name, +bool FDebugRegistry::registerDataSource(std::string_view name, void const* data, size_t count) noexcept { auto& dataSourceMap = mDataSourceMap; - if (dataSourceMap.find(name) == dataSourceMap.end()) { + bool const found = dataSourceMap.find(name) == dataSourceMap.end(); + if (found) { dataSourceMap[name] = { data, count }; } + return found; } -void FDebugRegistry::registerDataSource(std::string_view name, +bool FDebugRegistry::registerDataSource(std::string_view name, utils::Invocable&& creator) noexcept { - mDataSourceCreatorMap[name] = std::move(creator); + auto& dataSourceCreatorMap = mDataSourceCreatorMap; + bool const found = dataSourceCreatorMap.find(name) == dataSourceCreatorMap.end(); + if (found) { + dataSourceCreatorMap[name] = std::move(creator); + } + return found; } +void FDebugRegistry::unregisterDataSource(std::string_view name) noexcept { + mDataSourceCreatorMap.erase(name); + mDataSourceMap.erase(name); +} + + DebugRegistry::DataSource FDebugRegistry::getDataSource(const char* name) const noexcept { std::string_view const key{ name }; auto& dataSourceMap = mDataSourceMap; diff --git a/filament/src/details/DebugRegistry.h b/filament/src/details/DebugRegistry.h index b60a1c6994..92a25b82e9 100644 --- a/filament/src/details/DebugRegistry.h +++ b/filament/src/details/DebugRegistry.h @@ -101,12 +101,14 @@ public: } // registers a DataSource directly - void registerDataSource(std::string_view name, void const* data, size_t count) noexcept; + bool registerDataSource(std::string_view name, void const* data, size_t count) noexcept; // registers a DataSource lazily - void registerDataSource(std::string_view name, + bool registerDataSource(std::string_view name, utils::Invocable&& creator) noexcept; + void unregisterDataSource(std::string_view name) noexcept; + #if !defined(_MSC_VER) private: #endif diff --git a/filament/src/details/View.cpp b/filament/src/details/View.cpp index 1a3b54deb2..85fc9df5ca 100644 --- a/filament/src/details/View.cpp +++ b/filament/src/details/View.cpp @@ -31,15 +31,17 @@ #include "details/Skybox.h" #include +#include #include #include #include +#include +#include #include #include #include -#include #include #include @@ -47,6 +49,7 @@ #include #include +#include using namespace utils; @@ -78,21 +81,29 @@ FView::FView(FEngine& engine) mPidController.setOutputDeadBand(-0.01f, 0.05f); #ifndef NDEBUG - debugRegistry.registerDataSource("d.view.frame_info", - [this]() -> DebugRegistry::DataSource { - assert_invariant(!mDebugFrameHistory); - mDebugFrameHistory = std::make_unique>(); - return { mDebugFrameHistory->data(), mDebugFrameHistory->size() }; + // This can fail if another view has already registered this data source + mDebugState->owner = debugRegistry.registerDataSource("d.view.frame_info", + [weak = std::weak_ptr(mDebugState)]() -> DebugRegistry::DataSource { + // the View could have been destroyed by the time we do this + auto const state = weak.lock(); + if (!state) { + return { nullptr, 0 }; + } + // Lazily allocate the buffer for the debug data source, and mark this + // data source as active. It can never go back to inactive. + assert_invariant(!state->debugFrameHistory); + state->active = true; + state->debugFrameHistory = + std::make_unique>(); + return { state->debugFrameHistory->data(), state->debugFrameHistory->size() }; }); - debugRegistry.registerProperty("d.view.pid.kp", &engine.debug.view.pid.kp); - debugRegistry.registerProperty("d.view.pid.ki", &engine.debug.view.pid.ki); - debugRegistry.registerProperty("d.view.pid.kd", &engine.debug.view.pid.kd); - // default parameters for debugging UI - engine.debug.view.pid.kp = 1.0f - std::exp(-1.0f / 8.0f); - engine.debug.view.pid.ki = PID_CONTROLLER_Ki; - engine.debug.view.pid.kd = PID_CONTROLLER_Kd; - mPidController.setParallelGains( - engine.debug.view.pid.kp, engine.debug.view.pid.ki, engine.debug.view.pid.kd); + + if (UTILS_UNLIKELY(mDebugState->owner)) { + // publish the properties (they will be initialized in the main loop) + debugRegistry.registerProperty("d.view.pid.kp", &engine.debug.view.pid.kp); + debugRegistry.registerProperty("d.view.pid.ki", &engine.debug.view.pid.ki); + debugRegistry.registerProperty("d.view.pid.kd", &engine.debug.view.pid.kd); + } #endif // allocate UBOs @@ -126,6 +137,12 @@ void FView::terminate(FEngine& engine) { mFroxelizer.terminate(driver); engine.getEntityManager().destroy(mFogEntity); + +#ifndef NDEBUG + if (UTILS_UNLIKELY(mDebugState->owner)) { + engine.getDebugRegistry().unregisterDataSource("d.view.frame_info"); + } +#endif } void FView::setViewport(filament::Viewport const& viewport) noexcept { @@ -167,6 +184,16 @@ float2 FView::updateScale(FEngine& engine, Renderer::FrameRateOptions const& frameRateOptions, Renderer::DisplayInfo const& displayInfo) noexcept { +#ifndef NDEBUG + if (UTILS_LIKELY(!mDebugState->active)) { + // if we're not active, update the debug properties with the normal values + // and use that for configuring the PID controller. + engine.debug.view.pid.kp = 1.0f - std::exp(-frameRateOptions.scaleRate); + engine.debug.view.pid.ki = PID_CONTROLLER_Ki; + engine.debug.view.pid.kd = PID_CONTROLLER_Kd; + } +#endif + DynamicResolutionOptions const& options = mDynamicResolution; if (options.enabled) { if (!UTILS_UNLIKELY(info.valid)) { @@ -250,14 +277,15 @@ float2 FView::updateScale(FEngine& engine, #ifndef NDEBUG // only for debugging... - if (mDebugFrameHistory) { + if (UTILS_UNLIKELY(mDebugState->active && mDebugState->debugFrameHistory)) { + auto* const debugFrameHistory = mDebugState->debugFrameHistory.get(); using namespace std::chrono; using duration_ms = duration; const float target = (1000.0f * float(frameRateOptions.interval)) / displayInfo.refreshRate; const float targetWithHeadroom = target * (1.0f - frameRateOptions.headRoomRatio); - std::move(mDebugFrameHistory->begin() + 1, - mDebugFrameHistory->end(), mDebugFrameHistory->begin()); - mDebugFrameHistory->back() = { + std::move(debugFrameHistory->begin() + 1, + debugFrameHistory->end(), debugFrameHistory->begin()); + debugFrameHistory->back() = { .target = target, .targetWithHeadroom = targetWithHeadroom, .frameTime = duration_cast(info.frameTime).count(), diff --git a/filament/src/details/View.h b/filament/src/details/View.h index 73e7c9dc61..e24f150a2d 100644 --- a/filament/src/details/View.h +++ b/filament/src/details/View.h @@ -576,7 +576,12 @@ private: }}; #ifndef NDEBUG - std::unique_ptr> mDebugFrameHistory; + struct DebugState { + std::unique_ptr> debugFrameHistory{}; + bool owner = false; + bool active = false; + }; + std::shared_ptr mDebugState{ new DebugState }; #endif };