From b8551e50e17bab03636bb15e1ffe20950582821f Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 1 Nov 2024 23:05:44 -0700 Subject: [PATCH] a new Feature Flag API Feature flags are intended to be used when a new feature is added to filament and have generally two purposes: 1) during feature development, the feature can be implemented but disabled which can help developing large features. This way the feature can be tested by stakeholders while it's being developed without impacting other clients. 2) once a feature is ready, its flag can be enabled by default, but in case the feature breaks something or has unintended consequence, clients have the option to turn it off. Feature flags are intended to have a relatively short life span, i.e. once a feature is stable, the flag fill be removed. There two types of feature flags. Constant feature flags can only be set during Engine initialization via Engine::Builder. Non-constant feature flags can be set at any time. Feature flags SHOULD NOT be used as configuration or settings. Feature flags are designed with a few ideas in mind: - they are very cheap to check inside the engine - non-constant flags can easily be toggled using ImGUI --- .../filament-android/src/main/cpp/Engine.cpp | 41 ++++++++ .../com/google/android/filament/Engine.java | 60 +++++++++++- filament/include/filament/Engine.h | 74 +++++++++++++++ filament/src/Engine.cpp | 17 ++++ filament/src/details/Engine.cpp | 93 +++++++++++++++++-- filament/src/details/Engine.h | 37 +++++++- 6 files changed, 309 insertions(+), 13 deletions(-) diff --git a/android/filament-android/src/main/cpp/Engine.cpp b/android/filament-android/src/main/cpp/Engine.cpp index 7de25a58ba..56aecc66dd 100644 --- a/android/filament-android/src/main/cpp/Engine.cpp +++ b/android/filament-android/src/main/cpp/Engine.cpp @@ -499,6 +499,37 @@ Java_com_google_android_filament_Engine_nGetActiveFeatureLevel(JNIEnv *, jclass, return (jint)engine->getActiveFeatureLevel(); } +extern "C" +JNIEXPORT jboolean JNICALL +Java_com_google_android_filament_Engine_nHasFeatureFlag(JNIEnv *env, jclass clazz, + jlong nativeEngine, jstring name_) { + Engine* engine = (Engine*) nativeEngine; + const char *name = env->GetStringUTFChars(name_, 0); + std::optional result = engine->getFeatureFlag(name); + env->ReleaseStringUTFChars(name_, name); + return result.has_value(); +} +extern "C" +JNIEXPORT jboolean JNICALL +Java_com_google_android_filament_Engine_nSetFeatureFlag(JNIEnv *env, jclass clazz, + jlong nativeEngine, jstring name_, jboolean value) { + Engine* engine = (Engine*) nativeEngine; + const char *name = env->GetStringUTFChars(name_, 0); + jboolean result = engine->setFeatureFlag(name, (bool)value); + env->ReleaseStringUTFChars(name_, name); + return result; +} +extern "C" +JNIEXPORT jboolean JNICALL +Java_com_google_android_filament_Engine_nGetFeatureFlag(JNIEnv *env, jclass clazz, + jlong nativeEngine, jstring name_) { + Engine* engine = (Engine*) nativeEngine; + const char *name = env->GetStringUTFChars(name_, 0); + std::optional result = engine->getFeatureFlag(name); + env->ReleaseStringUTFChars(name_, name); + return result.value_or(false); // we should never fail here +} + extern "C" JNIEXPORT jlong JNICALL Java_com_google_android_filament_Engine_nCreateBuilder(JNIEnv*, jclass) { Engine::Builder* builder = new Engine::Builder{}; @@ -565,6 +596,16 @@ extern "C" JNIEXPORT void JNICALL Java_com_google_android_filament_Engine_nSetBu builder->paused((bool) paused); } +extern "C" +JNIEXPORT void JNICALL +Java_com_google_android_filament_Engine_nSetBuilderFeature(JNIEnv *env, jclass clazz, + jlong nativeBuilder, jstring name_, jboolean value) { + Engine::Builder* builder = (Engine::Builder*) nativeBuilder; + const char *name = env->GetStringUTFChars(name_, 0); + builder->feature(name, (bool)value); + env->ReleaseStringUTFChars(name_, name); +} + extern "C" JNIEXPORT jlong JNICALL Java_com_google_android_filament_Engine_nBuilderBuild(JNIEnv*, jclass, jlong nativeBuilder) { Engine::Builder* builder = (Engine::Builder*) nativeBuilder; diff --git a/android/filament-android/src/main/java/com/google/android/filament/Engine.java b/android/filament-android/src/main/java/com/google/android/filament/Engine.java index a433a55d8c..1e49bf54a3 100644 --- a/android/filament-android/src/main/java/com/google/android/filament/Engine.java +++ b/android/filament-android/src/main/java/com/google/android/filament/Engine.java @@ -258,6 +258,17 @@ public class Engine { return this; } + /** + * Set a feature flag value. This is the only way to set constant feature flags. + * @param name feature name + * @param value true to enable, false to disable + * @return A reference to this Builder for chaining calls. + */ + public Builder feature(@NonNull String name, boolean value) { + nSetBuilderFeature(mNativeBuilder, name, value); + return this; + } + /** * Creates an instance of Engine * @@ -393,6 +404,7 @@ public class Engine { /** * Set to `true` to forcibly disable parallel shader compilation in the backend. * Currently only honored by the GL backend. + * @Deprecated use "backend.disable_parallel_shader_compile" feature flag instead */ public boolean disableParallelShaderCompile = false; @@ -433,6 +445,7 @@ public class Engine { /** * Disable backend handles use-after-free checks. + * @Deprecated use "backend.disable_handle_use_after_free_check" feature flag instead */ public boolean disableHandleUseAfterFreeCheck = false; @@ -469,6 +482,7 @@ public class Engine { * Assert the native window associated to a SwapChain is valid when calling makeCurrent(). * This is only supported for: * - PlatformEGLAndroid + * @Deprecated use "backend.opengl.assert_native_window_is_valid" feature flag instead */ public boolean assertNativeWindowIsValid = false; } @@ -693,11 +707,11 @@ public class Engine { /** * Returns the maximum number of stereoscopic eyes supported by Filament. The actual number of - * eyes rendered is set at Engine creation time with the {@link - * Engine#Config#stereoscopicEyeCount} setting. + * eyes rendered is set at Engine creation time with the {@link Config#stereoscopicEyeCount} + * setting. * * @return the max number of stereoscopic eyes supported - * @see Engine#Config#stereoscopicEyeCount + * @see Config#stereoscopicEyeCount */ public long getMaxStereoscopicEyes() { return nGetMaxStereoscopicEyes(getNativeObject()); @@ -894,7 +908,8 @@ public class Engine { /** * Returns whether the object is valid. - * @param object Object to check for validity + * @param ma Material + * @param mi MaterialInstance to check for validity * @return returns true if the specified object is valid. */ public boolean isValidMaterialInstance(@NonNull Material ma, MaterialInstance mi) { @@ -1316,6 +1331,39 @@ public class Engine { */ public static native long getSteadyClockTimeNano(); + + /** + * Checks if a feature flag exists + * @param name name of the feature flag to check + * @return true if it exists false otherwise + */ + public boolean hasFeatureFlag(@NonNull String name) { + return nHasFeatureFlag(mNativeObject, name); + } + + /** + * Set the value of a non-constant feature flag. + * @param name name of the feature flag to set + * @param value value to set + * @return true if the value was set, false if the feature flag is constant or doesn't exist. + */ + public boolean setFeatureFlag(@NonNull String name, boolean value) { + return nSetFeatureFlag(mNativeObject, name, value); + } + + /** + * Retrieves the value of any feature flag. + * @param name name of the feature flag + * @return the value of the flag if it exists + * @exception IllegalArgumentException is thrown if the feature flag doesn't exist + */ + public boolean getFeatureFlag(@NonNull String name) { + if (!hasFeatureFlag(name)) { + throw new IllegalArgumentException("The feature flag \"" + name + "\" doesn't exist"); + } + return nGetFeatureFlag(mNativeObject, name); + } + @UsedByReflection("TextureHelper.java") public long getNativeObject() { if (mNativeObject == 0) { @@ -1405,6 +1453,9 @@ public class Engine { private static native int nGetSupportedFeatureLevel(long nativeEngine); private static native int nSetActiveFeatureLevel(long nativeEngine, int ordinal); private static native int nGetActiveFeatureLevel(long nativeEngine); + private static native boolean nHasFeatureFlag(long nativeEngine, String name); + private static native boolean nSetFeatureFlag(long nativeEngine, String name, boolean value); + private static native boolean nGetFeatureFlag(long nativeEngine, String name); private static native long nCreateBuilder(); private static native void nDestroyBuilder(long nativeBuilder); @@ -1420,5 +1471,6 @@ public class Engine { private static native void nSetBuilderFeatureLevel(long nativeBuilder, int ordinal); private static native void nSetBuilderSharedContext(long nativeBuilder, long sharedContext); private static native void nSetBuilderPaused(long nativeBuilder, boolean paused); + private static native void nSetBuilderFeature(long nativeBuilder, String name, boolean value); private static native long nBuilderBuild(long nativeBuilder); } diff --git a/filament/include/filament/Engine.h b/filament/include/filament/Engine.h index 311cf2e5a0..17aaea975f 100644 --- a/filament/include/filament/Engine.h +++ b/filament/include/filament/Engine.h @@ -24,6 +24,10 @@ #include #include +#include + +#include +#include #include #include @@ -309,6 +313,7 @@ public: /** * Set to `true` to forcibly disable parallel shader compilation in the backend. * Currently only honored by the GL and Metal backends. + * @deprecated use "backend.disable_parallel_shader_compile" feature flag instead */ bool disableParallelShaderCompile = false; @@ -349,6 +354,7 @@ public: /* * Disable backend handles use-after-free checks. + * @deprecated use "backend.disable_handle_use_after_free_check" feature flag instead */ bool disableHandleUseAfterFreeCheck = false; @@ -385,10 +391,32 @@ public: * Assert the native window associated to a SwapChain is valid when calling makeCurrent(). * This is only supported for: * - PlatformEGLAndroid + * @deprecated use "backend.opengl.assert_native_window_is_valid" feature flag instead */ bool assertNativeWindowIsValid = false; }; + + /** + * Feature flags can be enabled or disabled when the Engine is built. Some Feature flags can + * also be toggled at any time. Feature flags should alawys use their default value unless + * the feature enabled by the flag is faulty. Feature flags provide a last resort way to + * disable problematic features. + * Feature flags are intended to have a short life-time and are regularly removed as features + * mature. + */ + struct FeatureFlag { + char const* UTILS_NONNULL name; //!< name of the feature flag + char const* UTILS_NONNULL description; //!< short description + bool const* UTILS_NONNULL value; //!< pointer to the value of the flag + bool constant; //!< whether the flag is constant after construction + }; + + /** + * Returns the list of available feature flags + */ + utils::Slice getFeatureFlags() const noexcept; + #if UTILS_HAS_THREADING using CreateCallback = void(void* UTILS_NULLABLE user, void* UTILS_NONNULL token); #endif @@ -461,6 +489,21 @@ public: */ Builder& paused(bool paused) noexcept; + /** + * Set a feature flag value. This is the only way to set constant feature flags. + * @param name feature name + * @param value true to enable, false to disable + * @return A reference to this Builder for chaining calls. + */ + Builder& feature(char const* UTILS_NONNULL name, bool value) noexcept; + + /** + * Enables a list of features. + * @param list list of feature names to enable. + * @return A reference to this Builder for chaining calls. + */ + Builder& features(std::initializer_list list) noexcept; + #if UTILS_HAS_THREADING /** * Creates the filament Engine asynchronously. @@ -1078,6 +1121,37 @@ public: DebugRegistry& getDebugRegistry() noexcept; + /** + * Check if a feature flag exists + * @param name name of the feature flag to check + * @return true if the feature flag exists, false otherwise + */ + inline bool hasFeatureFlag(char const* UTILS_NONNULL name) noexcept { + return getFeatureFlag(name).has_value(); + } + + /** + * Set the value of a non-constant feature flag. + * @param name name of the feature flag to set + * @param value value to set + * @return true if the value was set, false if the feature flag is constant or doesn't exist. + */ + bool setFeatureFlag(char const* UTILS_NONNULL name, bool value) noexcept; + + /** + * Retrieves the value of any feature flag. + * @param name name of the feature flag + * @return the value of the flag if it exists + */ + std::optional getFeatureFlag(char const* UTILS_NONNULL name) const noexcept; + + /** + * Returns a pointer to a non-constant feature flag value. + * @param name name of the feature flag + * @return a pointer to the feature flag value, or nullptr if the feature flag is constant or doesn't exist + */ + bool* UTILS_NULLABLE getFeatureFlagPtr(char const* UTILS_NONNULL name) const noexcept; + protected: //! \privatesection Engine() noexcept = default; diff --git a/filament/src/Engine.cpp b/filament/src/Engine.cpp index 771d02c22a..c95b944ccf 100644 --- a/filament/src/Engine.cpp +++ b/filament/src/Engine.cpp @@ -40,6 +40,7 @@ #include #include +#include #include @@ -442,6 +443,22 @@ uint64_t Engine::getSteadyClockTimeNano() noexcept { return std::chrono::steady_clock::now().time_since_epoch().count(); } +utils::Slice Engine::getFeatureFlags() const noexcept { + return downcast(this)->getFeatureFlags(); +} + +bool Engine::setFeatureFlag(char const* name, bool value) noexcept { + return downcast(this)->setFeatureFlag(name, value); +} + +std::optional Engine::getFeatureFlag(char const* name) const noexcept { + return downcast(this)->getFeatureFlag(name); +} + +bool* Engine::getFeatureFlagPtr(char const* UTILS_NONNULL name) const noexcept { + return downcast(this)->getFeatureFlagPtr(name); +} + #if defined(__EMSCRIPTEN__) void Engine::resetBackendState() noexcept { downcast(this)->resetBackendState(); diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp index a6c5f2eead..6b4e56a1e4 100644 --- a/filament/src/details/Engine.cpp +++ b/filament/src/details/Engine.cpp @@ -46,6 +46,7 @@ #include #include +#include #include #include #include @@ -53,8 +54,17 @@ #include #include +#include #include +#include #include +#include +#include +#include + +#include +#include +#include #include "generated/resources/materials.h" @@ -73,6 +83,8 @@ struct Engine::BuilderDetails { FeatureLevel mFeatureLevel = FeatureLevel::FEATURE_LEVEL_1; void* mSharedContext = nullptr; bool mPaused = false; + std::unordered_map mFeatureFlags; + static Config validateConfig(Config config) noexcept; }; @@ -104,11 +116,11 @@ Engine* FEngine::create(Engine::Builder const& builder) { DriverConfig const driverConfig{ .handleArenaSize = instance->getRequestedDriverHandleArenaSize(), .metalUploadBufferSizeBytes = instance->getConfig().metalUploadBufferSizeBytes, - .disableParallelShaderCompile = instance->getConfig().disableParallelShaderCompile, - .disableHandleUseAfterFreeCheck = instance->getConfig().disableHandleUseAfterFreeCheck, + .disableParallelShaderCompile = instance->features.backend.disable_parallel_shader_compile, + .disableHandleUseAfterFreeCheck = instance->features.backend.disable_handle_use_after_free_check, .forceGLES2Context = instance->getConfig().forceGLES2Context, .stereoscopicType = instance->getConfig().stereoscopicType, - .assertNativeWindowIsValid = instance->getConfig().assertNativeWindowIsValid, + .assertNativeWindowIsValid = instance->features.backend.opengl.assert_native_window_is_valid, }; instance->mDriver = platform->createDriver(sharedContext, driverConfig); @@ -219,7 +231,34 @@ FEngine::FEngine(Engine::Builder const& builder) : mMainThreadId(ThreadUtils::getThreadId()), mConfig(builder->mConfig) { - // we're assuming we're on the main thread here. + // update a feature flag from Engine::Config if the flag is not specified in the Builder + auto const featureFlagsBackwardCompatibility = + [this, &builder](std::string_view name, bool value) { + if (builder->mFeatureFlags.find(name) == builder->mFeatureFlags.end()) { + auto* const p = getFeatureFlagPtr(name, true); + if (p) { + *p = value; + } + } + }; + + // update all the features flags specified in the builder + for (auto const& feature : builder->mFeatureFlags) { + auto* const p = getFeatureFlagPtr(feature.first, true); + if (p) { + *p = feature.second; + } + } + + // update "old" feature flags that were specified in Engine::Config + featureFlagsBackwardCompatibility("backend.disable_parallel_shader_compile", + mConfig.disableParallelShaderCompile); + featureFlagsBackwardCompatibility("backend.disable_handle_use_after_free_check", + mConfig.disableHandleUseAfterFreeCheck); + featureFlagsBackwardCompatibility("backend.opengl.assert_native_window_is_valid", + mConfig.assertNativeWindowIsValid); + + // We're assuming we're on the main thread here. // (it may not be the case) mJobSystem.adopt(); @@ -702,11 +741,11 @@ int FEngine::loop() { DriverConfig const driverConfig { .handleArenaSize = getRequestedDriverHandleArenaSize(), .metalUploadBufferSizeBytes = mConfig.metalUploadBufferSizeBytes, - .disableParallelShaderCompile = mConfig.disableParallelShaderCompile, - .disableHandleUseAfterFreeCheck = mConfig.disableHandleUseAfterFreeCheck, + .disableParallelShaderCompile = features.backend.disable_parallel_shader_compile, + .disableHandleUseAfterFreeCheck = features.backend.disable_handle_use_after_free_check, .forceGLES2Context = mConfig.forceGLES2Context, .stereoscopicType = mConfig.stereoscopicType, - .assertNativeWindowIsValid = mConfig.assertNativeWindowIsValid, + .assertNativeWindowIsValid = features.backend.opengl.assert_native_window_is_valid, }; mDriver = mPlatform->createDriver(mSharedGLContext, driverConfig); @@ -1316,6 +1355,32 @@ void FEngine::unprotected() noexcept { mUnprotectedDummySwapchain->makeCurrent(getDriverApi()); } +bool FEngine::setFeatureFlag(char const* name, bool value) noexcept { + auto* const p = getFeatureFlagPtr(name); + if (p) { + *p = value; + } + return p != nullptr; +} + +std::optional FEngine::getFeatureFlag(char const* name) const noexcept { + auto* const p = getFeatureFlagPtr(name, true); + if (p) { + return *p; + } + return std::nullopt; +} + +bool* FEngine::getFeatureFlagPtr(std::string_view name, bool allowConstant) const noexcept { + auto pos = std::find_if(mFeatures.begin(), mFeatures.end(), + [name](FeatureFlag const& entry) { + return name == entry.name; + }); + + return (pos != mFeatures.end() && (!pos->constant || allowConstant)) ? + const_cast(pos->value) : nullptr; +} + // ------------------------------------------------------------------------------------------------ Engine::Builder::Builder() noexcept = default; @@ -1355,6 +1420,20 @@ Engine::Builder& Engine::Builder::paused(bool paused) noexcept { return *this; } +Engine::Builder& Engine::Builder::feature(char const* name, bool value) noexcept { + mImpl->mFeatureFlags[name] = value; + return *this; +} + +Engine::Builder& Engine::Builder::features(std::initializer_list list) noexcept { + for (auto name : list) { + if (name) { + feature(name, true); + } + } + return *this; +} + #if UTILS_HAS_THREADING void Engine::Builder::build(Invocable&& callback) const { diff --git a/filament/src/details/Engine.h b/filament/src/details/Engine.h index 712b103038..1f21849051 100644 --- a/filament/src/details/Engine.h +++ b/filament/src/details/Engine.h @@ -56,21 +56,24 @@ #include #include #include -#include #include #include #include #include #include +#include #include #include #include -#include +#include +#include #include #include #include +#include +#include #include #include #include @@ -672,6 +675,36 @@ public: } stereo; matdbg::DebugServer* server = nullptr; } debug; + + struct { + struct { + struct { + bool assert_native_window_is_valid = false; + } opengl; + bool disable_parallel_shader_compile = false; + bool disable_handle_use_after_free_check = false; + } backend; + } features; + + std::array const mFeatures{{ + { "backend.disable_parallel_shader_compile", + "Disable parallel shader compilation in GL and Metal backends.", + &features.backend.disable_parallel_shader_compile, true }, + { "backend.disable_handle_use_after_free_check", + "Disable Handle<> use-after-free checks.", + &features.backend.disable_handle_use_after_free_check, true }, + { "backend.opengl.assert_native_window_is_valid", + "Asserts that the ANativeWindow is valid when rendering starts.", + &features.backend.opengl.assert_native_window_is_valid, true } + }}; + + utils::Slice getFeatureFlags() const noexcept { + return { mFeatures.data(), mFeatures.size() }; + } + + bool setFeatureFlag(char const* name, bool value) noexcept; + std::optional getFeatureFlag(char const* name) const noexcept; + bool* getFeatureFlagPtr(std::string_view name, bool allowConstant = false) const noexcept; }; FILAMENT_DOWNCAST(Engine)