rendergraph: add rendergraph library #14007

m0dB · 2024-12-09T23:39:23Z

Part of the rendergraph PR split into multiple PRs.

See https://mixxx.zulipchat.com/#narrow/channel/109171-development/topic/rendergraph.20PR.20split/near/487124048

…inked yet)

Holzhaus

Thanks for the PR. I started reviewing (see my comments) but then figured I'm likely not the right person to review this because I wasn't involved in the prior discussion and don't want to reopen discussion that you potentially already agreed upon. So it doesn't really make sense for me to review this further.

My general impression is that this code is quite complex and I'm a bit worried that it's more complex than it needs to be. But again, if you think this is needed that is okay for me because I wasn't involved in prior discussion and also don't know what the follow up PRs look like (might also be that you create the needed foundation for them).

Holzhaus · 2024-12-10T00:23:20Z

src/rendergraph/common/rendergraph/attributeinit.h

+struct AttributeInit;
+}
+
+// helper to create an AttributeSet using an initializer_list


Suggested change

// helper to create an AttributeSet using an initializer_list

/// Helper to create an AttributeSet using an initializer_list.

Holzhaus · 2024-12-10T00:24:52Z

src/rendergraph/common/rendergraph/material.h

+    // 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.


Should this use triple slash?

I should probably not leave TODOs!

Is this TODO solved somewhere in the following planned PRs? Otherwise, perhaps worth creating an issue and reference it there so we keep track and context of it?

Holzhaus · 2024-12-10T00:27:33Z

src/rendergraph/common/rendergraph/assert.h

@@ -0,0 +1,3 @@
+// rendergraph is a module but we want to include util/assert.h from Mixxx,
+// without adding the entire src/ dir to the include paths.
+#include "../../../util/assert.h"


Not a fan of the relative import from the parent dir. We don't do that anywhere else. Not sure if there is a better way.

It’s a bit ugly, but I didn’t want to add the entire src/ dir to the include paths.

I'm wondering if we could get CMake to copy assert.h into the rendergraph include? CMake noob so I have no idea if it could work

I really wouldn't go down that path. This is isolated and commented.

Holzhaus · 2024-12-10T00:32:40Z

src/rendergraph/opengl/CMakeLists.txt

+  ../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


This is repeated multiple times. Can't we define a RENDERGRAPH_COMMON_HEADERS variable in the parent dir and then reference this instead of copy/paste?

Holzhaus · 2024-12-10T00:33:48Z

src/rendergraph/opengl/backend/basenode.cpp

+// 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.


Suggested change

// 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.

And elsewhere.

Holzhaus · 2024-12-10T00:37:33Z

src/rendergraph/opengl/rendergraph/openglnode.h

+#pragma once
+
+#include "backend/baseopenglnode.h"
+
+namespace rendergraph {
+using OpenGLNode = BaseOpenGLNode;
+} // namespace rendergraph


There are many files with less than 10 lines. Are you going to extend these files in the subsequent PRs? Because this makes the code harder to follows and reeks of YAGNI.

This is a consequence of the design of having two backend implementations, and a thin common API to interface with either. I don’t see away around this with the current design.

Holzhaus · 2024-12-10T00:39:16Z

src/rendergraph/CMakeLists.txt

+
+add_subdirectory(opengl)
+add_subdirectory(scenegraph)
+add_subdirectory(../../res/shaders/rendergraph shaders)


Isn't using a parent directory kind of the opposite of what add_subdirectory is supposed to do?

The alternative is to move the rendergraph shaders to src/rendergraph/shaders. I am not against that.

m0dB · 2024-12-10T19:44:08Z

Thanks for the review, Jan @Holzhaus. You are right that there has been a lot of prior discussion and reviews and the current state is the end result after many iterations. Still, your comments are welcome.

As for the complexity, I don’t think it is that complex. What makes it seem more complex than it is, is having two implementations of the same API, one using OpenGL and one using Qt scenegraph. But if you look past that: the scenegraph layer is extremely thin, and the OpenGL classes are pretty straightforward, especially if you are familiar with the Qt scenegraph design.

Anyway, any complexity here is compensated in the waveform renders code, which become much cleaner by having a very simple and standard way of doing things: You render a tree of nodes, typically GeometryNode, each with a Material of choice and a Geometry that you populate. Completely hiding all the OpenGL boilerplate, shaders, bindings, etc.

I think that when you review the waveformrenderer code you will appreciate this. And of course the fact that the same code will run within QML!

acolombier

This is looking good, no major issue, only bunch of nits. Let me know if you would like me to approve anyway.

acolombier · 2024-12-12T18:21:51Z

src/rendergraph/README

Should this be a README.md? Arguably it doesn't have much formatting, but that might play nicer into some IDE or on Windows which uses the extension to guess the filename

acolombier · 2024-12-12T18:23:15Z

src/rendergraph/common/rendergraph/assert.h

@@ -0,0 +1,3 @@
+// rendergraph is a module but we want to include util/assert.h from Mixxx,
+// without adding the entire src/ dir to the include paths.
+#include "../../../util/assert.h"


I'm wondering if we could get CMake to copy assert.h into the rendergraph include? CMake noob so I have no idea if it could work

acolombier · 2024-12-12T18:25:26Z

src/rendergraph/common/rendergraph/material.h

+    // 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.


Is this TODO solved somewhere in the following planned PRs? Otherwise, perhaps worth creating an issue and reference it there so we keep track and context of it?

acolombier · 2024-12-12T18:25:39Z

src/rendergraph/common/rendergraph/material.h

+        if (cacheCompareResult != 0) {
+            return cacheCompareResult < 0 ? -1 : 1;
+        }
+        // TODO multiple textures


I edited the comments.

acolombier · 2024-12-12T18:27:42Z

src/rendergraph/common/rendergraph/material.h

+    // we can compare the uniforms cache and texture already here, which seems
+    // sufficient.
+    int compare(const Material* pOther) const {
+        DEBUG_ASSERT(type() == pOther->type());


Do we want to early return here in case this is not the same type? I guess we might want to do that if m_uniformsCache and pOther->m_uniformsCache have different size which could lead to out of bound access, right?

acolombier · 2024-12-12T18:46:45Z

src/rendergraph/opengl/engine.cpp

+#include "rendergraph/engine.h"
+
+#include <QDebug>
+#include <cassert>


Shall we replace those with DEBUG_ASSERT/VERIFY_OR_DEBUG_ASSERT?

acolombier · 2024-12-12T18:54:53Z

src/rendergraph/opengl/texture.cpp

+    /* TODO rendergraph ASK @acolombier to try if the following works as well
+     * (added the .copy())
+    if (image.format() == QImage::Format_RGBA8888_Premultiplied) {
+        return QImage(image.bits(), image.width(), image.height(), QImage::Format_RGBA8888).copy();
+    }
+    return QImage(
+            image.convertToFormat(QImage::Format_RGBA8888_Premultiplied)
+                    .bits(),
+            image.width(),
+            image.height(),
+            QImage::Format_RGBA8888).copy();
+    */


Looking at the code and the doc, I think this should work! Do you want to just get in and we will fix forward if turns out not to?

No, I prefer to leave it like this and check once all PRs are in and you can rebase your qml code.

acolombier · 2024-12-12T18:55:57Z

src/rendergraph/scenegraph/backend/basematerialshader.cpp

+        QSGTexture** texture,
+        QSGMaterial* newMaterial,
+        QSGMaterial* oldMaterial) {
+    if (!newMaterial || !static_cast<Material*>(newMaterial)->texture(binding)) {


Shall we add a VERIFY_OR_DEBUG_ASSERT to prevent the nullptr exception?

acolombier · 2024-12-12T18:56:40Z

src/rendergraph/scenegraph/geometry.cpp

+    case DrawingMode::TriangleStrip:
+        return QSGGeometry::DrawTriangleStrip;
+    default:
+        throw std::runtime_error("not implemented");


Ditto C++ exception

acolombier · 2024-12-12T18:57:25Z

src/rendergraph/scenegraph/geometry.cpp

+    while (numTuples--) {
+        int k = tupleSize;
+        while (k--) {
+            *to++ = *from++;


Ditto nullptr exception

m0dB · 2024-12-12T20:04:01Z

Thanks. I prefer to merge as is, and when all rendergraph code is merged do a follow up PR to address the things spotted here. When you merge this I will create the next PR.

(For the next two PRs goes the same: unless you spot something particularly bad or broken, let's merge anyway and keep polishing / nitpicking for the end. After that, there are the PRs for the waveform render nodes, these can be done in parallel, so any changes don't require me to rebase the rest)

m0dB · 2024-12-14T04:42:50Z

I improved some of the comments and removed the common source/headers list duplication. I prefer to do the rest of @acolombier 's (valid) comments to be done in a follow up PR, once all rendergraph PRs are in.

acolombier

LGTM!

Let's get the render-graph train started 🚄

m0dB · 2024-12-14T18:08:55Z

Thanks! I have created the next PR #14021

Swiftb0y

review I started by never finished. Better late than never, feel free to address in a follow up whenever you find time.

Swiftb0y · 2024-12-10T14:02:58Z

src/rendergraph/scenegraph/texture.cpp

+Texture::Texture(Context* pContext, const QImage& image)
+        : m_pTexture(pContext->window()->createTextureFromImage(image)) {
+    VERIFY_OR_DEBUG_ASSERT(pContext->window() != nullptr) {
+        return;
+    }
+    DEBUG_ASSERT(!m_pTexture->textureSize().isNull());


I don't think this works properly. The ctor initializer list is evaluated before the ctor body, so if pContext->window() == nullptr the code would've crashed before it got to the check. Its also difficult to model the error case: afaik if an error occurs in a class constructor, the proper C++ solution is to throw. That ensures the object which is now in an unknown state (since the ctor didn't complete) can not be used. Since we refuse to use exceptions, the only other alternative would be to introduce a validity state into the object (eg if Texture couldn't get properly constructed set m_pTexture to nullptr and require the caller to verify that it isn't invalid).

EDIT: I also just noticed that rendergraph::Context::window returns a gsl::not_null anyways, so we could just remove the check, right?

Swiftb0y · 2024-12-10T14:06:48Z

src/rendergraph/scenegraph/materialshader.cpp

+        const UniformSet& uniformSet,
+        const AttributeSet& attributeSet) {
+    (void)uniformSet;
+    (void)attributeSet;


NIT:

Suggested change

const UniformSet& uniformSet,

const AttributeSet& attributeSet) {

(void)uniformSet;

(void)attributeSet;

const UniformSet&,

const AttributeSet&) {

Swiftb0y · 2024-12-14T18:25:37Z

src/rendergraph/opengl/backend/basegeometrynode.cpp

+    case DrawingMode::TriangleStrip:
+        return GL_TRIANGLE_STRIP;
+    default:
+        throw std::runtime_error("not implemented");


I think the consensus was that using exceptions currently will result in brittle code (because you never know what throws and when) so the consensus was to avoid exceptions currently and instead use std::expected. In this case a dummy value or a std::optional could work.

rendergraph library with opengl and scenegraph implementations (not l…

b9080f1

…inked yet)

github-actions bot added the build label Dec 9, 2024

Holzhaus reviewed Dec 10, 2024

View reviewed changes

acolombier reviewed Dec 12, 2024

View reviewed changes

add on review

89bf0a2

acolombier approved these changes Dec 14, 2024

View reviewed changes

acolombier merged commit c1160ff into mixxxdj:main Dec 14, 2024
13 checks passed

m0dB mentioned this pull request Dec 14, 2024

rendergraph: use opengl node and add shaders #14021

Merged

Swiftb0y reviewed Dec 14, 2024

View reviewed changes

	// helper to create an AttributeSet using an initializer_list
	/// Helper to create an AttributeSet using an initializer_list.

rendergraph: add rendergraph library #14007

rendergraph: add rendergraph library #14007

Conversation

m0dB commented Dec 9, 2024 • edited Loading

Holzhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented Dec 10, 2024

acolombier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented Dec 12, 2024

m0dB commented Dec 14, 2024

acolombier left a comment

Choose a reason for hiding this comment

m0dB commented Dec 14, 2024

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented Dec 9, 2024 •

edited

Loading

acolombier left a comment •

edited

Loading