add on review

src/rendergraph/CMakeLists.txt

@@ -5,6 +5,39 @@ if(QT_VERSION_MINOR GREATER_EQUAL 6)
   set(USE_QSHADER_FOR_GL ON)
 endif()
 
+set(
+  COMMON_RENDERGRAPH_FILES
+  ../common/rendergraph/attributeinit.h
+  ../common/rendergraph/attributeset.h
+  ../common/rendergraph/geometry.h
+  ../common/rendergraph/geometrynode.h
+  ../common/rendergraph/material.h
+  ../common/rendergraph/material/endoftrackmaterial.cpp
+  ../common/rendergraph/material/endoftrackmaterial.h
+  ../common/rendergraph/material/patternmaterial.cpp
+  ../common/rendergraph/material/patternmaterial.h
+  ../common/rendergraph/material/rgbamaterial.cpp
+  ../common/rendergraph/material/rgbamaterial.h
+  ../common/rendergraph/material/rgbmaterial.cpp
+  ../common/rendergraph/material/rgbmaterial.h
+  ../common/rendergraph/material/texturematerial.cpp
+  ../common/rendergraph/material/texturematerial.h
+  ../common/rendergraph/material/unicolormaterial.cpp
+  ../common/rendergraph/material/unicolormaterial.h
+  ../common/rendergraph/materialshader.h
+  ../common/rendergraph/materialtype.h
+  ../common/rendergraph/node.h
+  ../common/rendergraph/opacitynode.h
+  ../common/rendergraph/texture.h
+  ../common/rendergraph/types.h
+  ../common/rendergraph/uniform.h
+  ../common/rendergraph/uniformscache.cpp
+  ../common/rendergraph/uniformscache.h
+  ../common/rendergraph/uniformset.cpp
+  ../common/rendergraph/uniformset.h
+  ../common/types.cpp
+)
+
 add_subdirectory(opengl)
 add_subdirectory(scenegraph)
-add_subdirectory(../../res/shaders/rendergraph shaders)
+add_subdirectory(shaders)

src/rendergraph/common/rendergraph/assert.h

@@ -1,3 +1,4 @@
-// rendergraph is a module but we want to include util/assert.h from Mixxx,
-// without adding the entire src/ dir to the include paths.
+// Note: in principal, rendergraph is a module independent of Mixxx, but we
+// do want to include util/assert.h from Mixxx here, using a relative path
+// so we avoid adding the entire Mixxx src/ dir to the include paths.
 #include "../../../util/assert.h"

src/rendergraph/common/rendergraph/attributeinit.h

@@ -6,7 +6,7 @@ namespace rendergraph {
 struct AttributeInit;
 }
 
-// helper to create an AttributeSet using an initializer_list
+/// Helper to create an AttributeSet using an initializer_list.
 struct rendergraph::AttributeInit {
     int m_tupleSize;
     PrimitiveType m_primitiveType;

src/rendergraph/common/rendergraph/material.h

@@ -19,11 +19,11 @@ class rendergraph::Material : public rendergraph::BaseMaterial {
     Material(const UniformSet& uniformSet);
     virtual ~Material();
 
-    // see QSGMaterial::compare.
-    // TODO decide if this should be virtual. QSGMaterial::compare is virtual
-    // to concrete Material can implement a compare function, but in rendergraph
-    // we can compare the uniforms cache and texture already here, which seems
-    // sufficient.
+    /// See QSGMaterial::compare.
+    // Note: QSGMaterial::compare is virtual, so that a concrete Material can
+    // implement a custom compare function. But in rendergraph we can always
+    // compare the uniforms cache and texture already, and this is sufficient
+    // for our purpose.
     int compare(const Material* pOther) const {
         DEBUG_ASSERT(type() == pOther->type());
         int cacheCompareResult = std::memcmp(m_uniformsCache.data(),
@@ -32,7 +32,7 @@ class rendergraph::Material : public rendergraph::BaseMaterial {
         if (cacheCompareResult != 0) {
             return cacheCompareResult < 0 ? -1 : 1;
         }
-        // TODO multiple textures
+        // Note: we currently support only a single texture per material
         if (!texture(0) || !pOther->texture(0)) {
             return texture(0) ? 1 : -1;
         }

src/rendergraph/common/rendergraph/material/rgbamaterial.cpp

@@ -18,7 +18,8 @@ const AttributeSet& RGBAMaterial::attributes() {
     return set;
 }
 
-/* static */ const UniformSet& RGBAMaterial::uniforms() {
+// static
+const UniformSet& RGBAMaterial::uniforms() {
     static UniformSet set = makeUniformSet<QMatrix4x4>({"ubuf.matrix"});
     return set;
 }

src/rendergraph/opengl/CMakeLists.txt

@@ -1,34 +1,6 @@
 add_library(
   rendergraph_gl
-  ../common/rendergraph/attributeinit.h
-  ../common/rendergraph/attributeset.h
-  ../common/rendergraph/geometry.h
-  ../common/rendergraph/geometrynode.h
-  ../common/rendergraph/material.h
-  ../common/rendergraph/material/endoftrackmaterial.cpp
-  ../common/rendergraph/material/endoftrackmaterial.h
-  ../common/rendergraph/material/patternmaterial.cpp
-  ../common/rendergraph/material/patternmaterial.h
-  ../common/rendergraph/material/rgbamaterial.cpp
-  ../common/rendergraph/material/rgbamaterial.h
-  ../common/rendergraph/material/rgbmaterial.cpp
-  ../common/rendergraph/material/rgbmaterial.h
-  ../common/rendergraph/material/texturematerial.cpp
-  ../common/rendergraph/material/texturematerial.h
-  ../common/rendergraph/material/unicolormaterial.cpp
-  ../common/rendergraph/material/unicolormaterial.h
-  ../common/rendergraph/materialshader.h
-  ../common/rendergraph/materialtype.h
-  ../common/rendergraph/node.h
-  ../common/rendergraph/opacitynode.h
-  ../common/rendergraph/texture.h
-  ../common/rendergraph/types.h
-  ../common/rendergraph/uniform.h
-  ../common/rendergraph/uniformscache.cpp
-  ../common/rendergraph/uniformscache.h
-  ../common/rendergraph/uniformset.cpp
-  ../common/rendergraph/uniformset.h
-  ../common/types.cpp
+  ${COMMON_RENDERGRAPH_FILES}
   attributeset.cpp
   backend/baseattributeset.cpp
   backend/baseattributeset.h

src/rendergraph/opengl/backend/basegeometry.cpp

@@ -6,7 +6,7 @@
 using namespace rendergraph;
 
 namespace {
-// to mimic sg default
+// This mimics the scenegraph default.
 constexpr auto defaultDrawingMode = DrawingMode::TriangleStrip;
 } // namespace
 

src/rendergraph/opengl/backend/basegeometry.h

@@ -10,7 +10,7 @@ class BaseAttributeSet; // fwd decl to avoid circular dependency
 class BaseGeometry;
 } // namespace rendergraph
 
-// TODO this assumes all vertices consist of floats
+// Note: this assumes all vertices consist of floats, which is sufficient for our purposes
 class rendergraph::BaseGeometry {
   protected:
     BaseGeometry(const BaseAttributeSet& attributeSet, int vertexCount);

src/rendergraph/opengl/backend/basegeometrynode.cpp

@@ -40,8 +40,8 @@ void BaseGeometryNode::render() {
     }
 
     glEnable(GL_BLEND);
-    // qt scene graph uses premultiplied alpha color in the shader,
-    // so we need to do the same
+    // Note: Qt scenegraph uses premultiplied alpha color in the shader,
+    // so we need to do the same.
     glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
 
     QOpenGLShaderProgram& shader = material.shader();

src/rendergraph/opengl/backend/basegeometrynode.h

@@ -14,7 +14,7 @@ class rendergraph::BaseGeometryNode : public rendergraph::BaseNode,
     BaseGeometryNode() = default;
     virtual ~BaseGeometryNode() = default;
 
-    // called by Engine
+    // Called by Engine.
     void initialize() override;
     void render() override;
     void resize(int w, int h) override;

src/rendergraph/opengl/backend/basematerial.h

@@ -14,7 +14,7 @@ class rendergraph::BaseMaterial {
   public:
     virtual MaterialType* type() const = 0;
 
-    // For parity with QSGMaterial, not used yet
+    // For parity with QSGMaterial, not used yet.
     int compare(const BaseMaterial* other) const;
 
     void setShader(std::shared_ptr<MaterialShader> pShader);

src/rendergraph/opengl/backend/basenode.cpp

@@ -16,10 +16,10 @@ BaseNode::~BaseNode() {
     }
 }
 
-// This mimics QSGNode::appendChildNode.
-// Use NodeInterface<T>::appendChildNode(std::unique_ptr<BaseNode> pNode)
-// for a more clear transfer of ownership. pChild is considered owned by
-// this at this point.
+/// This mimics QSGNode::appendChildNode.
+/// Use NodeInterface<T>::appendChildNode(std::unique_ptr<BaseNode> pNode)
+/// for a more clear transfer of ownership. pChild is considered owned by
+/// this at this point.
 void BaseNode::appendChildNode(BaseNode* pChild) {
     if (m_pLastChild) {
         pChild->m_pPreviousSibling = m_pLastChild;
@@ -37,10 +37,10 @@ void BaseNode::appendChildNode(BaseNode* pChild) {
     }
 }
 
-// This mimics QSGNode::removeChildNode.
-// Use NodeInterface<T>::detachChildNode(BaseNode* pNode)
-// for a more clear transfer of ownership. Otherwise,
-// deleting pChild is responsibility of the caller.
+/// This mimics QSGNode::removeChildNode.
+/// Use NodeInterface<T>::detachChildNode(BaseNode* pNode)
+/// for a more clear transfer of ownership. Otherwise,
+/// deleting pChild is responsibility of the caller.
 void BaseNode::removeChildNode(BaseNode* pChild) {
     if (pChild == m_pFirstChild) {
         m_pFirstChild = pChild->m_pNextSibling;

src/rendergraph/opengl/backend/basenode.h

@@ -36,9 +36,11 @@ class rendergraph::BaseNode {
         return m_pEngine;
     }
 
-    // Prefer using NodeInterface<T>::appendChildNode(std::unique_ptr<BaseNode> pNode);
+    /// Mimicking scenegraph node API.
+    /// Prefer using NodeInterface<T>::appendChildNode(std::unique_ptr<BaseNode> pNode);
     void appendChildNode(BaseNode* pChild);
-    // Prefer using std::unique_ptr<BaseNode> NodeInterface<T>::detachChildNode(BaseNode* pNode);
+    /// Mimicking scenegraph node API.
+    /// Prefer using std::unique_ptr<BaseNode> NodeInterface<T>::detachChildNode(BaseNode* pNode);
     void removeChildNode(BaseNode* pChild);
 
     BaseNode* parent() const {
@@ -61,7 +63,7 @@ class rendergraph::BaseNode {
     Engine* m_pEngine{};
     bool m_usePreprocess{};
 
-    // Mimicking scenegraph node hierarchy. A parent owns its children.
+    /// Mimicking scenegraph node hierarchy. A parent owns its children.
     BaseNode* m_pParent{};
     BaseNode* m_pFirstChild{};
     BaseNode* m_pLastChild{};

src/rendergraph/opengl/geometry.cpp

@@ -10,7 +10,8 @@ Geometry::Geometry(const AttributeSet& attributeSet, int vertexCount)
 }
 
 void Geometry::setAttributeValues(int attributePosition, const float* from, int numTuples) {
-    // TODO this code assumes all vertices are floats
+    // Note: this code assumes all vertices are floats, which is sufficient for
+    // our purpose.
     VERIFY_OR_DEBUG_ASSERT(attributePosition < attributeCount()) {
         return;
     }

src/rendergraph/scenegraph/CMakeLists.txt

@@ -1,34 +1,6 @@
 add_library(
   rendergraph_sg
-  ../common/rendergraph/attributeinit.h
-  ../common/rendergraph/attributeset.h
-  ../common/rendergraph/geometry.h
-  ../common/rendergraph/geometrynode.h
-  ../common/rendergraph/material.h
-  ../common/rendergraph/material/endoftrackmaterial.cpp
-  ../common/rendergraph/material/endoftrackmaterial.h
-  ../common/rendergraph/material/patternmaterial.cpp
-  ../common/rendergraph/material/patternmaterial.h
-  ../common/rendergraph/material/rgbamaterial.cpp
-  ../common/rendergraph/material/rgbamaterial.h
-  ../common/rendergraph/material/rgbmaterial.cpp
-  ../common/rendergraph/material/rgbmaterial.h
-  ../common/rendergraph/material/texturematerial.cpp
-  ../common/rendergraph/material/texturematerial.h
-  ../common/rendergraph/material/unicolormaterial.cpp
-  ../common/rendergraph/material/unicolormaterial.h
-  ../common/rendergraph/materialshader.h
-  ../common/rendergraph/materialtype.h
-  ../common/rendergraph/node.h
-  ../common/rendergraph/opacitynode.h
-  ../common/rendergraph/texture.h
-  ../common/rendergraph/types.h
-  ../common/rendergraph/uniform.h
-  ../common/rendergraph/uniformscache.cpp
-  ../common/rendergraph/uniformscache.h
-  ../common/rendergraph/uniformset.cpp
-  ../common/rendergraph/uniformset.h
-  ../common/types.cpp
+  ${COMMON_RENDERGRAPH_FILES}
   attributeset.cpp
   backend/baseattributeset.cpp
   backend/baseattributeset.h

src/rendergraph/scenegraph/attributeset.cpp

@@ -6,5 +6,5 @@ using namespace rendergraph;
 
 AttributeSet::AttributeSet(std::initializer_list<AttributeInit> list, const std::vector<QString>&)
         : BaseAttributeSet(list) {
-    // names are not used in scenegraph
+    // names are not used in scenegraph, but needed in the API for the opengl backend
 }

src/rendergraph/scenegraph/backend/basematerial.cpp

@@ -18,8 +18,8 @@ int BaseMaterial::compare(const QSGMaterial* other) const {
 }
 
 QSGMaterialShader* BaseMaterial::createShader(QSGRendererInterface::RenderMode) const {
-    // This looks like a leak but it isn't: we pass ownership to Qt. Qt will
-    // cache and reuse the shader for all Material of the same type.
+    // Note: this looks like a leak but it isn't: we pass ownership to Qt.
+    // Qt will cache and reuse the shader for all Material of the same type.
     // TODO make sure that RenderMode is always the same.
     auto pThis = static_cast<const Material*>(this);
     auto pShader = pThis->createShader().release();

src/rendergraph/scenegraph/backend/basematerialshader.cpp

@@ -21,8 +21,8 @@ bool BaseMaterialShader::updateUniformData(RenderState& state,
     return result;
 }
 
-// override for QSGMaterialShader; this function is called by the Qt scene graph to prepare use of
-// sampled images in the shader, typically in the form of combined image samplers.
+/// Override for QSGMaterialShader; this function is called by the Qt scene graph to prepare use of
+/// sampled images in the shader, typically in the form of combined image samplers.
 void BaseMaterialShader::updateSampledImage(RenderState& state,
         int binding,
         QSGTexture** texture,

src/rendergraph/scenegraph/geometry.cpp

@@ -35,7 +35,8 @@ Geometry::Geometry(const rendergraph::AttributeSet& attributeSet, int vertexCoun
 }
 
 void Geometry::setAttributeValues(int attributePosition, const float* from, int numTuples) {
-    // TODO this code assumes all vertices are floats
+    // Note: this code assumes all vertices are floats, which is sufficient for our
+    // purpose.
     const auto attributeArray = QSGGeometry::attributes();
     int vertexOffset = 0;
     for (int i = 0; i < attributePosition; i++) {