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.
Randalphwa
@Randalphwa pushed 4 commits.
- 6df1c93 Move common headers outside of luna conditionals
- 6a628eb Add wxlunasvg.lib to core if available
- 43ef62a Revert .circleci/config.yml, .cirrus.yml and src/zlib to state before de4ab3628 (keep other changes)
- 7d9fde1 Refactor to minimize duplication
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa pushed 1 commit.
- 2c3efd4 Move LoadSVGFile() out of anonymous namespace
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa pushed 1 commit.
- 931043a Add wxUSE_LUNASVG 1 to C++20 msw build
—
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:
I reverted the changes to the two CI files as well as this one.
—
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 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.ymland.github/workflows/ci.ymlto adduse_lunasvgparameter to the builds matrix there and use it if it is set to true? TIA!
I added #define wxUSE_LUNASVG to the msw build that uses c++20 since that way I knew it had the c++17 minimum compiler to build wxlunasvg. The Ubuntu 24.04 wxGTK UBSAN matrix also uses c++20, so if it makes sense to shoehorn the wxlunasvg builds into the cxx20 builds, I'll look into adding it to that matrix as well.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In 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 wxlunasvg)
Linking still fails for me. Because this should be LUNASVG, it needs to match the prefix of the LUNASVG_LIBRARIES and LUNASVG_INCLUDE_DIRS variables.
Do you use monolithic build if it succeeds for you?
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: __cdecl wxlunasvg::Bitmap::~Bitmap(void)" (??1Bitmap@wxlunasvg@@QEAA@XZ) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: unsigned char * __cdecl wxlunasvg::Bitmap::data(void)const " (?data@Bitmap@wxlunasvg@@QEBAPEAEXZ) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: int __cdecl wxlunasvg::Bitmap::width(void)const " (?width@Bitmap@wxlunasvg@@QEBAHXZ) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: int __cdecl wxlunasvg::Bitmap::height(void)const " (?height@Bitmap@wxlunasvg@@QEBAHXZ) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: int __cdecl wxlunasvg::Bitmap::stride(void)const " (?stride@Bitmap@wxlunasvg@@QEBAHXZ) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: static class std::unique_ptr<class wxlunasvg::Document,struct std::default_delete<class wxlunasvg::Document> > __cdecl wxlunasvg::Document::loadFromData(char const *)" (?loadFromData@Document@wxlunasvg@@SA?AV?$unique_ptr@VDocument@wxlunasvg@@U?$default_delete@VDocument@wxlunasvg@@@std@@@std@@PEBD@Z) referenced in function "public: __cdecl wxBitmapBundleImplSVG::wxBitmapBundleImplSVG(char const *,class wxSize const &)" (??0wxBitmapBundleImplSVG@@QEAA@PEBDAEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: static class std::unique_ptr<class wxlunasvg::Document,struct std::default_delete<class wxlunasvg::Document> > __cdecl wxlunasvg::Document::loadFromData(char const *,unsigned __int64)" (?loadFromData@Document@wxlunasvg@@SA?AV?$unique_ptr@VDocument@wxlunasvg@@U?$default_delete@VDocument@wxlunasvg@@@std@@@std@@PEBD_K@Z) referenced in function "public: __cdecl wxBitmapBundleImplSVG::wxBitmapBundleImplSVG(unsigned char const *,unsigned __int64,class wxSize const &)" (??0wxBitmapBundleImplSVG@@QEAA@PEBE_KAEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: class wxlunasvg::Bitmap __cdecl wxlunasvg::Document::renderToBitmap(int,int,unsigned int)const " (?renderToBitmap@Document@wxlunasvg@@QEBA?AVBitmap@2@HHI@Z) referenced in function "private: class wxBitmap __cdecl wxBitmapBundleImplSVG::DoRasterize(class wxSize const &)" (?DoRasterize@wxBitmapBundleImplSVG@@AEAA?AVwxBitmap@@AEBVwxSize@@@Z)
16>wxmsw33ud_core.lib(bmpsvg.obj) : error LNK2019: unresolved external symbol "public: __cdecl wxlunasvg::Document::~Document(void)" (??1Document@wxlunasvg@@QEAA@XZ) referenced in function "public: void * __cdecl wxlunasvg::Document::`scalar deleting destructor'(unsigned int)" (??_GDocument@wxlunasvg@@QEAAPEAXI@Z)
—
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.
- 53d4c4d Fix prefix for luna _LIBRRIES and _INCLUDE_DIRS
—
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 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 wxlunasvg)
My bad -- I still had lunasvg as a required wxWidgets component in my test app. Corrected version is incoming...
—
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)Unlike CMake, for makefile.* and *.vc(x)proj builds, there isn't a way to modify the setup.h file, which means there isn't a way on the command line to enable wxUSE_LUNASVG as well as checking for whether the the compiler is set to use C++17 or greater. So for those build systems, it will be up to the person customizing their build to change setup.h to enable wxUSE_LUNASVG as well as ensuring that "CXXFLAGS = /std:c++17" or later.
I can add documentation about how to enable wxlunasvg for these build systems, but I'm not sure where the best place to put that would be that someone could actually find. Any suggestions?
Aside from the above possible documentation and VZ letting me know if he still wants the bmpsvg.cpp split into two files, and whether or not to add building wxlunasvg to the Ubuntu 24.04 wxGTK UBSAN matrix, I believe this PR is complete.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25902)I can add documentation about how to enable wxlunasvg for these build systems, but I'm not sure where the best place to put that would be that someone could actually find. Any suggestions?
I'd say the best is to explain it in interface/wx/bmpbndl.h, i.e. add a section about configuring the build to use LunaSVG. This may not be the most logical place, but it should increase the probability of people finding this documentation.
—
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.
I agree that this looks good now, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25902)and whether or not to add building wxlunasvg to the Ubuntu 24.04 wxGTK UBSAN matrix, I believe this PR is complete.
If it's possible to easily make one of Unix builds use LunaSVG (I don't really care which one), please do it, but if not, I'm fine with merging this without it too.
Thanks again for all your work on this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25902)Oh, and I forgot to ask: would you like to preserve the existing commits or squash merge this entire PR as a single 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 pushed 2 commits.
- 580e25d Add brief comment about how to enable LunaSVG library
- 475b8a3 Set wxUSE_LUNASVG to 1 in cxx-20 Ubuntu build
—
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)Oh, and I forgot to ask: would you like to preserve the existing commits or squash merge this entire PR as a single commit?
Definitely squash and merge.
Brief description of what is needed to build the library added to interface/bmpbndl.h with a link to the CMake Overview.
I added what should enable wxUSE_LUNASVG in matrix.use_ubsan (ci.yml) since that matric also set the compiler to cxx20. That's the extent of my knowledge about modifying the Ubuntu builds, so if it doesn't work, and no one has suggestions about how to fix it, then I'll revert it.
—
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)Sorry for asking again: Do we really really need all those changes in the lunasvg sources (i.e., is that different from all other 3d party libraries)? This can make updating the module quite burdensome. And IMO, generally, the more difficult updating is, the chances of the module getting outdated quickly are so much higher than usual. Also, think of the third party packagers (e.g., vcpkg) which AFAIK vastly prefer the libraries using a single original library for their dependencies.
And if wxWidgets cannot use the lunasvg as is: Is what is needed to be done when updating the module documented somewhere?
—
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)Good point. I didn't look at the submodule before but it has quite a lot of, in my opinion unnecessary, changes.
- namespace change: I don't know why this is necessary and makes it impossible to use an external version. lunasvg headers are only included in
bmpsvg.cppso there is no chance of leakinglunasvgnamespace or headers into public headers. This change modifies all the files so can be a merge conflict nightmare. - fix build warnings: we also do this in other submodules, so that is fine. We could also silence them via
target_compile_definitions. Most of them seem simple float/double/int conversion warnings. - adding defines: I see
PLUTOVG_BUILD,PLUTOVG_BUILD_STATIC,LUNASVG_BUILDandLUNASVG_BUILD_STATIC. We can define this externally usingtarget_compile_definitionsand bakefile equivalent.
Personally I would only keep the warning fixes (until they are fixed upstream) and revert all the other changes.
I've updated the others submodules quite a few times, and keeping the number of changes to a minimum makes it a lot easier to merge updates into it. (though updating the makefile stuff (not relevant here) is usually the most work).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> + setup_h_file=$(find lib/wx/include -name setup.h) + if [ -n "$setup_h_file" ]; then + sed -i 's/#define wxUSE_LUNASVG 0/#define wxUSE_LUNASVG 1/' "$setup_h_file" + fi
Can't you just add a configure flag: wxCONFIGURE_OPTIONS="--with-nanosvg $wxCONFIGURE_FLAGS"
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In configure.ac:
> @@ -541,6 +541,7 @@ WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifica WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG) +WX_ARG_WITH(lunasvg, [ --with-lunasvg use LunaSVG for rasterizing SVG], wxUSE_LUNASVG)
You've only modified this configure.ac file. You'll need to run autogen.sh to update the configure file.
—
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)Sorry for asking again: Do we really really need all those changes in the lunasvg sources (i.e., is that different from all other 3d party libraries)? This can make updating the module quite burdensome. And IMO, generally, the more difficult updating is, the chances of the module getting outdated quickly are so much higher than usual. Also, think of the third party packagers (e.g., vcpkg) which AFAIK vastly prefer the libraries using a single original library for their dependencies.
And if wxWidgets cannot use the lunasvg as is: Is what is needed to be done when updating the module documented somewhere?
Currently with all but one of the graphics libraries, you can be fairly confident that an image written in that graphics format will be rendered correctly by wxWidgets. However, with SVG images this is not the case. Neither nanosvg or lunasvg fully implement the 1.2 SVG specification supported by browsers, commercial SVG editors, and some of the open source SVG editors like Inkscape. LunaSVG is much accurate than NanoSVG for the portions of the spec they both support, but I have literally looked at hundreds of SVG images that do not render correctly with either library.
Using the wxlunasvg namespace makes it possible to add on to the lunasvg library and support more of the SVG 1.2 spec. For example, hooking it up to wxWidgets itself makes it possible to support embedded graphics without duplicating the graphics libraries that wxWidgets already supports. Hooking it up to wxWidgets font support should make it possible to more accurately and efficiently render text that lunasvg partially supports.
If I revert it back to the lunasvg namespace, then neither I nor anyone else will be able to work on supporting more of the SVG 1.2 spec. As the only person so far who has expressed an interest in working on this, I would request keeping the wxlunasvg namespace for now. If I get to the point where I clearly am not going to be able to make any progress on this, then I can convert it to just the lunasvg namespace if we all agree that it will not be worked on by anyone else.
—
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)But why would this need a separate namespace? IMO it would be much better to keep support for package managers that already ship lunasvg (vcpkg, homebrew).
And contribute svg improvements to lunasvg directly, so we and everyone else can benefit from the improvements as soon as a new lunasvg version is released. Like the text rendering improvements in the last 2 releases. Without needing a new wxWidgets release.
I don't know how viable it is to use wxWidgets inside lunasvg to do text rendering (if I understand you correctly). You'll have a cyclic dependency of wxWidgets requiring lunasvg, and lunasvg requiring wxWidgets. But it can also cause text to be rendered differently on different platforms, due to differences in win32/cairo/coretext. While the goal of svg->bitmap should be that it looks the same on each platform.
—
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)With the same namespace, an application could link to both the wxWidgets library and an external copy of the 3rdparty lunasvg library used by the application. The order that you link to those libraries will determine which lunasvg symbols are used by the application -- and if it's the version that doesn't have the expanded SVG support, then your SVG images won't render correctly. The unique namespace means you have the option to use our version for wxWidgets rendering, and a different version used by the application itself for it's own purposes.
To use the lunasvg code in wxWidgets, the wxCore library needs symbols from the lunasvg library. Why would it be a problem for the linker if the lunasvg code also needed symbols from wxCore? I don't see why this library would be any different than any other of the wxWidgets libraries that depend on a different wxWidgets library.
"While the goal of svg->bitmap should be that it looks the same on each platform." -- this is not the case for SVG rendering of text. The SVG 1.2 specification treats missing‑font situations much like CSS does for HTML: the renderer must fall back to another font rather than simply refusing to draw the text.
This is the same as writing a wxWidgets application that sets the font of a text control which is only available on one platform. The difference is how font fallback is handled. wxFont fallback is handled very differently then lunasvg does. I could of course duplicate the wxFont fallback code in lunasvg, so that we had two somewhat identical implementations of font fallback in the wxWidgets codebase. Just as lunasvg may at some time duplicate wxWidgets graphics code to handle embedded images, and then everyone using wxWidgets and lunasvg can have two copies of jpeg/png/tiff decompression code in their applications. Bottom line is that sooner or later, it's very likely that lunasvg will be adding their own code that does the same thing as wxWidgets code, and then we either we have to figure out how to prevent that in wxWidgets builds, or ship with duplicate code.
"And contribute SVG improvements to lunasvg directly, so we and everyone else can benefit from the improvements as soon as a new lunasvg version is released. Like the text rendering improvements in the last 2 releases. Without needing a new wxWidgets release." Of course, I wouldn't do it any other way as long as wxWidgets itself doesn't already have the functionality needed. However, I cannot envision any scenario where the lunasvg maintainers would accept a PR that requires including the wxWidgets libraries.
I should probably mention that I have a vested interest in getting the DrawText SVG code working according to the SVG specification. I maintain a project that currently has 172 SVG files, all created by a commercial SVG editor. I have to use bitmaps for text rendering right now, even though it means that font isn't even close to correct on other platforms. There's other functionality that I also wish lunasvg supported -- but that doesn't require wxWidgets, so if I implement that it will get offered upstream to lunasvg.
—
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 configure.ac:
> @@ -541,6 +541,7 @@ WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifica WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG) +WX_ARG_WITH(lunasvg, [ --with-lunasvg use LunaSVG for rasterizing SVG], wxUSE_LUNASVG)
You've only modified this
configure.acfile. You'll need to runautogen.shto update theconfigurefile.
Thanks! I forgot about that setting. Hmm, but if I use wxCONFIGURE_OPTIONS="--with-lunasvg $wxCONFIGURE_FLAGS" wouldn't that set wxUSE_LUNASVG in the build while leaving it set to 0 in the setup.h file?
One of the items on my list of things to do with lunasvg is to add some SVG testing -- seems like I should bump up the priority on that while I'm dealing with non-cmake builds...
—
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 don't see why this library would be any different than any other of the wxWidgets libraries that depend on a different wxWidgets library
Because wx libraries have no cyclic dependency, see for example the dependency picture in https://docs.wxwidgets.org/trunk/page_libs.html.
You mention png, and its exported symbols have indeed a wx_ prefix. So it does make sense to do something similar to lunasvg. And I suppose changing namespace would do it. I can't think of any simpler, less intrusive way either.
Still, I'd like to see support for using system lunasvg as well, because it could be a nice upgrade from nanosvg. And we allow it for every third-party library.
You don't have to add it to this PR (maybe error if sys is used, or warning and force builtin).
But to prevent annoying cmake errors in the future, it would be beneficial if you could use wx_add_thirdparty_library (with default option builtin) instead of wx_option. Because the first one has allowed options builtin, sys and off, and the other has allowed options on and off.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In configure.ac:
> @@ -541,6 +541,7 @@ WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifica WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG) +WX_ARG_WITH(lunasvg, [ --with-lunasvg use LunaSVG for rasterizing SVG], wxUSE_LUNASVG)
I haven't used configure in a while, but I think it generates its own setup.h from setup.h.in.
I'll try it with lunasvg and report back.
Did you manually update the setup files or use build/update-setup-h?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ollydbg
asmwarrior left a comment (wxWidgets/wxWidgets#25902)Hello, I'd like to ask a question about how to modify the svg image at runtime. I mean the lunasvg library support modify or apply the styles, such as the colors can be changed, see here:
How to tweak the svg image, and later update bundle bitmap · Issue #1 · PBfordev/wxtestsvg2
So, the svg file is parsed once, and the styles can change many times. see:
https://github.com/sammycage/lunasvg?tab=readme-ov-file#dynamic-styling
So, is there any way to support this level of operation? Thanks.
—
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)Hello, I'd like to ask a question about how to modify the svg image at runtime. I mean the lunasvg library support modify or apply the styles, such as the colors can be changed, see here:
How to tweak the svg image, and later update bundle bitmap · Issue #1 · PBfordev/wxtestsvg2
So, the svg file is parsed once, and the styles can change many times. see:
https://github.com/sammycage/lunasvg?tab=readme-ov-file#dynamic-styling
So, is there any way to support this level of operation? Thanks.
I added it as a separate issue since it while lunasvg would support the style change, it would need to work with wxWidgets to have controls process it as per @PBfordev comment to that effect.
—
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 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.