Add wxLunaSVG rendering enginge library (PR #25902)
Randalphwa
This draft PR is related to issue #24216 -- it is designed to provide the option of replacing the NanoSVG rendering engine with the somewhat faster and definitely more accurate wxLunaSVG rendering engine.
By default, the wxWidgets build will not change, and NanoSVG will still be used. To replace it with wxLunaSVG when building with CMake, you need to do three things:
- Run git submodule update --init 3rdparty/lunasvg
- Set the wxBUILD_CXX_STANDARD to 17 or higher
- Change the wxUSE_LUNASVG option to ON.
While I have updated the necessary bakefiles, it's been a long time since I have worked with makefiles or vs project files, so I still need to look further into this to see how to get it to build under those build systems. Hence the PR being a draft...
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25902
Commit Summary
- 3728448 Add updated lunasvg submodule
- 6d5ff91 Add wxUSE_LUNASVG
- 93b11dc Add wxWidgets and lunasvg code
- de4ab36 Add configuration and docs
- f944a60 Add lunasvg submodule
- 9293aef Support USE_LUNASVG for building wxlunasvg library
- 32b72f6 Add wxlunasvg to bakefiles
File Changes
(28 files)
- M .circleci/config.yml (2)
- M .cirrus.yml (2)
- M .gitmodules (3)
- A 3rdparty/lunasvg (1)
- M build/autogen.mk (1)
- M build/bakefiles/common.bkl (2)
- M build/bakefiles/config.bkl (8)
- A build/bakefiles/lunasvg.bkl (66)
- M build/bakefiles/monolithic.bkl (3)
- M build/bakefiles/multilib.bkl (5)
- M build/cmake/lib/CMakeLists.txt (2)
- A build/cmake/lib/lunasvg.cmake (39)
- M build/cmake/options.cmake (2)
- M build/cmake/setup.h.in (2)
- M build/msw/config.gcc (3)
- M build/msw/config.vc (3)
- M configure.ac (24)
- M docs/doxygen/mainpages/const_wxusedef.h (1)
- M include/wx/features.h (2)
- M include/wx/gtk/setup.h (4)
- M include/wx/msw/setup.h (4)
- M include/wx/osx/setup.h (4)
- M include/wx/setup_inc.h (4)
- M include/wx/univ/setup.h (4)
- M interface/wx/bmpbndl.h (11)
- M setup.h.in (2)
- M src/generic/bmpsvg.cpp (245)
- M src/zlib (2)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25902.patch
- https://github.com/wxWidgets/wxWidgets/pull/25902.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25902)Looks good. I tried the toolbar sample (it uses svg images) and it seems to render everything correctly.
There was a link error in the CMake build, because you didn't link wxlunasvg to wxcore.
It seems LunaSVG can also be available as an external library, so you might want to use wx_add_thirdparty_library instead of wx_option. And then include <lunasvg.h> instead of "../../3rdparty/lunasvg/include/lunasvg.h".
I didn't look at the other build systems, but these would also need to be updated to have lunasvg.h in the include search path.
I came up with the following patch for CMake:
patchbuild/cmake/lib/core/CMakeLists.txt | 2 +- build/cmake/lib/lunasvg.cmake | 20 +++++++++++++++++++- build/cmake/options.cmake | 3 +-- src/generic/bmpsvg.cpp | 4 ++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/build/cmake/lib/core/CMakeLists.txt b/build/cmake/lib/core/CMakeLists.txt index f05f19a3f81..68f139b4d2f 100644 --- a/build/cmake/lib/core/CMakeLists.txt +++ b/build/cmake/lib/core/CMakeLists.txt @@ -60,7 +60,7 @@ elseif(WXX11) endif() wx_add_library(wxcore ${CORE_SRC}) -foreach(lib JPEG PNG TIFF NANOSVG WebP) +foreach(lib JPEG PNG TIFF NANOSVG WebP LUNASVG) if(${lib}_LIBRARIES) wx_lib_link_libraries(wxcore PRIVATE ${${lib}_LIBRARIES}) endif() diff --git a/build/cmake/lib/lunasvg.cmake b/build/cmake/lib/lunasvg.cmake index 36582a75284..b24e4ed791e 100644 --- a/build/cmake/lib/lunasvg.cmake +++ b/build/cmake/lib/lunasvg.cmake @@ -7,7 +7,23 @@ # Licence: wxWindows licence ############################################################################# -if(wxUSE_LUNASVG STREQUAL "ON" OR wxUSE_LUNASVG STREQUAL "builtin") +if(NOT wxUSE_LUNASVG STREQUAL "OFF") + if(NOT (CMAKE_CXX_STANDARD GREATER_EQUAL 17 OR wxHAVE_CXX17)) + message(FATAL_ERROR "LunaSVG requires at least C++17") + endif() +endif() + +if(wxUSE_LUNASVG STREQUAL "sys") + find_package(lunasvg) + if(NOT lunasvg_FOUND) + # If the sys library can not be found use builtin + wx_option_force_value(wxUSE_LUNASVG builtin) + else() + set(LUNASVG_LIBRARIES lunasvg::lunasvg) + endif() +endif() + +if(wxUSE_LUNASVG STREQUAL "builtin") wx_add_builtin_library(wxlunasvg 3rdparty/lunasvg/source/lunasvg.cpp 3rdparty/lunasvg/source/graphics.cpp @@ -36,4 +52,6 @@ if(wxUSE_LUNASVG STREQUAL "ON" OR wxUSE_LUNASVG STREQUAL "builtin") ${wxSOURCE_DIR}/3rdparty/lunasvg/include ${wxSOURCE_DIR}/3rdparty/lunasvg/plutovg/include ) + set(LUNASVG_LIBRARIES wxlunasvg) + set(LUNASVG_INCLUDE_DIRS ${wxSOURCE_DIR}/3rdparty/lunasvg/include) endif() diff --git a/build/cmake/options.cmake b/build/cmake/options.cmake index aa9daf6fce3..0a8dee31107 100644 --- a/build/cmake/options.cmake +++ b/build/cmake/options.cmake @@ -147,8 +147,7 @@ wx_add_thirdparty_library(wxUSE_LIBPNG PNG "use libpng (PNG image format)") wx_add_thirdparty_library(wxUSE_LIBTIFF TIFF "use libtiff (TIFF file format)") wx_add_thirdparty_library(wxUSE_LIBWEBP WebP "use libwebp (WebP file format)") wx_add_thirdparty_library(wxUSE_NANOSVG NanoSVG "use NanoSVG for rasterizing SVG" DEFAULT builtin) -wx_option(wxUSE_LUNASVG "use LunaSVG for rasterizing SVG (C++17 minimum)" OFF) -set(wxTHIRD_PARTY_LIBRARIES ${wxTHIRD_PARTY_LIBRARIES} wxUSE_LUNASVG "use LunaSVG for rasterizing SVG (C++17 minimum)") +wx_add_thirdparty_library(wxUSE_LUNASVG "use LunaSVG for rasterizing SVG" DEFAULT builtin) wx_option(wxUSE_LIBLZMA "use LZMA compression" OFF) set(wxTHIRD_PARTY_LIBRARIES ${wxTHIRD_PARTY_LIBRARIES} wxUSE_LIBLZMA "use liblzma for LZMA compression") diff --git a/src/generic/bmpsvg.cpp b/src/generic/bmpsvg.cpp index 39f0d600eca..d055155fee1 100644 --- a/src/generic/bmpsvg.cpp +++ b/src/generic/bmpsvg.cpp @@ -65,13 +65,13 @@ // your source tree doesn't contain 3rdparty/lunasvg and you should initialize // and update the corresponding submodule. #ifdef __has_include - #if ! __has_include("../../3rdparty/lunasvg/include/lunasvg.h") + #if ! __has_include("../../3rdparty/lunasvg/include/lunasvg.h") && ! __has_include(<lunasvg.h>) #error You need to run "git submodule update --init 3rdparty/lunasvg" from the wxWidgets directory. #undef wxHAS_SVG #endif #endif // __has_include -#include "../../3rdparty/lunasvg/include/lunasvg.h" +#include <lunasvg.h> class wxBitmapBundleImplSVG : public wxBitmapBundleImpl {
A few months ago I added a third party library as well (WebP), so if you encounter issues you can use that as example, or ask here for help.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25902)IMO, the code maintainability could be improved by removing the duplicate code existing for the two implementations.
I would factor out and share the common code:
- Common wx include headers
wxBitmapBundleImplSVGdeclaration and all its methods implementations (e.g., inlined) besides the ctor andDoRasterize()as well asm_sizeDefandm_cachedBitmapvariables- Most
wxBitmapBundle::From*()method definitions
If you want, I could give it a try, provided CMake build on MSW works.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa pushed 1 commit.
- d486f02 Generate FATAL_ERROR if building without C++17
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#25902)MaartenBent
Unlike the other graphics libraries (png, tiff, webp, etc.), the external lunasvg is not an exact replacement for wxlunasvg. Currently, the differences are minor, but I'm going to looking into further integrating this code with wxWidgets to provide some additional SVG functionality that is only partially available in the lunasvg library. As such, I want to leave it as wxlunasvg for now.
By default, wxlunasvg.lib is not built -- which means adding it to /build/cmake/lib/core/CMakeLists.txt causes the build to fail because the library doesn't exist. I'm puzzled about why you were getting a broken link, as I am not having any issue with that once both wxBUILD_CXX_STANDARD is set to 17 and wxUSE_LUNASVG is checked.
Currently it is possible for wxWidgets to use wxlunasvg and for an application to use lunasvg. That could be potentially useful if the lunasvg version adds functionality before it gets pulled into wxWidgets. As long as you build wxWidgets as a separate library or via a separate build, there shouldn't be an issue with two versions of <lunasvg.h>. However, for someone who is using FetchContent to pull in the wxWidgets code, my concern is ensuring that the two header files don't get mixed up. I have an app that does exactly this, so I'll look further into this to ensure that letting the compiler locate lunasvg.h instead of hard-coding the location the way nanosvg does above, will actually work.
I'm probably being dense here, but what is the purpose of:
set(LUNASVG_LIBRARIES wxlunasvg)
set(LUNASVG_INCLUDE_DIRS ${wxSOURCE_DIR}/3rdparty/lunasvg/include)
I grepped through all *.cpp, *.h, *.bkl, and *.cmake and it appears no one is using these?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#25902)PBfordev
Thanks for the suggestions!
I've moved both common headers and code to remove duplicates. I want to verify the changes with some apps I have that use both rendering engines before I check it in, probably not until this weekend.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25902)I'm probably being dense here, but what is the purpose of:
This is what links wxlunasvg to wxcore. Together with this modification in wxcore:
-foreach(lib JPEG PNG TIFF NANOSVG WebP) +foreach(lib JPEG PNG TIFF NANOSVG WebP LUNASVG)
it checks if LUNASVG_LIBRARIES and/or LUNASVG_INCLUDE_DIRS exists, and if they exist it will add the dependency to wxcore.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#25902)I'm probably being dense here, but what is the purpose of:
This is what links
wxlunasvgtowxcore. Together with this modification inwxcore:-foreach(lib JPEG PNG TIFF NANOSVG WebP) +foreach(lib JPEG PNG TIFF NANOSVG WebP LUNASVG)it checks if
LUNASVG_LIBRARIESand/orLUNASVG_INCLUDE_DIRSexists, and if they exist it will add the dependency towxcore.
I'm inclined to add something along the lines of:
if(NOT wxUSE_LUNASVG STREQUAL "OFF" AND (CMAKE_CXX_STANDARD GREATER_EQUAL 17 OR wxHAVE_CXX17)) set(HAVE_LUNASVG "WXLUNASVG")
endif() wx_add_library(wxcore ${CORE_SRC})
foreach(lib JPEG PNG TIFF NANOSVG WebP ${HAVE_LUNASVG})
That way it's clear that the library isn't always available.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25902)Any of the third party libraries might be unavailable, that's why there are checks inside the foreach loop. I prefer to handle all of the third party libraries the same instead of having this special check for lunasvg.
And keep all the lunasvg code inside the lunasvg cmake file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for your work on this!
I think we need at least 1, and preferably 2 (1 under Unix and 1 under Windows) CI jobs testing this to detect any breakage to non-default build. Could you please modify .github/workflows/ci_cmake.yml and .github/workflows/ci.yml to add use_lunasvg parameter to the builds matrix there and use it if it is set to true? TIA!
> @@ -34,7 +34,7 @@ jobs:
- run:
name: Checkout required submodules
command: |
- git submodule update --init 3rdparty/catch 3rdparty/nanosvg src/stc/scintilla src/stc/lexilla
+ git submodule update --init 3rdparty/catch 3rdparty/nanosvg 3rdparty/lunasvg src/stc/scintilla src/stc/lexilla
I'm not sure why do we do this if we don't use it during the build (which still uses NanoSVG) anyhow.
We probably ought to use LunaSVG in at least one of the builds, but it's probably going to be one of those running on GHA and not this one.
In .cirrus.yml:
> @@ -56,7 +56,7 @@ task: # Rather than getting all submodules, get just the ones we need, as we can # use system libraries instead of the other ones. update_submodules_script: | - git submodule update --init 3rdparty/catch 3rdparty/nanosvg src/stc/scintilla src/stc/lexilla + git submodule update --init 3rdparty/catch 3rdparty/nanosvg 3rdparty/lunasvg src/stc/scintilla src/stc/lexilla
Same comment as for the Circle CI change above.
I wonder if it might be worth it to split this in 2 different files (bmpnanosvg.cpp and bmplunasvg.cpp?).
On src/zlib:
Is the change to zlib intentional?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa commented on this pull request.
As per @PBfordev suggestion, I have refactored the code to pull out shared headers and code and placed them at the top of the file. The change will be in the next push to the PR. Once you see the refactored code, let me know if you still think we need to split the file -- though it would probably need to be 3 files, one for common, one for lunasvg, and one for nanosvg.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa commented on this pull request.
On src/zlib:
Yikes, no. Apparently I had git refresh the submodule accidentally, hence the commit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa commented on this pull request.
In .cirrus.yml:
> @@ -56,7 +56,7 @@ task: # Rather than getting all submodules, get just the ones we need, as we can # use system libraries instead of the other ones. update_submodules_script: | - git submodule update --init 3rdparty/catch 3rdparty/nanosvg src/stc/scintilla src/stc/lexilla + git submodule update --init 3rdparty/catch 3rdparty/nanosvg 3rdparty/lunasvg src/stc/scintilla src/stc/lexilla
I'll add the build CI jobs as requested, and modify when we pull in either the nanosvg or lunasvg modules based on the build. I haven't had a chance to look yet, but is there already a CI for testing against the C++17 compiler since we do have some conditionalized code in wxString that uses that? If so, would it make sense to add the wxlunasvg portion to that CI (or CIs) rather than creating separate CI jobs?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#25902)Any of the third party libraries might be unavailable, that's why there are checks inside the
foreachloop. I prefer to handle all of the third party libraries the same instead of having this special check for lunasvg. And keep all the lunasvg code inside the lunasvg cmake file.
Got it -- I've made the suggested changes and it will be in the next push to the PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.