I slighly remember that the png I made where slighly different then the SVG (optimized shadows, etc.). But not sure if this is still the case and if it matters. Just something to dbl-check.

Source: bitcoin-core/packaging#27 (comment)

Isn't SVG rendering a lot of complexity to add? What's the benefit?

If we want to use SVGs in the source code, we could adopt Knots's build system changes that render SVG to PNG at dist-time.

@luke-jr

Isn't SVG rendering a lot of complexity to add?

How does SVG rendering complexity differ from QIcon complexity with multiple pixmaps, icon engines and rescaling?

What's the benefit?

Unconditional visual quality.

@jonasschnelli

As you're a designer of bitcoin.svg, mind looking at this PR? 🌴

How does SVG rendering complexity differ from QIcon complexity with multiple pixmaps, icon engines and rescaling?

Choosing an image is absolutely trivial in comparison to rendering a vector image...

Why would we ever be rescaling?

What's the benefit?

Unconditional visual quality.

You get the same quality no matter what stage you render.

Choosing an image is absolutely trivial in comparison to rendering a vector image...

Isn't rendering a vector image hidden by qtsvg for us?

Why would we ever be rescaling?

In cases when a requested pixmap size does not coincide with one of stored by QIcon.

You get the same quality no matter what stage you render.

The difference in quality is obvious on screenshots though.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• build: improve sed robustness by not using sed #19761 (build: improve sed robustness by not using sed by fanquake)
• build: Qt 5.15.x #19716 (build: Qt 5.15.x by fanquake)
• build: Add Qt version checking #19689 (build: Add Qt version checking by hebasto)
• Replace boost::filesystem with std::filesystem #19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)
• wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
• build: Fix Qt processing of configure script for depends with DEBUG=1 #18298 (build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto)
• BIP-325: Signet [consensus] #18267 (BIP-325: Signet [consensus] by kallewoof)
• net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Isn't rendering a vector image hidden by qtsvg for us?

Yes, the concern is importing something complex like qtsvg into the overall codebase/runtime process... Any vulnerability compromises the entire program.

I cases when a requested pixmap size does not coincide with one of stored by QIcon.

I don't know of any cases where that should happen.

The difference in quality is obvious on screenshots though.

Most of the screenshots here are the DE, not Bitcoin Core...? Providing SVGs for that purpose seems fine and aside from using SVGs inside Core itself.

If we're getting scaling artefacts inside Core, we obviously aren't rendering to the size we're using. That's fixed by adjusting what sizes we render.

Is the GUI repo the appropriate home for a PR like this? :)

@luke-jr

Most of the screenshots here are the DE, not Bitcoin Core...? Providing SVGs for that purpose seems fine and aside from using SVGs inside Core itself.

Correct. bitcoin-core/gui#38 or #15068 should be used instead.

@practicalswift

Is the GUI repo the appropriate home for a PR like this? :)

https://github.com/bitcoin-core/gui is for changes within src/qt only. But this PR touches build-aux/m4/bitcoin_qt.m4, depends/packages/qt.mk, and src/Makefile.qt.include.

Closing.

Given SVG files are auditable (it's only text), and we don't accept SVG from any untrusted source, I don't think this is a good concern @luke-jr

Concept ACK if this worked out of the box without the build system changes.

Concept ACK if this worked out of the box without the build system changes.

The build system changes are required for static builds.