Skip to content

Commit 52ffd80

Browse files
pixelflingerpixelflinger
authored
Refactor FrameGraph pass API for flexibility (#9571)
This commit simplifies and unifies the FrameGraph's `addPass` API. Key changes: - The `addPass` method now uses a single, more flexible implementation that handles different lambda signatures for both the setup and execute stages. - The execute lambda can now optionally omit the `DriverApi&` parameter, reducing boilerplate for simple passes. - The `addTrivialSideEffectPass` helper has been removed. Side-effect-only passes can now be created directly with `addPass`
1 parent 44dde74 commit 52ffd80

File tree

6 files changed

+111
-82
lines changed

6 files changed

+111
-82
lines changed

‎filament/src/PostProcessManager.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3154,7 +3154,7 @@ FrameGraphId<FrameGraphTexture> PostProcessManager::taa(FrameGraph& fg,
31543154
// an "import".
31553155
builder.sideEffect();
31563156
data.color = builder.sample(history); // FIXME: an access must be declared for detach(), why?
3157-
}, [&current](FrameGraphResources const& resources, auto const& data, auto&) {
3157+
}, [&current](FrameGraphResources const& resources, auto const& data) {
31583158
resources.detach(data.color, &current.color, &current.desc);
31593159
});
31603160

@@ -3805,7 +3805,7 @@ FrameGraphId<FrameGraphTexture> PostProcessManager::vsmMipmapPass(FrameGraph& fg
38053805

38063806
auto const& depthMipmapPass = fg.addPass<VsmMipData>("VSM Generate Mipmap Pass",
38073807
[&](FrameGraph::Builder& builder, auto& data) {
3808-
utils::StaticString name = builder.getName(input);
3808+
StaticString const name = builder.getName(input);
38093809
data.in = builder.sample(input);
38103810

38113811
auto out = builder.createSubresource(data.in, "Mip level", {

‎filament/src/details/Renderer.cpp‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1057,10 +1057,11 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
10571057
builder.declareRenderPass("Picking Resolve Target", {
10581058
.attachments = { .color = { data.picking }}
10591059
});
1060+
// This pass' output is the result of the picking query (via readPixels),
1061+
// which exists outside the FrameGraph. So it needs to declare a side effect.
10601062
builder.sideEffect();
10611063
},
1062-
[=, &view](FrameGraphResources const& resources,
1063-
auto const&, DriverApi& driver) mutable {
1064+
[=, &view](FrameGraphResources const& resources, auto&, DriverApi& driver) mutable {
10641065
auto [target, params] = resources.getRenderPassInfo();
10651066
view.executePickingQueries(driver, target, scale * aoOptions.resolution);
10661067
});
@@ -1108,9 +1109,12 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
11081109
}();
11091110

11101111
// a non-drawing pass to prepare everything that need to be before the color passes execute
1111-
fg.addTrivialSideEffectPass("Prepare Color Passes",
1112-
[colorGradingConfig, colorGrading, colorBufferDesc, vignetteOptions,
1113-
&js, &view, &ppm](DriverApi& driver) {
1112+
fg.addPass<FrameGraph::Empty>("Prepare Color Passes",
1113+
[](FrameGraph::Builder& builder) {
1114+
// FIXME: use a dummy resource instead
1115+
builder.sideEffect();
1116+
},
1117+
[=, &js, &view, &ppm](auto&, auto&, DriverApi& driver) {
11141118
// prepare color grading as subpass material
11151119
if (colorGradingConfig.asSubpass) {
11161120
ppm.colorGradingPrepareSubpass(driver,
@@ -1133,7 +1137,6 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
11331137
}
11341138
});
11351139

1136-
11371140
// --------------------------------------------------------------------------------------------
11381141
// Color passes
11391142

@@ -1270,8 +1273,7 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
12701273

12711274
// we can't use colorPassOutput here because it could be tonemapped
12721275
data.history = builder.sample(colorPassOutput.linearColor); // FIXME: an access must be declared for detach(), why?
1273-
}, [&view, projection](FrameGraphResources const& resources, auto const& data,
1274-
DriverApi&) {
1276+
}, [&view, projection](FrameGraphResources const& resources, auto const& data) {
12751277
auto& history = view.getFrameHistory();
12761278
auto& current = history.getCurrent();
12771279
current.ssr.projection = projection;

‎filament/src/fg/FrameGraph.h‎

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -276,23 +276,24 @@ class FrameGraph {
276276
* @return A reference to a Pass object
277277
*/
278278
template<typename Data, typename Setup, typename Execute>
279-
FrameGraphPass<Data>& addPass(const char* name, Setup setup, Execute&& execute);
280-
281-
template<typename Data, typename Setup>
282-
FrameGraphPass<Data>& addPass(const char* name, Setup setup);
279+
auto& addPass(const char* name, Setup setup, Execute&& execute);
283280

284281
/**
285-
* Adds a simple execute-only pass with side-effect. Use with caution as such a pass is never
286-
* culled.
282+
* Add a setup only pass to the frame graph.
287283
*
288-
* @tparam Execute A lambda of type [](DriverApi&)
284+
* @tparam Data A user-defined structure containing this pass data
285+
* @tparam Setup A lambda of type [](Builder&, Data&).
289286
* @param name A name for this pass. Used for debugging only.
290-
* @param execute lambda called asynchronously from FrameGraph::execute(),
291-
* where immediate drawing commands can be issued.
292-
* Captures must be done by copy.
287+
* @param setup lambda called synchronously, used to declare which resources or
288+
* sub-resources.
289+
* @return
293290
*/
294-
template<typename Execute>
295-
void addTrivialSideEffectPass(const char* name, Execute&& execute);
291+
template<typename Data, typename Setup>
292+
auto& addPass(const char* name, Setup&& setup) {
293+
return addPass<Data>(name, std::forward<Setup>(setup),
294+
[](FrameGraphResources const&, auto const&, backend::DriverApi&) {
295+
});
296+
}
296297

297298
/**
298299
* Allocates concrete resources and culls unreferenced passes.
@@ -549,39 +550,24 @@ class FrameGraph {
549550
};
550551

551552
template<typename Data, typename Setup, typename Execute>
552-
FrameGraphPass<Data>& FrameGraph::addPass(char const* name, Setup setup, Execute&& execute) {
553+
auto& FrameGraph::addPass(char const* name, Setup setup, Execute&& execute) {
553554
static_assert(sizeof(Execute) < 2048, "Execute() lambda is capturing too much data.");
554555

555556
// create the FrameGraph pass
556-
auto* const pass = mArena.make<FrameGraphPassConcrete<Data, Execute>>(std::forward<Execute>(execute));
557+
auto* const pass = mArena.make<FrameGraphPass<Data, Execute>>(std::forward<Execute>(execute));
557558

558559
Builder builder(addPassInternal(name, pass));
559-
setup(builder, const_cast<Data&>(pass->getData()));
560560

561-
// return a reference to the pass to the user
562-
return *pass;
563-
}
564-
565-
template<typename Data, typename Setup>
566-
FrameGraphPass<Data>& FrameGraph::addPass(char const* name, Setup setup) {
567-
// create the FrameGraph pass without an execute stage
568-
auto* const pass = mArena.make<FrameGraphPass<Data>>();
569-
570-
Builder builder(addPassInternal(name, pass));
571-
setup(builder, const_cast<Data&>(pass->getData()));
561+
if constexpr (std::is_invocable_v<Setup, Builder&, Data&>) {
562+
setup(builder, const_cast<Data&>(pass->getData()));
563+
} else if constexpr (std::is_invocable_v<Setup, Builder&>) {
564+
setup(builder);
565+
}
572566

573567
// return a reference to the pass to the user
574568
return *pass;
575569
}
576570

577-
template<typename Execute>
578-
void FrameGraph::addTrivialSideEffectPass(char const* name, Execute&& execute) {
579-
addPass<Empty>(name, [](Builder& builder, auto&) { builder.sideEffect(); },
580-
[execute](FrameGraphResources const&, auto const&, backend::DriverApi& driver) {
581-
execute(driver);
582-
});
583-
}
584-
585571
template<typename RESOURCE>
586572
void FrameGraph::present(FrameGraphId<RESOURCE> input) {
587573
// present doesn't add any usage flags, only a dependency

‎filament/src/fg/FrameGraphPass.h‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323

2424
#include <utils/Allocator.h>
2525

26+
#include <type_traits>
27+
2628
namespace filament {
2729

2830
class FrameGraphPassExecutor {
@@ -53,42 +55,34 @@ class FrameGraphPassBase : protected FrameGraphPassExecutor {
5355
~FrameGraphPassBase() noexcept override;
5456
};
5557

56-
template<typename Data>
57-
class FrameGraphPass : public FrameGraphPassBase {
58-
friend class FrameGraph;
59-
60-
// allow our allocators to instantiate us
61-
template<typename, typename, typename, typename>
62-
friend class utils::Arena;
63-
64-
void execute(FrameGraphResources const&, backend::DriverApi&) noexcept override {}
65-
66-
protected:
67-
FrameGraphPass() = default;
68-
Data mData;
69-
70-
public:
71-
Data const& getData() const noexcept { return mData; }
72-
Data const* operator->() const { return &mData; }
73-
};
74-
7558
template<typename Data, typename Execute>
76-
class FrameGraphPassConcrete : public FrameGraphPass<Data> {
59+
class FrameGraphPass final : public FrameGraphPassBase {
7760
friend class FrameGraph;
7861

7962
// allow our allocators to instantiate us
8063
template<typename, typename, typename, typename>
8164
friend class utils::Arena;
8265

83-
explicit FrameGraphPassConcrete(Execute&& execute) noexcept
66+
explicit FrameGraphPass(Execute&& execute) noexcept
8467
: mExecute(std::move(execute)) {
8568
}
8669

87-
void execute(FrameGraphResources const& resources, backend::DriverApi& driver) noexcept final {
88-
mExecute(resources, this->mData, driver);
70+
void execute(FrameGraphResources const& resources,
71+
backend::DriverApi& driver) noexcept override {
72+
// execute can omit the DriverApi parameter
73+
if constexpr (std::is_invocable_v<Execute, FrameGraphResources const&, Data&>) {
74+
mExecute(resources, mData);
75+
} else {
76+
mExecute(resources, mData, driver);
77+
}
8978
}
9079

80+
Data mData;
9181
Execute mExecute;
82+
83+
public:
84+
Data const& getData() const noexcept { return mData; }
85+
Data const* operator->() const { return &mData; }
9286
};
9387

9488
} // namespace filament

‎filament/src/fg/details/PassNode.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626

2727
#include <backend/TargetBufferInfo.h>
2828

29+
#include <cstdint>
2930
#include <unordered_set>
31+
#include <vector>
3032

3133
namespace utils {
3234
class CString;
@@ -62,7 +64,7 @@ class PassNode : public DependencyGraph::Node {
6264
Vector<VirtualResource*> destroy; // resources we need to destroy after executing
6365
};
6466

65-
class RenderPassNode : public PassNode {
67+
class RenderPassNode final : public PassNode {
6668
public:
6769
class RenderPassData {
6870
public:
@@ -107,7 +109,7 @@ class RenderPassNode : public PassNode {
107109
std::vector<RenderPassData> mRenderTargetData;
108110
};
109111

110-
class PresentPassNode : public PassNode {
112+
class PresentPassNode final : public PassNode {
111113
public:
112114
explicit PresentPassNode(FrameGraph& fg) noexcept;
113115
PresentPassNode(PresentPassNode&& rhs) noexcept;

‎filament/src/fg/details/ResourceAllocator.h‎

Lines changed: 58 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "fg/details/ResourceCreationContext.h"
2020

2121
#include <fg/FrameGraphDummyLink.h>
22+
#include <fg/FrameGraphTexture.h>
2223

2324
#include <utils/StaticString.h>
2425

@@ -29,19 +30,38 @@
2930

3031
namespace filament {
3132

33+
// Helper to check if T::create(Args...) exists
34+
template<typename T, typename... Args>
35+
struct check_create {
36+
template<typename U>
37+
static auto test(int) -> decltype(
38+
std::declval<U&>().create(std::declval<Args>()...),
39+
std::true_type{});
40+
template<typename>
41+
static std::false_type test(...);
42+
static constexpr bool value = decltype(test<T>(0))::value;
43+
};
44+
45+
template<typename T, typename... Args>
46+
inline constexpr bool check_create_v = check_create<T, Args...>::value;
47+
3248
// A type-trait to check if a resource is a "backend" resource, i.e. if it has a
3349
// create() method that takes a DriverApi& as its first parameter.
34-
template<typename T, typename = void>
35-
struct is_backend_resource : std::false_type {};
36-
3750
template<typename T>
38-
struct is_backend_resource<T, std::void_t<decltype(
39-
std::declval<T&>().create(
40-
std::declval<backend::DriverApi&>(),
41-
std::declval<utils::StaticString>(),
42-
std::declval<typename T::Descriptor const&>(),
43-
std::declval<typename T::Usage>())
44-
)>> : std::true_type {};
51+
struct is_backend_resource : std::bool_constant<
52+
check_create_v<T,
53+
backend::DriverApi&,
54+
utils::StaticString,
55+
typename T::Descriptor const&,
56+
typename T::Usage> ||
57+
check_create_v<T,
58+
backend::DriverApi&,
59+
utils::StaticString,
60+
typename T::Descriptor const&> ||
61+
check_create_v<T,
62+
backend::DriverApi&,
63+
typename T::Descriptor const&>> {
64+
};
4565

4666
template<typename T>
4767
inline constexpr bool is_backend_resource_v = is_backend_resource<T>::value;
@@ -64,12 +84,37 @@ struct ResourceAllocator {
6484
T::Descriptor const& desc, T::Usage usage) {
6585
if constexpr (is_backend_resource_v<T>) {
6686
// This is a backend resource, pass the driver.
67-
resource.create(context.driver, std::move(name), desc, usage);
87+
if constexpr (check_create_v<T,
88+
backend::DriverApi&,
89+
utils::StaticString,
90+
typename T::Descriptor const&,
91+
typename T::Usage>) {
92+
resource.create(context.driver, name, desc, usage);
93+
} else if constexpr (check_create_v<T,
94+
backend::DriverApi&,
95+
utils::StaticString,
96+
typename T::Descriptor const&>) {
97+
resource.create(context.driver, name, desc);
98+
} else {
99+
resource.create(context.driver, desc);
100+
}
68101
} else {
69102
// This is a generic resource, call the simple version.
70-
resource.create(std::move(name), desc, usage);
103+
if constexpr (check_create_v<T,
104+
utils::StaticString,
105+
typename T::Descriptor const&,
106+
typename T::Usage>) {
107+
resource.create(name, desc, usage);
108+
} else if constexpr (check_create_v<T,
109+
utils::StaticString,
110+
typename T::Descriptor const&>) {
111+
resource.create(name, desc);
112+
} else {
113+
resource.create(desc);
114+
}
71115
}
72116
}
117+
73118
static void destroy(T& resource, ResourceCreationContext const& context) {
74119
if constexpr (is_backend_resource_v<T>) {
75120
// This is a backend resource, pass the driver.
@@ -96,7 +141,7 @@ struct ResourceAllocator<FrameGraphTexture> {
96141
// FIXME: I think we should restrict this to attachments and blit destinations only
97142
usage |= FrameGraphTexture::Usage::PROTECTED;
98143
}
99-
resource.create(context.getTextureCache(), std::move(name), desc, usage);
144+
resource.create(context.getTextureCache(), name, desc, usage);
100145
}
101146

102147
static void destroy(FrameGraphTexture& resource, ResourceCreationContext const& context) {

0 commit comments

Comments
 (0)