Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rendergraph #13599

Draft
wants to merge 59 commits into
base: main
Choose a base branch
from
Draft

rendergraph #13599

wants to merge 59 commits into from

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Aug 25, 2024

rendergraph

rendergraph is an abstraction layer that can be compiled with one of two backends:

  • scenegraph (QtQuick scene graph classes)
  • opengl (custom classes using QOpenGL)

This abstraction layer follows the design of the QtQuick scene graph, with classes such as Material, Geometry, Node and GeometryNode, but it only gives access to and only uses a subset of its functionality.

The scenegraph backend class the underlying QtQuick scene graph classes directly or almost directly. The opengl layer implements classes that mimic the behaviour of the
Qt scene graph classes.

The objective of rendergraph is to be able to write code that can be used verbatim within the context of both QWidgets applications and QML applications. This includes using the same Vulkan-style GLSL shader code for both. (The QSGMaterialShader uses the qsb files, QOpenGLShader uses the OpenGL shader code generated by qsb)

Primarily, rendering with rendergraph uses a graph of Node's, typically of type GeometryNode. A GeometryNode uses a Geometry, which contains the vertex data of what the node renders (positional, texture coordinates, color information...) and a Material, which is the interface to the underlying shader. Uniform data is set through the Material. The QtQuick scene graph documentation is a good started point to understand the design.

The code is organized as follows:

common/rendergraph
Public header files. This is the primary API, identical for both backends, and the only files that should be included to write code compatible with both backends.

common
Implementation of the functionality declared in the common/rendergraph headers that is identical for both backends.
The general guideline is "code duplicated between the backends should be in the common layer".

common/rendergraph/material
Several basic 'materials' for common rendering operations (e.g. uniform or variying color, texture)

scenegraph
Implementation of the functionality declared in the common/rendergraph headers that is specific to call the scenegraph backend

scenegraph/rendergraph
Public header files specific for the opengl backend. This is the layer between QtQuick application code and the common rendergraph API.

scenegraph/backend
The scenegraph backend implementation, the layer between the common/rendergraph API and the underlying QtQuick scene graph classes.

opengl
Implementation of the functionality declared in the common/rendergraph headers that is specific to call the opengl backend

opengl/rendergraph
Public header files specific for the opengl backend. This is the layer between QWidgets/QOpenGL application code and the common rendergraph API.

opengl/backend
The opengl backend implementation, the layer between the common/rendergraph API and custom implemented classes that mimic the behaviour QtQuick scene graph classes.

Furthermore, some simple examples:

examples/sg_example
An example QtQuick application that uses the scenegraph backend

examples/gl_example
An example QOpenGLWindow based application that uses the opengl backend

examples/common
The graphics code that is identical for both example

../../res/shaders/rendergraph/
The qsb and (generated) gl shader files
See ../../res/shaders/rendergraph/README for more info

More advanced use of rendergraph can be found in the waveform/renderers/allshader classes, which have partially been ported the rendergraph.

rendergraph in the context of Mixxx

The objective of rendergraph in the context of Mixxx is to be able to have a single implementation of the waveforms rendering that can be used both in the QWidgets-based application and in the QML application.

This could also have been achieved by using the OpenGL code in a QQuickItem, but by using the scenegraph we will be able to run with other backends such as Vulkan, Metal and DirectX.

All allshader::::WaveformRenderer* classes have been ported to rendergraph nodes, except for the waveform signal rendering, of which only WaveformRendererRGB has been ported. The remaining nodes derive from rendergraph OpenGLNode, which is a transitional solution so they can be used in the rendergraph opengl backend (but not in the scenegraph backend)

TODO

  • port the remaining allshader::::WaveformRenderer* signal renderer classes.
  • Write a QQuickItem that uses the ported allshader::::WaveformRenderer* Node classes

@m0dB m0dB marked this pull request as draft August 25, 2024 19:21
@Swiftb0y
Copy link
Member

Thank you very much for this PR. As I've already mentioned in DMs, currently my biggest issues are the following:

  1. Usage of owning raw pointers (returned by rendergraph::Material::createShader implementations)
  2. constexpr all the things that makes sense (new vocabulary types, Attribute for example).
  3. re-evaluate ubiquitous use of the PIMPL idiom:
  4. Its primary advantage is providing better ABI stability as well the ability to hide its implementation. The former is only needed for big libraries while latter is only relevant to proprietary code. Neither constraints apply to us.
  5. Another advantage is that it reduces inter-header dependencies leading to faster recompiles. Doing this in a blanket fashion for the majority of all classes is overreaching IMO instead if should be done in select classes where needed because of the disadvantages:
  6. Disadvantage 1: Forces(!) an indirect call at any use, prohibits the compiler from doing most inlining.
  7. Disadvantage 2: Forces(!) dynamic memory allocation for the PIMPL pointer which is not always desired.
  8. Disadvantage 3: prohibits most constexpring most code because the impl can not live in the header.

@m0dB
Copy link
Contributor Author

m0dB commented Aug 26, 2024

Thank you very much for this PR. As I've already mentioned in DMs, currently my biggest issues are the following:

  1. Usage of owning raw pointers (returned by rendergraph::Material::createShader implementations)

Yes, this should be a unique_ptr, and it currently leaks. I will fix this.

  1. constexpr all the things that makes sense (new vocabulary types, Attribute for example).

Agreed. If you have time to change this, a PR would be appreciated!

  1. re-evaluate ubiquitous use of the PIMPL idiom:

Let me explain why I use the PIMPL idiom here. I want to have the same API for the OpenGL and for the scenegraph implementations. I could have done this by simply doing two implementations, each with their own header files exposing the same public API but with different private information. E.g. in the scenegraph version, GeometryNode could derive (privately) from QSGGeometryNode, while in the OpenGL version it obviously can't. However, that would make it harder to ensure that what is exposed by the two versions is publicly identical, especially during initial development.

My intention was to remove the PIMPLs once I am sure I have all functionality needed to implement all waveform renderers. But it might be better to do it already, as I am now past the POC stage and I don't think there is much left to be added. I agree that it does make the code harder to follow.

@Swiftb0y
Copy link
Member

Thanks for the explanation, that very much makes sense. I could think of a couple other ways to ensure the public APIs stay the same even without usage of the Pimpl idiom. One would be to have an pure virtual abstract class and inherit privately from it. The other would be to encode the API in a C++20 concept and assert both implementations adhere to it, etc.

@Swiftb0y
Copy link
Member

Agreed. If you have time to change this, a PR would be appreciated!

I will look into it, but it'll be better to do after removing the PIMPLs because dynamic memory allocation in constexpr code gets complicated.

@m0dB m0dB force-pushed the rendergraph branch 2 times, most recently from 2799436 to 118afd9 Compare September 1, 2024 20:54
@m0dB
Copy link
Contributor Author

m0dB commented Sep 1, 2024

@Swiftb0y the OpenGL implementation now doesn't use pimpls anymore. It certainly makes the code easier to follow!

I also have ported all allshader::WaveformRender* classes to rendergraph::Node , expect for the actual waveform rendering, but that should be straightforward. There is still some cleanup to be done in the WaveformRenderer code, but it so far it all works.

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 2, 2024

great. ping me when this is ready for review. Unfortunately this also got quite big quite fast, so do you see any way of splitting this up into more reviewable chunks again?

@m0dB m0dB force-pushed the rendergraph branch 3 times, most recently from 7c8899c to b4cbbe6 Compare September 21, 2024 18:08
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments:

@@ -0,0 +1,9 @@
Generate the GLSL shaders from the spirv shaders by running
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is that step required? Can you give some more context in the readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/rendergraph/common/basenode.cpp Outdated Show resolved Hide resolved
return std::move(m_pFirstChild);
}

std::unique_ptr<BaseNode> BaseNode::removeChildNode(BaseNode* pChild) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here, return value is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. And in fact here is an example, in waveform/renderers/allshader/waveformrendermark.cpp:

218     while (auto pChild = m_pMarkNodesParent->firstChild()) {
219         // Pop child from front of m_pMarkNodesParent
220         auto pRemoved = m_pMarkNodesParent->removeChildNode(pChild);

But I will add a comment explaining this use case.

Note btw that rendergraph is different from Qt scene graph. Qt scene graph uses raw pointers (also when assigning a QSGGeometry and QSGMaterial to a QSGGeometryNode) and optionally setting a boolean to transfer ownership. I much prefer using unique_ptrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2bf9952

Please resolve conversation if this is now clear.

int RGBMaterial::compare(const Material* other) const {
Q_ASSERT(other && type() == other->type());
const auto* otherCasted = static_cast<const RGBMaterial*>(other);
return otherCasted == this ? 0 : 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we do not return a bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from Qt scene graph.

See

https://doc.qt.io/qt-6/qsgmaterial.html#compare

I guess the question is: shouldn't this return -1 when appropriate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is a compare used for sorting. So you need to return -1 or 1 to "to minimize state changes"
(I don't have a clue ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I don't have a clue either and would need to dive into the Qt code to see what optimisation they achieve through this. I am ignoring it in the opengl backend.

Anyway, I am now wondering why I didn't simply do the pointer comparison in the scenegraph backend. And that makes we wonder why Qt isn't doing a pointer comparison internally (I guess because there are situations where there are more ideal comparison methods?).

Interestingly, this comes from the Qt example code:

int CustomMaterial::compare(const QSGMaterial *o) const
{
    Q_ASSERT(o && type() == o->type());
    const auto *other = static_cast<const CustomMaterial *>(o);
    return other == this ? 0 : 1; // ### TODO: compare state???
}

That TODO seems to indicate that who implemented this example didn't have a clue either :-D

Now, from another example:

    int compare(const QSGMaterial *m) const override
    {
        const NoisyMaterial *other = static_cast<const NoisyMaterial *>(m);

        if (int diff = int(state.color.rgb()) - int(other->state.color.rgb()))
            return diff;

        if (!state.texture || !other->state.texture)
            return state.texture ? 1 : -1;

        const qint64 diff = state.texture->comparisonKey() - other->state.texture->comparisonKey();
        return diff < 0 ? -1 : (diff > 0 ? 1 : 0);
    }

Note the cast to NoisyMaterial ! And indeed the Qt docs read

The this pointer and other is guaranteed to have the same [type](https://doc.qt.io/qt-6/qsgmaterial.html#type)()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 9d55ab3 I have removed all concrete material compares, and implemented a generic compare in rendergraph/Material::compare . Based on the Qt examples I think this is correct. The Qt examples need to compare concrete data, but rendergraph uses a generic cache for the uniform values, so I can leverage that.

please review and resolve this thread

using namespace rendergraph::backend;

bool Material::updateUniformsByteArray(QByteArray* buf) {
auto pThis = static_cast<rendergraph::Material*>(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already of type Material

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is of type rendergraph::backend::Material, we need to cast it to its derived class rendergraph::Material here. (A static cast is justified, because the backend class should only be instantiated as base class for the frontend class)

Now, this confusion raises the question: is using a namespace for the backend base classes a good idea? It might be more readable to use different class names, e.g. MaterialBackend or BackendMaterial, or BaseMaterial. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just disambiguate it from the backend by adding rendergraph::common?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea to use a different name for the base class like base "Base" or any more descriptive name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are already classes with that in the name though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 0a0ebdc I renamed BaseNode to TreeNode and replace the backend namespace with a Base.. prefix. It definitely makes things more clear.

Please resolve if you are okay with the change.

src/rendergraph/opengl/backend/geometrynode.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the long-term plan with this? Is rendergraph supposed to be a permanent abstraction layer or just a stopgap to transition the waveform code to something QML-like in the legacy skin system? I'm asking because if rendergraph is merely a temporary proxy I'm would have a much more relaxed view on its API design.

res/shaders/rendergraph/README Outdated Show resolved Hide resolved
res/shaders/rendergraph/texture.vert.qsb Outdated Show resolved Hide resolved
res/shaders/rendergraph/unicolor.frag Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
#version 440
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason for 440? could it also be lower (is there an advantage?) or higher (460 is up-to-date iirc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste from the qt example code. But I suppose it could be lower.

src/rendergraph/trash Outdated Show resolved Hide resolved
}

private:
const std::unique_ptr<backend::Texture> m_pTexture{};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the pointer or the texture be const? currently its just the pointer afaict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer. Its lifetime is tight to its owner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but thats not implying const. As already pointed out, this inhibits moves (and due to the unique_ptr) also copies. That's artificially limiting and could cause consumers to wrap this class in yet another unique_ptr to work around the immovability (even though there is no runtime polymorphism which would require it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I am already using Texture wrapped in a std::unique_ptr , and I don't see that as a problem.

See for example:

rendergraph::TextureMaterial
   void setTexture(std::unique_ptr<Texture> texture) 

This is the typical use of Texture, create it with std::make_unique<rendergraph::Texture>(context, img)) and pass the ownership to the material.

(Now, possibly it should be std::shared_ptr, but not now)

Maybe I am missing your point, but I don't see any advantage of moving Texture instances, I think it would make the code much less clear than using std::unique_ptr , with it's obvious move semantics.

(On a side note, the backend::Texture has to be a pointer. In the case of OpenGL we could simply store the QOpenGLTexture itself, but in the case of scenegraph, the QSGTexture* has to be created by the QQuickWindow (which is why we need to pass the context to the rendergraph::Texture constructor, unused in the case of the opengl backend)

Comment on lines 9 to 10
class Context;
class Texture;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a reader, I now question whether you meant to implement Context in this class and forgot or if its just a necessary forward declaration (as opposed to the unnecessary forward declaration of Texture).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will include context.h for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. and at other locations where forward declaration are done out of necessity (to avoid circular dependency), I added a comment. Please resolve if okay.

Comment on lines +9 to +11
const int size = sizeOf(uniform.m_type);
m_infos.push_back({uniform.m_type, offset});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless you're dealing with return values from std::make_* functions, you'll probably want to emplace_back instead to avoid the move

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct Info has const members, so emplace_back won't work. I know you will suggest I make them non const and add accessors, but I prefer the simplicity of the struct with const members here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually thats not what's preventing you from using emplace_back. for emplace_back you'll need to add a constructor to call with the forwarded arguments. In this case that probably makes sense because otherwise you will get a temporary and a full copy instead of a temporary+move (which you would usually get if you called push_back(element&&). If you add the ctor and use emplace_back, you will get the most efficient code (no temporary, move or copy, just constructed the value right where it needs to be in the container).

I'm not saying that we should start optimizing here, just wanted to share my own findings while looking into this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of my comments on the Attribute and AttributeSet apply here as well.

@acolombier acolombier self-requested a review September 22, 2024 11:30
@m0dB
Copy link
Contributor Author

m0dB commented Sep 22, 2024

whats the long-term plan with this? Is rendergraph supposed to be a permanent abstraction layer or just a stopgap to transition the waveform code to something QML-like in the legacy skin system? I'm asking because if rendergraph is merely a temporary proxy I'm would have a much more relaxed view on its API design.

The short-term plan is to have waveform rendering in the QML layer as functional and using the same source code verbatim as the waveform rendering the QWidget layer.

What I am doing with rendergraph in the waveform renderers could also have been done with Qt scene graph directly, but that would have resulted in two different implementations to do the same. (It would have been less work...). And I do see the rendergraph approach as a code quality improvement for the waveform renderers.

When we move all development to QML we can remove the layer and change the code to use Qt scene graph directly, which should be straightforward, since rendergraph follows the naming and design of Qt scene graph closely. So, to answer your question, yes, long term it is planned to be a temporary proxy. But at current rate of the QML version development, and considering that we keep adding improvements to the QWidget version, I expect that the two versions will coexist for quite some time. I do hope that having full featured waveforms will give the QML version a bit of a boost though.

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass of comment. This is looking good!

I have noticed a slight difference of look on the waveform. I believe the opacity attribute of the color doesn't let the back layer's color blending properly, buit this is just a wild guess!

With rendergraph
Screenshot from 2024-09-22 12-53-00

With main
Screenshot from 2024-09-22 12-55-21

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these material classes appears to introduce a fair amount of duplication. Could we consider using templates here? I'm not entirely sure what generics would look like when using string literal, but it looks like there could be some elegant way to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed some of the duplication (the compare method). I don't think the code will become more clear by reducing the remaining duplication with templates.

src/rendergraph/common/rendergraph/attribute.h Outdated Show resolved Hide resolved
Comment on lines 5 to 11
int MaterialShader::attributeLocation(int attributeIndex) const {
return m_attributeLocations[attributeIndex];
}

int MaterialShader::uniformLocation(int uniformIndex) const {
return m_uniformLocations[uniformIndex];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth to do some bound checks here? Or maybe at least add some DEBUG_ASSERT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add DEBUG_ASSERT.

void setLastModifiedByMaterial(Material* pMaterial);

static QString resource(const char* filename) {
return QString(":/shaders/rendergraph/") + QString(filename) + QString(".gl");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is QString::arg a better fit here? I guess this could help having a dynamic pattern as well (e.g to point to a folder of .gl files, instead of referencing .gl file in the QRC file)

Copy link
Contributor Author

@m0dB m0dB Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keeping it simple and stupid :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I am okay with using arg. I'll make the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardcoding the path here is suboptimal anyways because the library (rendergraph) makes assumptions about the structure of its consumers (mixxx), especially the assumption that it makes use of the Qt resource system (at least thats how I interpret the :/ prefix). This should at least be changeable at configure time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I am okay with using arg. I'll make the change.

don't forget to use QStringLiteral on the formatstring ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardcoding the path here is suboptimal anyways because the library (rendergraph) makes assumptions about the structure of its consumers (mixxx), especially the assumption that it makes use of the Qt resource system (at least thats how I interpret the :/ prefix). This should at least be changeable at configure time.

Let's worry about that when we deploy rendergraph outside of mixxx :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now using arg and QStringLiteral , please resolve if ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to have gotten lost somehow

src/rendergraph/scenegraph/backend/attributeset.cpp Outdated Show resolved Hide resolved

void MaterialShader::updateSampledImage(RenderState& state,
int binding,
QSGTexture** texture,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we consider returning a QSGTexture* here instead of passing a QSGTexture**? I don't think this function is used yet, so I cannot tell if the pointer reference is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is an override for a QSGMaterialShader method (and it is used, by the Qt scene graph).

https://doc.qt.io/qt-6/qsgmaterialshader.html#updateSampledImage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments. Please resolve if now clear.

src/rendergraph/scenegraph/backend/materialshader.h Outdated Show resolved Hide resolved
src/rendergraph/scenegraph/geometry.cpp Show resolved Hide resolved
src/rendergraph/trash Outdated Show resolved Hide resolved
@m0dB
Copy link
Contributor Author

m0dB commented Sep 22, 2024

First pass of comment. This is looking good!

I have noticed a slight difference of look on the waveform. I believe the opacity attribute of the color doesn't let the back layer's color blending properly, buit this is just a wild guess!

The waveform signal rendering has not been ported yet and is still using a opengl layer-only node that allows to put OpenGL code into the rendergraph node graph. I guess the blend function is wrong because rendergraph (as Qt scene graph) uses premultiplied alpha. But I will address this when I port the waveform signal rendering.

@Swiftb0y
Copy link
Member

I've looked a bit into constexpring some of this but I don't think its feasible for the QSG code (and by extension not the common layer on top). Most of the calls into native QSG code are stupid simple (Attribute::create for example) and could easily constexpr upstream, but they aren't. The code has also not been touched in ~8 years so I'm not sure if thats a Qt5 limitation or they're simply not interested in modernizing there. Regardless, we might still be able to shift some stuff to compile-time even without underlying support, but lets focus on something else for now.

@m0dB
Copy link
Contributor Author

m0dB commented Sep 22, 2024

I now use QShader to load the qsb files and extract the glsl shaders for Qt >= 6.6

@m0dB
Copy link
Contributor Author

m0dB commented Sep 24, 2024

Note that I simplified things a bit by moving struct Attribute inside opengl::BaseGeometry, to be more similar to QSGGeometry. This means that Attribute is now an opaque class and the AttributeSet is now initialised with a list of AttributeInit structs.

@m0dB
Copy link
Contributor Author

m0dB commented Sep 24, 2024

any idea what is wrong with precommit? If I run the command locally all is good.

@m0dB
Copy link
Contributor Author

m0dB commented Sep 24, 2024

Could you please resolve all conversations where appropriate? It is hard to keep track of things with so many conversation open.

@acolombier
Copy link
Member

any idea what is wrong with precommit? If I run the command locally all is good.

Did you try to run?

pre-commit run --show-diff-on-failure --color=always --from-ref upstream/main --to-ref HEAD

Looks like one of your commit didn't have the pre-commit refactor for some reason. Sometimes happens to me upon merge/rebase/amend too!

@Swiftb0y
Copy link
Member

went through my comments again and resolved everything I'm sure about. I didn't manage to go address the more complex ones though. I'm very much excited about the changes here, but I'm unfortunately quite short on time so I'm afraid I'll have to abandon my review for now.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just quickly spotted this one and wanted to comment so I don't forget later. I still highly appreciate that you're keeping this alive.

src/rendergraph/common/rendergraph/nodeinterface.h Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple more comments on things that I would like to improve or that weren't obvious to me.

src/rendergraph/common/rendergraph/assert.h Show resolved Hide resolved
src/rendergraph/common/rendergraph/attributeset.h Outdated Show resolved Hide resolved
src/rendergraph/common/rendergraph/attributeset.h Outdated Show resolved Hide resolved
src/rendergraph/common/rendergraph/geometry.h Outdated Show resolved Hide resolved
src/rendergraph/examples/sg_example/customitem.h Outdated Show resolved Hide resolved
src/rendergraph/opengl/backend/baseopenglnode.cpp Outdated Show resolved Hide resolved
src/rendergraph/opengl/backend/shadercache.h Outdated Show resolved Hide resolved
src/rendergraph/opengl/backend/shadercache.h Outdated Show resolved Hide resolved
src/rendergraph/opengl/engine.cpp Outdated Show resolved Hide resolved
src/rendergraph/opengl/backend/basegeometry.h Outdated Show resolved Hide resolved
@m0dB
Copy link
Contributor Author

m0dB commented Nov 2, 2024

@Swiftb0y I took the liberty of resolving the conversations where I took your suggestions verbatim or where there was no possible doubt. Could you please resolve the remaining conversations after reviewing the changes?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple comments from the new commits since the last one.

Comment on lines +16 to +17
return AttributeSet({(AttributeInit::create<Ts>())...},
std::vector<QString>(std::cbegin(names), std::cend(names)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to consider propagating this further, because the constructor also just copies stuff out of the vector to assemble another vector. So this vector is conceptually still just another temporary allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really care? This isn't exactly in the critical path!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not. I'll look into though and see if its worth the extra code.

src/rendergraph/opengl/CMakeLists.txt Show resolved Hide resolved
return GL_TRIANGLES;
case Geometry::DrawingMode::TriangleStrip:
case DrawingMode::TriangleStrip:
return GL_TRIANGLE_STRIP;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to add some default here, because the way enums are technically defined is a little weird...

Suggested change
}
}
DEBUG_ASSERT(!"unreachable");
return GL_TRIANGLES;

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 2, 2024

Could you please resolve the remaining conversations after reviewing the changes?

Done. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants