From 45953c0481577f0312e8606ab99359d33730baee Mon Sep 17 00:00:00 2001 From: Vincent Hengel Date: Tue, 29 Sep 2026 10:26:42 +0200 Subject: [PATCH] Feature flags live where they are read --- docs/STRUCTURE.md | 7 ++-- foundation/features/feature_registry.cc | 43 +++++++--------------- foundation/features/feature_registry.h | 48 ++++++++----------------- foundation/features/features.def | 18 ---------- rxe/physics/physics_world.cc | 5 ++- 5 files changed, 34 insertions(+), 87 deletions(-) delete mode 100644 foundation/features/features.def diff --git a/docs/STRUCTURE.md b/docs/STRUCTURE.md index 5ca7009d..fdac2073 100644 --- a/docs/STRUCTURE.md +++ b/docs/STRUCTURE.md @@ -189,7 +189,7 @@ nearly everywhere. Nothing in it may include `rxe/`. | `files` | `core/file_system`, `core/paths` | | `tasks` | `core/job_system` | | `memory` | `core/memory/*`, `core/shared.h` | -| `features` | `core/feature_registry`. `features.def` goes away: each module declares its own flags in its `features.h`, Chromium's `BASE_DECLARE_FEATURE` shape, so adding a flag no longer edits a shared file. | +| `features` | `core/feature_registry`. `features.def` is gone (5b): a module declares each flag as a `base::Feature` in the file that reads it, so adding a flag edits no shared file. `InitFeatures` applies `RX_FEATURES` over the one process-wide chain. | | `time` | `core/frame_timer` | | `system` | `core/platform`, `core/app_identity` | | `math` | `core/math.h`, `core/scalar.h` | @@ -371,7 +371,7 @@ What has to exist once per process, and how the shared build keeps it so: | `render/util` | grab-bag | done (4c): `gpu/rhi`, `asset/exr_write`, `ui/imgui` | | `runtime/engine_context.h` | god object passed to every demo | phase 6, with the rest of `runtime/` | | `fly_camera` | duplicated in `runtime/` and `apps/editor/` | done (2b): `scene::FlyCamera`, fed a resolved `FlyCameraInput` | -| `core/features.def` | one file every feature edits | phase 5: per-module flags self-register, which needs the one-per-process registry | +| `core/features.def` | one file every feature edits | done (5b): flags live where they are read | | `CMakeLists.txt` | 1381 lines, registers every test centrally | done (4b): each module registers its own | | `anim/locomotion` vs `engine/locomotion` | two modules called locomotion | done (2b): `anim::ProceduralGait` | @@ -424,7 +424,8 @@ and after. - 5a (done): the whole tree links as shared objects (missing exports added), one option registry per process, per-DSO volk filled through `GetVulkanHandles`, and the `linux-dev` preset. - - 5b: per-module feature flags on the single registry. + - 5b (done): per-module feature flags on the single registry; + `features.def` and its three never-read render flags are gone. 6. **Plugins and apps.** Add `RX_PLUGIN`, `PluginRegistry` and `rx_add_app`. `runtime/` becomes `apps/shell/`, the tools move into `apps/`, and `EngineContext` goes away. diff --git a/foundation/features/feature_registry.cc b/foundation/features/feature_registry.cc index 95249668..b73a602b 100644 --- a/foundation/features/feature_registry.cc +++ b/foundation/features/feature_registry.cc @@ -2,48 +2,29 @@ #include +#include "base/strings/string_ref.h" #include "foundation/logging/log.h" namespace rx { namespace { -// The one and only table. Each entry is direct-initialized in place, so -// base::Feature's deleted copy/move is never needed. Non-const because the env -// override flips `enabled`. This object is referenced by InitFeatures() which -// runs from engine startup, so the linker keeps it: no static-init stripping. -base::Feature g_features[] = { -#define RX_FEATURE(id, name, enabled) {name, enabled}, -#include "base/strings/string_ref.h" -#include "foundation/features/features.def" -#undef RX_FEATURE -}; - -constexpr unsigned kCount = static_cast(FeatureId::kCount); -static_assert(sizeof(g_features) / sizeof(g_features[0]) == kCount, - "features.def and FeatureId disagree on the flag count"); - -// Flip the flag whose name matches `name`. Returns false if there is no such -// flag. +// Flip the flag whose name matches `name`. Returns false if no flag in this +// binary carries it. bool ApplyOne(base::StringRef name, bool enabled) { - for (auto& f : g_features) { - if (name == f.name) { - f.enabled = enabled; - return true; + bool found = false; + base::Feature::VisitAll([&](const base::Feature* feature) { + if (name == feature->name) { + // The chain hands out const pointers; flags are mutable globals, which is + // the one thing this function exists to write. + const_cast(feature)->enabled = enabled; + found = true; } - } - return false; + }); + return found; } } // namespace -bool FeatureEnabled(FeatureId id) { - return g_features[static_cast(id)].enabled; -} - -base::Span Features() { - return base::Span(g_features); -} - void InitFeatures() { const char* spec = ::getenv("RX_FEATURES"); if (!spec || !*spec) return; diff --git a/foundation/features/feature_registry.h b/foundation/features/feature_registry.h index abbbbf9c..54eea096 100644 --- a/foundation/features/feature_registry.h +++ b/foundation/features/feature_registry.h @@ -1,8 +1,6 @@ #ifndef RX_FOUNDATION_FEATURES_FEATURE_REGISTRY_H_ #define RX_FOUNDATION_FEATURES_FEATURE_REGISTRY_H_ -#include - #include "foundation/build_config/export.h" // base/feature.h's constructor parameters shadow its members, so the header is // not -Wshadow clean. Silence just this include rather than spraying the @@ -21,42 +19,24 @@ #pragma warning(pop) #endif -// A linker-proof feature-flag registry. +// Feature flags. A module declares each flag at namespace scope in the file that +// reads it, and reads it as a bool: // -// base::Feature self-registers through InitChain, but that registration is a -// static initializer: when a flag lives in a static library and nothing else -// references its object file, the linker drops the object (and the -// registration with it). This registry sidesteps that entirely. Every flag is -// declared in core/features.def and lives in one table in one translation -// unit; InitFeatures() is called from engine startup, so that object is always -// pulled into the link and the table is always complete. We never walk the -// InitChain. - -namespace rx { - -// Stable handle for every flag in core/features.def. -enum class FeatureId : unsigned { -#define RX_FEATURE(id, name, enabled) k##id, -#include "foundation/features/features.def" -#undef RX_FEATURE - kCount -}; - -// True when the flag is on (default from the manifest, after env overrides). -// A single array read, no list walk. +// base::Feature ClothFeature{"physics.cloth", /*enabled=*/true}; +// ... +// if (!ClothFeature) return {}; // -// The g_features table backing these is an anonymous-namespace global in -// feature_registry.cc, reached only through these three functions, so under -// RX_SHARED it stays a single instance inside the core DSO (verified: no other -// translation unit can name the array). -RX_FOUNDATION_EXPORT bool FeatureEnabled(FeatureId id); +// base::Feature registers itself on one process-wide chain (equilibrium makes +// the chain root unique across shared objects), so there is no central list to +// edit. A flag lives exactly as long as the code that reads it: if the linker +// drops an unreferenced object, it drops the flag with the only reader. -// The whole table, for listing in the debug UI. size() == kCount. -RX_FOUNDATION_EXPORT base::Span Features(); +namespace rx { -// Apply RX_FEATURES overrides once, early in engine startup. Tokens are comma -// or whitespace separated: "name" / "+name" enable, "-name" / "name=0" disable. -// Unknown names are warned about and skipped. +// Apply RX_FEATURES overrides once, early in startup and before any flag is +// read. Tokens are comma or whitespace separated: "name" / "+name" enable, +// "-name" / "name=0" disable. A name no flag in this binary carries is warned +// about and skipped. RX_FOUNDATION_EXPORT void InitFeatures(); } // namespace rx diff --git a/foundation/features/features.def b/foundation/features/features.def deleted file mode 100644 index f46ee400..00000000 --- a/foundation/features/features.def +++ /dev/null @@ -1,18 +0,0 @@ -// The feature-flag manifest: the single source of truth for every flag. -// -// This file is intentionally header-guard-free and #included several times -// with RX_FEATURE redefined each time (the X-macro trick), so the enum, the -// table and the count can never drift apart. Editing this list is the only -// thing you do to add a flag. -// -// RX_FEATURE(Id, "dotted.name", default_enabled) -// Id C++ identifier, reached as rx::FeatureId::k -// "dotted.name" stable string the RX_FEATURES env override matches on -// default_enabled value before any override is applied -// -// A subsystem reads a flag with rx::FeatureEnabled(rx::FeatureId::k). - -RX_FEATURE(PathTracer, "render.pathtracer", false) -RX_FEATURE(MeshShaderLod, "render.mesh_shader_lod", false) -RX_FEATURE(DistantLod, "render.distant_lod", true) -RX_FEATURE(Cloth, "physics.cloth", true) diff --git a/rxe/physics/physics_world.cc b/rxe/physics/physics_world.cc index 80c7e58b..548d3386 100644 --- a/rxe/physics/physics_world.cc +++ b/rxe/physics/physics_world.cc @@ -70,6 +70,9 @@ static_assert(JPH_VERSION_MAJOR > 5 || (JPH_VERSION_MAJOR == 5 && JPH_VERSION_MI namespace rx::physics { namespace { +// Cloth simulation as a whole: off, AddCloth refuses and nothing steps. +base::Feature ClothFeature{"physics.cloth", /*enabled=*/true}; + namespace layers { constexpr JPH::ObjectLayer kStatic = 0; constexpr JPH::ObjectLayer kDynamic = 1; @@ -2568,7 +2571,7 @@ void PhysicsWorld::RemoveStrandGroom(StrandGroomId id) { } ClothId PhysicsWorld::CreateCloth(const ClothDesc& desc, const Mat4& transform) { - if (!impl_ || !FeatureEnabled(FeatureId::kCloth) || !desc.positions || !desc.indices || + if (!impl_ || !ClothFeature || !desc.positions || !desc.indices || desc.vertex_count < 3 || desc.index_count < 3 || (desc.pin_count > 0 && !desc.pins) || !IsFinite(transform)) { return 0;