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

Rework Crossover EQ plugin GUI #7781

Merged
merged 8 commits into from
Mar 20, 2025
Merged

Conversation

rubiefawn
Copy link
Contributor

@rubiefawn rubiefawn commented Mar 14, 2025

Replaces the .png raster assets for the native Crossover EQ plugin with .svg vector assets. Additionally, this PR reworks the entire layout of the Crossover EQ since it was kind of busted. Now it looks better, stuff no longer overlaps, and it uses the new mute buttons.

image
image

Note

Part of #7767

Mixers have screwed up alignment which needs to be resolved first
@rubiefawn
Copy link
Contributor Author

I appear to have reverted my changes to the wrong file, so now instead of a layout refactor this PR is just an extra newline on an unrelated file. 🤦‍♀️ I'll mark this as ready once I redo it, I guess

@rubiefawn rubiefawn marked this pull request as draft March 14, 2025 23:37
@rubiefawn rubiefawn marked this pull request as ready for review March 15, 2025 02:15
lol. lmao even.
Co-authored-by: Tres Finocchiaro <[email protected]>
@tresf tresf mentioned this pull request Mar 15, 2025
5 tasks
@rubiefawn rubiefawn linked an issue Mar 15, 2025 that may be closed by this pull request
1 task
@rubiefawn
Copy link
Contributor Author

Crossover EQ still does not display the loudness of the bands. I'd like to do that and also add sample-exactness to the controls (so they can be automated without tons of crackling) in a separate PR. This PR can just focus on fixing the GUI layout.

@regulus79
Copy link
Contributor

I have not reviewed the code, but I tested it and it appears to work and look fine.

@tresf
Copy link
Member

tresf commented Mar 16, 2025

This is fantastic work. As mentioned here #7767 (comment) I'd like to see the QPixmap workarounds moved to a shared location prior to merging if possible.

@tresf
Copy link
Member

tresf commented Mar 17, 2025

This is fantastic work. As mentioned here #7767 (comment) I'd like to see the QPixmap workarounds moved to a shared location prior to merging if possible.

After following my own advice and attempting this, I came across some technical challenges. In short, to avoid unnecessary complexity and computation, the size really does belong in the constructor.

That's not to say we can't simplify this a bit still. Here's a diff that would move the computation to a helper, which I think is the best we can do without drawbacks.

crossover_eq_logicalSize.diff.txt

Click to preview diff
diff --git a/include/embed.h b/include/embed.h
index 40a3622c6..1ddcdbefa 100644
--- a/include/embed.h

+++ b/include/embed.h
@@ -51,6 +51,20 @@ auto LMMS_EXPORT getIconPixmap(std::string_view name,
 	int width = -1, int height = -1, const char* const* xpm = nullptr) -> QPixmap;
 auto LMMS_EXPORT getText(std::string_view name) -> QString;
 
+/**
+ * @brief logicalSize is a backwards-compatible adapter for
+ * QPixmap::deviceIndependentSize.
+ * @param pixmap The pixmap to get the size of.
+ * @return The device-independent size of the pixmap.
+ */
+inline auto LMMS_EXPORT logicalSize(const QPixmap& pixmap) {
+#if QT_VERSION >= QT_VERSION_CHECK(6, 2, 0)
+    return pixmap.deviceIndependentSize().toSize();
+#else
+    return pixmap.isNull() ? QSize() : pixmap.size() / pixmap.devicePixelRatio();
+#endif
+}
+
 } // namespace embed
 
 class PixmapLoader
diff --git a/src/gui/widgets/Fader.cpp b/src/gui/widgets/Fader.cpp
index a1324016e..c8c240e9a 100644
--- a/src/gui/widgets/Fader.cpp
+++ b/src/gui/widgets/Fader.cpp
@@ -73,7 +73,7 @@ SimpleTextFloat* Fader::s_textFloat = nullptr;
 Fader::Fader(FloatModel* model, const QString& name, QWidget* parent, bool modelIsLinear) :
 	QWidget(parent),
 	FloatModelView(model, this),
-	m_knobSize(m_knob.size() / m_knob.devicePixelRatio()),
+	m_knobSize(embed::logicalSize(m_knob)),
 	m_modelIsLinear(modelIsLinear)
 {
 	if (s_textFloat == nullptr)
diff --git a/src/gui/widgets/PixmapButton.cpp b/src/gui/widgets/PixmapButton.cpp
index bd683ed71..5858f3b8c 100644
--- a/src/gui/widgets/PixmapButton.cpp
+++ b/src/gui/widgets/PixmapButton.cpp
@@ -107,7 +107,7 @@ void PixmapButton::mouseDoubleClickEvent( QMouseEvent * _me )
 void PixmapButton::setActiveGraphic( const QPixmap & _pm )
 {
 	m_activePixmap = _pm;
-	resize(m_activePixmap.size() / m_activePixmap.devicePixelRatio());
+	resize(embed::logicalSize(m_activePixmap));
 }
 
 
@@ -129,9 +129,7 @@ QSize PixmapButton::sizeHint() const
 
 QSize PixmapButton::minimumSizeHint() const
 {
-	const auto activeSize = m_activePixmap.size() / m_activePixmap.devicePixelRatio();
-	const auto inactiveSize = m_inactivePixmap.size() / m_inactivePixmap.devicePixelRatio();
-	return activeSize.expandedTo(inactiveSize);
+	return embed::logicalSize(m_activePixmap).expandedTo(embed::logicalSize(m_inactivePixmap));
 }
 
 bool PixmapButton::isActive() const

Introduces a temporary shim function to calculate a pixmap's device-
independent size for Qt versions < 6.2.
@rubiefawn
Copy link
Contributor Author

I've added the helper function as requested, with the caveat that it will show deprecation warnings when being compiled with Qt versions >= 6.2, since QPixmap::deviceIndependentSize() is available. Ideally this helper function should be removed once it's no longer needed.

@rubiefawn rubiefawn force-pushed the feat/crossovereq-svg branch from 1a613e1 to 8b16a62 Compare March 18, 2025 19:29
@bratpeki bratpeki self-assigned this Mar 18, 2025
@tresf
Copy link
Member

tresf commented Mar 18, 2025

I've added the helper function as requested, with the caveat that it will show deprecation warnings when being compiled with Qt versions >= 6.2, since QPixmap::deviceIndependentSize() is available. Ideally this helper function should be removed once it's no longer needed.

Thanks!

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

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

Tested saving a Crossover EQ from previous versions of LMMS and reloading , no regressions found. Due to the number of lines changed, I think a code review should be done as well.

@bratpeki
Copy link
Member

Lovely PR! Taking a look now.

@bratpeki
Copy link
Member

Tested visually and with some white noise and a spectrum analyzer, and it's still functioning. LGTM!

@tresf
Copy link
Member

tresf commented Mar 19, 2025

I've reviewed the code. The lambdas look much cleaner and reduce code duplication. The only thing that stands out is that some translation strings will have to be updated since the help text has been slightly changed.

I've also noticed that the plugin uses both "gain" and "volume" to describe the DBFS, which seems like a bit of bitrot, but the "Volume" label wasn't touched in this refactor, so I'm fine leaving it as-is.

@tresf tresf merged commit 953f6b7 into LMMS:master Mar 20, 2025
10 checks passed
@rubiefawn rubiefawn deleted the feat/crossovereq-svg branch March 20, 2025 05:50
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.

4 participants