Scrolling waveform using SceneGraph in QML #13470
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
acolombier
changed the title
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
stale
|
Note that this is still being actively worked on with @m0dB and myself! |
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
3 times, most recently
from
d16b1dc to
23f331f
Compare
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
2 times, most recently
from
fe570b1 to
3d9deed
Compare
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
4 times, most recently
from
317256a to
8c71da3
Compare
|
I see this when trying to build it: |
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
from
8c71da3 to
a59fe88
Compare
|
Unfortunately, there are merge conflicts now. |
|
FYI, the PR is still broken :( |
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
from
d424088 to
141a5e2
Compare
|
@Holzhaus this should now be ready for some very early testing! |
|
@m0dB @acolombier I wonder if a clearer seperation between QML and QtQuick would be of benefit here, as we could use a QQuickWindow instead of the QOpenGLWindow in the existing QWidget GUI: https://doc.qt.io/qt-6/qtquick-embeddedinwidgets-example.html |
|
Hi, configuring works now. I'm getting the following build error though: [ 25%] Building CXX object CMakeFiles/mixxx-qml-lib.dir/src/qml/qmlstemsmodel.cpp.o
In file included from src/rendergraph/scenegraph/../common/rendergraph/assert.h:4,
from src/rendergraph/scenegraph/../common/rendergraph/material.h:6,
from src/rendergraph/scenegraph/../common/rendergraph/geometrynode.h:5,
from src/waveform/renderers/allshader/waveformrendermark.h:5,
from src/waveform/renderers/allshader/waveformrendermark.cpp:1:
src/waveform/renderers/allshader/waveformrendermark.cpp: In member function ‘void {anonymous}::WaveformMarkNode::update(float, float, float)’:
src/waveform/renderers/allshader/waveformrendermark.cpp:50:35: error: ‘RoundToPixel’ was not declared in this scope
50 | DEBUG_ASSERT(std::abs(x - RoundToPixel(devicePixelRatio)(x)) < epsilon);
| ^~~~~~~~~~~~
src/rendergraph/scenegraph/../common/rendergraph/../../../util/assert.h:59:32: note: in definition of macro ‘DEBUG_ASSERT’
59 | if (!static_cast<bool>(cond)) [[unlikely]] { \
| ^~~~
src/waveform/renderers/allshader/waveformrendermark.cpp:51:35: error: ‘RoundToPixel’ was not declared in this scope
51 | DEBUG_ASSERT(std::abs(y - RoundToPixel(devicePixelRatio)(y)) < epsilon);
| ^~~~~~~~~~~~
src/rendergraph/scenegraph/../common/rendergraph/../../../util/assert.h:59:32: note: in definition of macro ‘DEBUG_ASSERT’
59 | if (!static_cast<bool>(cond)) [[unlikely]] { \
| ^~~~
make[2]: *** [CMakeFiles/mixxx-qml-lib.dir/build.make:666: CMakeFiles/mixxx-qml-lib.dir/src/waveform/renderers/allshader/waveformrendermark.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:803: CMakeFiles/mixxx-qml-lib.dir/all] Error 2
make: *** [Makefile:166: all] Error 2 |
|
I fixed this in the rg-wr-… PR, so if @acolombier merges the latest commits of my branches it will be fixed here too. |
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
from
bb5c4f8 to
7e39d2f
Compare
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
5 times, most recently
from
3af3679 to
4b99a8b
Compare
|
@Swiftb0y I assume you are currently looking into this - you reacted with the 👁️ on the PR - thank you! There is still a lot to work on, but I'd been keen to hear your feedback on how to propagate properties for each renderers. Currently, there is two clashing design, and I will need to unify it into one, but I'm having a hard time to take a decision.
It feels like there is no perfect answers. Also QML is still quite unstable (note the number of changes made in this PR to consolidate it - happy to move some in other PR btw) so I don't think we should overly concern on stability yet with QML, as long as we don't introduce risks on legacy. Edit: keen to hear everyone feedback! I just meant to tag Niko to make sure you don't spend too much time writing a thorough review yet :D |
I'm actually not atm, I'm sorry my reaction gave that impression. I'll try to once the preliminary PRs have been merged. I have not looked enough into this PR to even understand the problem you're describing, so I can't provide any opinion. |
|
It gets much further in the build now, but now it fails with: In file included from src/waveform/renderers/allshader/waveformrenderersignalbase.h:8,
from src/waveform/renderers/allshader/waveformrendererrgb.h:5,
from src/waveform/renderers/allshader/waveformrendererrgb.cpp:1:
src/waveform/renderers/waveformrenderersignalbase.h:21:16: error: ‘virtual void WaveformRendererSignalBase::setup(const QDomNode&, const SkinContext&)’ was hidden [-Werror=overloaded-virtual=]
21 | virtual void setup(const QDomNode& node, const SkinContext& context);
| ^~~~~
src/waveform/renderers/allshader/waveformrendererrgb.h:24:10: note: by ‘void allshader_gl::WaveformRendererRGB::setup(const QColor&, const QColor&, const QColor&, const QColor&)’
24 | void setup(const QColor& axesColor,
| ^~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:8731: CMakeFiles/mixxx-lib.dir/src/waveform/renderers/allshader/waveformrendererrgb.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from src/waveform/renderers/allshader/waveformrenderersignalbase.h:8,
from src/waveform/widgets/allshader/waveformwidget.h:5,
from src/waveform/widgets/allshader/waveformwidget.cpp:2:
src/waveform/renderers/waveformrenderersignalbase.h:21:16: error: ‘virtual void WaveformRendererSignalBase::setup(const QDomNode&, const SkinContext&)’ was hidden [-Werror=overloaded-virtual=]
21 | virtual void setup(const QDomNode& node, const SkinContext& context);
| ^~~~~
In file included from src/waveform/widgets/allshader/waveformwidget.cpp:13:
src/waveform/renderers/allshader/waveformrendererrgb.h:24:10: note: by ‘void allshader_gl::WaveformRendererRGB::setup(const QColor&, const QColor&, const QColor&, const QColor&)’
24 | void setup(const QColor& axesColor,
| ^~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:8815: CMakeFiles/mixxx-lib.dir/src/waveform/widgets/allshader/waveformwidget.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:388: CMakeFiles/mixxx-lib.dir/all] Error 2
make: *** [Makefile:166: all] Error 2 |
|
Ok, I applied to following patch to work around the build error: diff --git a/src/qml/qmlwaveformrenderer.cpp b/src/qml/qmlwaveformrenderer.cpp
index 3095243945..1d368ebfd4 100644
--- a/src/qml/qmlwaveformrenderer.cpp
+++ b/src/qml/qmlwaveformrenderer.cpp
@@ -89,7 +89,7 @@ QmlWaveformRendererFactory::Renderer QmlWaveformRendererPreroll::create(
QmlWaveformRendererFactory::Renderer QmlWaveformRendererRGB::create(
WaveformWidgetRenderer* waveformWidget) const {
auto* renderer = new WaveformRendererRGB(waveformWidget, m_position, m_options, this);
- renderer->setup(
+ renderer->setupColors(
m_axesColor,
m_lowColor,
m_midColor,
diff --git a/src/waveform/renderers/allshader/waveformrendererrgb.cpp b/src/waveform/renderers/allshader/waveformrendererrgb.cpp
index 40b92d28e5..409232a66a 100644
--- a/src/waveform/renderers/allshader/waveformrendererrgb.cpp
+++ b/src/waveform/renderers/allshader/waveformrendererrgb.cpp
@@ -29,7 +29,7 @@ WaveformRendererRGB::WaveformRendererRGB(WaveformWidgetRenderer* waveformWidget,
setUsePreprocess(true);
}
-void WaveformRendererRGB::setup(const QColor& axesColor,
+void WaveformRendererRGB::setupColors(const QColor& axesColor,
const QColor& lowColor,
const QColor& midColor,
const QColor& highColor) {
diff --git a/src/waveform/renderers/allshader/waveformrendererrgb.h b/src/waveform/renderers/allshader/waveformrendererrgb.h
index acd06ec50f..47833edd0d 100644
--- a/src/waveform/renderers/allshader/waveformrendererrgb.h
+++ b/src/waveform/renderers/allshader/waveformrendererrgb.h
@@ -21,7 +21,7 @@ class allshader::WaveformRendererRGB final
// Pure virtual from WaveformRendererSignalBase, not used
void onSetup(const QDomNode& node) override;
- void setup(const QColor& axesColor,
+ void setupColors(const QColor& axesColor,
const QColor& lowColor,
const QColor& midColor,
const QColor& highColor);Now it crashes with a segfault: |
|
This fixes the crash: diff --git a/src/waveform/renderers/allshader/waveformrendererstem.cpp b/src/waveform/renderers/allshader/waveformrendererstem.cpp
index 2736d498b1..c39b8bb48b 100644
--- a/src/waveform/renderers/allshader/waveformrendererstem.cpp
+++ b/src/waveform/renderers/allshader/waveformrendererstem.cpp
@@ -32,6 +32,7 @@ void WaveformRendererStem::onSetup(const QDomNode&) {
}
bool WaveformRendererStem::init() {
+ WaveformRendererSignalBase::init();
auto group = m_pEQEnabled->getKey().group;
for (int stemIdx = 0; stemIdx < mixxx::kMaxSupportedStems; stemIdx++) {
QString stemGroup = EngineDeck::getGroupForStem(group, stemIdx); |
|
@acolombier Kudos, looks great! And the annoying flickering that we saw with the previous waveforms is also gone. Some minor issues I found:
Screencast.From.2025-02-03.22-50-43.mp4 |
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
from
4b99a8b to
ceaab87
Compare
acolombier
force-pushed
the
feat/qml-scrolling-waveform-with-sg
branch
from
ceaab87 to
02a2b3d
Compare
|
Thanks for you feedback @Holzhaus! Kudos to @m0dB to make that possible with the rendergraph!
Yes, this is due to some warning related to shadowing the base class method and your compiler seem to treat warning as error (
I'm actually still not completely sure how to use the scene graph clipping - Any pointer or fix for this @m0dB ?
Indeed, I can also reproduce it locally - it feels like the renderer are not synched for some reason (beats are responding faster than signal, itself responding faster than marker). I must say here again, I have no idea where to start and could definitely use some help @m0dB 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a rebase of @m0dB's
qml_scrolling_waveformon the latest mainThe waveform rendering approach should provided better result in the long run and should integrate with the new
allshaderapproach.Currently, the waveform rendering logic which happen in
QmlWaveformDisplay::updatePaintNodeis duplicated (or rather strongly inspired) from the RGB allshader rendering. A "definition of ready" for this PR would be to have some kind of compatibility layer/integration to reuse the rendering logic define inallshaderand should support at least one waveform type logic (e.gRGB)Depends on:
TODO: