Move resolving #includes from MaterialBuilder to MaterialCompiler, before parsing the material.

- resolving #includes was happening after parsing, now moving before parsing.
this is because we could offload this resolution at build time for RuntimeMaterialCompiler
- filamat::resolveIncludes used to have an assumption where the given text was already
a shader block. this assumption is now broken so it finds the line offset internally.
thus the line offset field is removed from IncludeResult.

Author: trie94

libs/filamat/CMakeLists.txt

@@ -9,9 +9,11 @@ set(PUBLIC_HDR_DIR include)
 # ==================================================================================================
 set(HDRS
         include/filamat/Enums.h
+        include/filamat/Includes.h
         include/filamat/IncludeCallback.h
         include/filamat/MaterialBuilder.h
-        include/filamat/Package.h)
+        include/filamat/Package.h
+)
 
 set(COMMON_PRIVATE_HDRS
         src/eiff/Chunk.h
@@ -23,7 +25,6 @@ set(COMMON_PRIVATE_HDRS
         src/eiff/MaterialInterfaceBlockChunk.h
         src/eiff/ShaderEntry.h
         src/eiff/SimpleFieldChunk.h
-        src/Includes.h
         src/PushConstantDefinitions.h)
 
 set(COMMON_SRCS

libs/filamat/include/filamat/IncludeCallback.h

@@ -26,6 +26,7 @@ namespace filamat {
 struct IncludeResult {
     // The include name of the root file, as if it were being included.
     // I.e., 'foobar.h' in the case of #include "foobar.h"
+    // Only relevant when the ResolveOptions::insertLineDirectives is enabled.
     const utils::CString includeName;
 
     // The following fields should be filled out by the IncludeCallback when processing an include,
@@ -35,9 +36,6 @@ struct IncludeResult {
     // directives.
     utils::CString text;
 
-    // The line number for the first line of text (first line is 0).
-    size_t lineNumberOffset = 0;
-
     // The name of the include file. This gets passed as "includerName" for any includes inside of
     // source. This field isn't used by the include system; it's up to the callback to give meaning
     // to this value and interpret it accordingly.  In the case of DirIncluder, this is an empty

libs/filamat/src/Includes.h renamed to libs/filamat/include/filamat/Includes.h

@@ -17,10 +17,10 @@
 #ifndef TNT_FILAMAT_INCLUDES_H
 #define TNT_FILAMAT_INCLUDES_H
 
-#include <utils/CString.h>
-
 #include <filamat/IncludeCallback.h>
 
+#include <utils/CString.h>
+
 #include <vector>
 
 namespace filamat {
@@ -48,7 +48,7 @@ struct FoundInclude {
     utils::CString name;
     size_t startPosition;
     size_t length;
-    size_t line;        // the line number the include was found on (first line is 1)
+    size_t line;        // the line number the #include was found on.
 };
 
 std::vector<FoundInclude> parseForIncludes(const utils::CString& source);

libs/filamat/include/filamat/MaterialBuilder.h

@@ -384,18 +384,13 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase {
      * }
      * ~~~~~
      *
-     * @param code The source code of the material.
+     * @param code The source code of the material. Expected it to be all inlined. (#includes are
+     * resolved.)
      * @param line The line number offset of the material, where 0 is the first line. Used for error
      *             reporting
      */
     MaterialBuilder& material(const char* code, size_t line = 0) noexcept;
 
-    /**
-     * Set the callback used for resolving include directives.
-     * The default is no callback, which disallows all includes.
-     */
-    MaterialBuilder& includeCallback(IncludeCallback callback) noexcept;
-
     /**
      * Set the vertex code content of this material.
      *
@@ -419,7 +414,8 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase {
      * }
      * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-     * @param code The source code of the material.
+     * @param code The source code of the material. Expected it to be all inlined. (#includes are
+     * resolved.)
      * @param line The line number offset of the material, where 0 is the first line. Used for error
      *             reporting
      */
@@ -888,22 +884,16 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase {
     bool isLit() const noexcept { return mShading != filament::Shading::UNLIT; }
 
     utils::CString mMaterialName;
-    utils::CString mFileName;
     utils::CString mCompilationParameters;
 
     class ShaderCode {
     public:
         void setLineOffset(size_t offset) noexcept { mLineOffset = offset; }
-        void setUnresolved(const utils::CString& code) noexcept {
-            mIncludesResolved = false;
+        void setCode(const utils::CString& code) noexcept {
             mCode = code;
         }
 
-        // Resolve all the #include directives, returns true if successful.
-        bool resolveIncludes(IncludeCallback callback, const utils::CString& fileName) noexcept;
-
-        const utils::CString& getResolved() const noexcept {
-            assert(mIncludesResolved);
+        const utils::CString& getCode() const noexcept {
             return mCode;
         }
 
@@ -912,14 +902,11 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase {
     private:
         utils::CString mCode;
         size_t mLineOffset = 0;
-        bool mIncludesResolved = false;
     };
 
     ShaderCode mMaterialFragmentCode;
     ShaderCode mMaterialVertexCode;
 
-    IncludeCallback mIncludeCallback = nullptr;
-
     PropertyList mProperties;
     ParameterList mParameters;
     ConstantList mConstants;

libs/filamat/src/Includes.cpp

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "Includes.h"
+#include <filamat/Includes.h>
 
 #include <utils/Log.h>
 #include <utils/compiler.h>
@@ -83,10 +83,16 @@ bool resolveIncludes(IncludeResult& root, IncludeCallback callback,
         return false;
     }
 
-    const size_t lineNumberOffset = root.lineNumberOffset;
     utils::CString& text = root.text;
 
     std::vector<FoundInclude> includes = parseForIncludes(text);
+    if (includes.empty()) {
+        // No more to resolve.
+        return true;
+    }
+
+    const size_t lineNumberOffset = includes[0].line;
+
     bool sourceDirty = false;
 
     // If we weren't given an include name, use "0", which is default when no #line directives are
@@ -96,7 +102,7 @@ bool resolveIncludes(IncludeResult& root, IncludeCallback callback,
     auto insertLineDirective = [&options](utils::io::ostream& stream, size_t line, const char* filename) {
         if (options.insertLineDirectiveCheck) {
             stream << "#if defined(GL_GOOGLE_cpp_style_line_directive)\n";
-            // The #endif itself will count as a line, so subtact 1.
+            // The #endif itself will count as a line, so subtract 1.
             line--;
         }
         stream << "#line " << line << " \"" << filename << '\"';
@@ -128,9 +134,10 @@ bool resolveIncludes(IncludeResult& root, IncludeCallback callback,
         // 3
 
         // We want to insert a closing directive with a line number of 3.
-        // In this example, include.line is 2 and lineNumberOffset is 0.
+        // In this example, include.line is 2 and lineNumberOffset is 1. (Since the offset is the
+        // line number of the #include, thus it is inclusive)
         // So, the math works out as such:
-        const size_t lineDirectiveLine = include.line + lineNumberOffset + 1;
+        const size_t lineDirectiveLine = include.line + lineNumberOffset;
 
         utils::io::sstream closingDirective;
 
@@ -169,12 +176,13 @@ bool resolveIncludes(IncludeResult& root, IncludeCallback callback,
         sourceDirty = true;
     }
 
-    // Add a line directive on the first line for the root include.
+    // Add a line directive on the previous line for the root include.
     if (options.insertLineDirectives && depth == 0) {
         utils::io::sstream lineDirective;
-        insertLineDirective(lineDirective, lineNumberOffset + 1, rootIncludeName);
+        insertLineDirective(lineDirective, lineNumberOffset, rootIncludeName);
         lineDirective << '\n';
-        text.insert(0, utils::CString(lineDirective.c_str()));
+        const size_t startPos = includes[0].startPosition;
+        text.insert(startPos, utils::CString(lineDirective.c_str()));
         sourceDirty = true;
     }
 

libs/filamat/src/MaterialBuilder.cpp

@@ -20,10 +20,9 @@
 #include <filamat/IncludeCallback.h>
 #include <filamat/Package.h>
 
-#include "Includes.h"
+#include "GLSLPostProcessor.h"
 #include "MaterialVariants.h"
 #include "PushConstantDefinitions.h"
-#include "GLSLPostProcessor.h"
 
 #include "sca/GLSLTools.h"
 
@@ -226,29 +225,19 @@ MaterialBuilder& MaterialBuilder::name(const char* name) noexcept {
     return *this;
 }
 
-MaterialBuilder& MaterialBuilder::fileName(const char* name) noexcept {
-    mFileName = CString(name);
-    return *this;
-}
-
 MaterialBuilder& MaterialBuilder::compilationParameters(const char* params) noexcept {
     mCompilationParameters = CString(params);
     return *this;
 }
 
 MaterialBuilder& MaterialBuilder::material(const char* code, size_t const line) noexcept {
-    mMaterialFragmentCode.setUnresolved(CString(code));
+    mMaterialFragmentCode.setCode(CString(code));
     mMaterialFragmentCode.setLineOffset(line);
     return *this;
 }
 
-MaterialBuilder& MaterialBuilder::includeCallback(IncludeCallback callback) noexcept {
-    mIncludeCallback = std::move(callback);
-    return *this;
-}
-
 MaterialBuilder& MaterialBuilder::materialVertex(const char* code, size_t const line) noexcept {
-    mMaterialVertexCode.setUnresolved(CString(code));
+    mMaterialVertexCode.setCode(CString(code));
     mMaterialVertexCode.setLineOffset(line);
     return *this;
 }
@@ -650,7 +639,7 @@ bool MaterialBuilder::hasSamplerType(SamplerType const samplerType) const noexce
 void MaterialBuilder::prepareToBuild(MaterialInfo& info) noexcept {
     prepare(mEnableFramebufferFetch, mFeatureLevel);
 
-    const bool hasEmptyVertexCode = mMaterialVertexCode.getResolved().empty();
+    const bool hasEmptyVertexCode = mMaterialVertexCode.getCode().empty();
     const bool isPostProcessMaterial = mMaterialDomain == MaterialDomain::POST_PROCESS;
     // TODO: Currently, for surface materials, we rely on the presence of a custom vertex shader to
     // infer the default shader stages. We could do better by analyzing the AST of the vertex shader
@@ -850,29 +839,6 @@ bool MaterialBuilder::runSemanticAnalysis(MaterialInfo* inOutInfo,
     return success;
 }
 
-bool MaterialBuilder::ShaderCode::resolveIncludes(IncludeCallback callback,
-        const CString& fileName) noexcept {
-    if (!mCode.empty()) {
-        ResolveOptions const options {
-                .insertLineDirectives = true,
-                .insertLineDirectiveCheck = true
-        };
-        IncludeResult source {
-                .includeName = fileName,
-                .text = mCode,
-                .lineNumberOffset = getLineOffset(),
-                .name = CString("")
-        };
-        if (!filamat::resolveIncludes(source, std::move(callback), options)) {
-            return false;
-        }
-        mCode = source.text;
-    }
-
-    mIncludesResolved = true;
-    return true;
-}
-
 static void showErrorMessage(const char* materialName, filament::Variant const variant,
         MaterialBuilder::TargetApi const targetApi, backend::ShaderStage const shaderType,
         MaterialBuilder::FeatureLevel const featureLevel,
@@ -944,8 +910,8 @@ bool MaterialBuilder::generateShaders(JobSystem& jobSystem, const std::vector<Va
     // End: must be protected by lock
 
     ShaderGenerator sg(mProperties, mVariables, mOutputs, mDefines, mConstants, mPushConstants,
-            mMaterialFragmentCode.getResolved(), mMaterialFragmentCode.getLineOffset(),
-            mMaterialVertexCode.getResolved(), mMaterialVertexCode.getLineOffset(),
+            mMaterialFragmentCode.getCode(), mMaterialFragmentCode.getLineOffset(),
+            mMaterialVertexCode.getCode(), mMaterialVertexCode.getLineOffset(),
             mMaterialDomain);
 
     container.emplace<bool>(MaterialHasCustomDepthShader,
@@ -1317,12 +1283,6 @@ Package MaterialBuilder::build(JobSystem& jobSystem) {
 
     // TODO: maybe check MaterialDomain::COMPUTE has outputs
 
-    // Resolve all the #include directives within user code.
-    if (!mMaterialFragmentCode.resolveIncludes(mIncludeCallback, mFileName) ||
-        !mMaterialVertexCode.resolveIncludes(mIncludeCallback, mFileName)) {
-        goto error;
-    }
-
     if (mCustomSurfaceShading && mShading != Shading::LIT) {
         slog.e << "Error: customSurfaceShading can only be used with lit materials." << io::endl;
         goto error;
@@ -1533,7 +1493,7 @@ bool MaterialBuilder::hasCustomVaryings() const noexcept {
 }
 
 bool MaterialBuilder::needsStandardDepthProgram() const noexcept {
-    const bool hasEmptyVertexCode = mMaterialVertexCode.getResolved().empty();
+    const bool hasEmptyVertexCode = mMaterialVertexCode.getCode().empty();
     return !hasEmptyVertexCode ||
            hasCustomVaryings() ||
            mBlendingMode == BlendingMode::MASKED ||
@@ -1546,8 +1506,8 @@ std::string MaterialBuilder::peek(backend::ShaderStage const stage,
         const CodeGenParams& params, const PropertyList& properties) noexcept {
 
     ShaderGenerator const sg(properties, mVariables, mOutputs, mDefines, mConstants, mPushConstants,
-            mMaterialFragmentCode.getResolved(), mMaterialFragmentCode.getLineOffset(),
-            mMaterialVertexCode.getResolved(), mMaterialVertexCode.getLineOffset(),
+            mMaterialFragmentCode.getCode(), mMaterialFragmentCode.getLineOffset(),
+            mMaterialVertexCode.getCode(), mMaterialVertexCode.getLineOffset(),
             mMaterialDomain);
 
     MaterialInfo info;
@@ -1732,8 +1692,8 @@ void MaterialBuilder::writeCommonChunks(ChunkContainer& container, MaterialInfo&
     }
 
     // create a unique material id
-    auto const& vert = mMaterialVertexCode.getResolved();
-    auto const& frag = mMaterialFragmentCode.getResolved();
+    auto const& vert = mMaterialVertexCode.getCode();
+    auto const& frag = mMaterialFragmentCode.getCode();
     std::hash<std::string_view> const hasher;
     size_t const materialId = hash::combine(
             MATERIAL_VERSION,

libs/filamat/tests/MockIncluder.h

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "Includes.h"
+#include <filamat/Includes.h>
 
 #include <unordered_map>
 #include <string>

libs/filamat/tests/test_filamat.cpp

@@ -56,14 +56,12 @@ std::string shaderWithAllProperties(JobSystem& jobSystem, ShaderStage type,
     MockIncluder includer;
     includer
             .sourceForInclude("modify_normal.h", "material.normal = vec3(0.8);");
-
     filamat::MaterialBuilder builder;
     builder.material(fragmentCode.c_str());
     builder.materialVertex(vertexCode.c_str());
     builder.platform(filamat::MaterialBuilder::Platform::MOBILE);
     builder.optimization(filamat::MaterialBuilder::Optimization::NONE);
     builder.shading(shadingModel);
-    builder.includeCallback(includer);
     builder.refractionMode(refractionMode);
     builder.vertexDomain(vertexDomain);