Improved nanosvg integration (PR #22393)
tamasmeszaros
Added option wxUSE_NANOSVG similarly to other 3rdparty libs.
See PR for nanosvg as well: wxWidgets/nanosvg#1
The only thing missing here is the addition of nanosvg library into the list of libraries to link with wxWidgets for downstream projects. But that should only be necessary if wxUSE_NANOSVG=sys and the linked library is static. Donwstream projects can solve that by linking manually.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22393
Commit Summary
- 54b3fdd Improved nanosvg integration
File Changes
(6 files)
- M build/cmake/functions.cmake (2)
- M build/cmake/lib/CMakeLists.txt (2)
- M build/cmake/lib/core/CMakeLists.txt (5)
- A build/cmake/lib/nanosvg.cmake (16)
- M build/cmake/options.cmake (1)
- M src/generic/bmpsvg.cpp (23)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22393.patch
- https://github.com/wxWidgets/wxWidgets/pull/22393.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Marek Roszko
Just going to throw my 2 cents in, I am currently in the effort at vcpkg upstreaming updates to 3.1.6. nanosvg came up but the point is, nanosvg officially has no packaging, the author does not care to get into the business of it. The problem here I am commenting is you creating a completely arbitrary, non-existent namespaced library
nanosvg::nanosvgrast that is probably only available in somebody's random code library. In comparison, at vcpkg, because the author doesn't have a package, we are labeling it unofficial::nanosvg to avoid namespace squatting conflicts.
find_package(NanoSVG REQUIRED)
set(NANOSVG_LIBRARIES NanoSVG::nanosvgrast)
get_target_property(_nanosvg_incl NanoSVG::nanosvg INTERFACE_INCLUDE_DIRECTORIES)
set(NANOSVG_INCLUDE_DIRS ${_nanosvg_incl})
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
Hi @marekr ! Thank you for the remarks, you have a good point. Let's see what the upstream will react for the PR I've posted (if it does), and then subsequently what the wx fork will say. After looking at the build results here in this PR I see that this pack of change is not complete anyways and I probably will not have the resources to make it so.
I don't mind changing the namespace or creating only one ::nanosvg target, as long as some solution is accepted by at least the wx fork.
What is a bit lame is that if we namespace it with wxwidgets::nanosvg, then shouldn't the package name change also? I mean from find_package(NanoSVG) you would search for find_package(wxNanoSVG) or similar.
If you request a search for a package with a certain name, you expect that the exported targets and the namespace in which they reside is well defined and can be relied on. That is why I've posted PRs for both the upstream and for the wx fork. If there is no consensus, I guess we can't carry on with this solution.
I don't really have the answers here. It would be best to agree with the upstream.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent requested changes on this pull request.
Hi, thanks for the PR. It is a good start at making nanosvg a separate library instead of using it directly in wxWidgets.
After looking at the build results here in this PR I see that this pack of change is not complete anyways and I probably will not have the resources to make it so.
wxWidgets has basically 3 different build systems, that all have to be modified:
-
CMake
You already did this one, but it still has a build problem. It can't find the wxnanosvg library. Probably because it only has headers and no actual library is generated. -
Bakefiles
Bakefiles are used to generate the makefiles and vxproj files. -
MSVS and XCode project files
The newer vcxproj files are manually updated, and I think the pbxproj files are generated.
To keep things simple, I think we can focus only at CMake for now, and use the old method for the other build systems.
The implementation could then look like (not tested):
#define NANOSVG_ALL_COLOR_KEYWORDS #if wxHAS_NANOSVG #include <nanosvg.h> #include <nanosvgrast.h> #else #define NANOSVG_IMPLEMENTATION #define NANOSVGRAST_IMPLEMENTATION #include "../../3rdparty/nanosvg/src/nanosvg.h" #include "../../3rdparty/nanosvg/src/nanosvgrast.h" #endif
I am not sure if integrating wxWidgets/nanosvg#1 into the wxWidgets fork is a good idea. We don't use it ourselves, and it puts the burden of maintaining it on us.
The CMake target name is indeed another problem. Since nanosvg has no CMake support, there is no official namespace or target name (or even package name).
We could either check a list of known target names, and see if any of them is found. And package managers can request theirs to be added. Or use the more legacy way and expect NANOSVG_LIBRARIES and NANOSVG_INCLUDE_DIRS to be set.
I don't know if you're able to address these issues. I don't mind taking over and doing it myself.
In build/cmake/lib/core/CMakeLists.txt:
> + if(lib STREQUAL NANOSVG) + wx_lib_compile_definitions(wxbase PUBLIC wxHAS_NANOSVG) + endif()
This is targeting the base library instead of core. But these options should not be added as compile definitions, but via setup.h.
> @@ -114,6 +114,7 @@ wx_add_thirdparty_library(wxUSE_EXPAT EXPAT "use expat for XML parsing" DEFAULT_ wx_add_thirdparty_library(wxUSE_LIBJPEG JPEG "use libjpeg (JPEG file format)") 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_NANOSVG NanoSVG "use nanosvg for svg conversion")
Ideally these new options are added to setup.h(.in), like wxUSE_LIBTIFF so it is availble for all build systems. And then CMake can enable it when using external nanosvg.
> -
-// Try to help people updating their sources from Git and forgetting to
-// initialize new submodules, if possible: if you get this error, it means that
-// your source tree doesn't contain 3rdparty/nanosvg and you should initialize
-// and update the corresponding submodule.
-#ifdef __has_include
- #if ! __has_include("../../3rdparty/nanosvg/src/nanosvg.h")
- #error You need to run "git submodule update --init 3rdparty/nanosvg".
- #undef wxHAS_SVG
- #endif
-#endif // __has_include
-
-#endif // wxHAS_SVG
-
-#ifdef wxHAS_SVG
+#if defined(wxHAS_SVG) && defined(wxHAS_NANOSVG)
We still need #undef wxHAS_SVG if nanosvg is not available.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
It's fine to add extra features to CMake build system only, but we can't merge this PR as long as it simply breaks the build when not using CMake. I.e. you really need to make the changes here conditional in some way and ensure that the default build, using the bundled nanoSVG, still works, both with CMake and the other systems.
> -// Try to help people updating their sources from Git and forgetting to
-// initialize new submodules, if possible: if you get this error, it means that
-// your source tree doesn't contain 3rdparty/nanosvg and you should initialize
-// and update the corresponding submodule.
-#ifdef __has_include
- #if ! __has_include("../../3rdparty/nanosvg/src/nanosvg.h")
- #error You need to run "git submodule update --init 3rdparty/nanosvg".
- #undef wxHAS_SVG
- #endif
-#endif // __has_include
I'd like to keep this as the experience has shown it's a pretty common mistake. Of course, it could be made conditional on not using external library.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
I am trying out my proposed solution, and noticed another problem.
There is a difference between this solution and the one proposed for / used by vcpkg and also Hunter package manager.
This proposal expects an actual library of NanoSVG, while vcpkg and Hunter use a header-only interface library.
The header-only still needs below defines. But, if I understand your problem in the mailing list correctly, adding these defines causes 'function already defined' warnings for you? Because you link both wxWidgets and another NanoSVG statically into your application?
#define NANOSVG_IMPLEMENTATION #define NANOSVGRAST_IMPLEMENTATION
As a solution I thought of checking if a target has any libraries, and disable the defines in this case. I created a personal branch here (diff with master), let me know what you think and if it works for you. I can make a PR of it to replace this one.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
Hi @MaartenBent! As I see it now, this might be a lot of complication for a simple C library that is not actively maintained anyways. In PrusaSlicer we are probably going to bundle a modified version of the library that has the interface enclosed in our namespace (with some other mods as well). This way there can be no duplicate symbol errors (these are link errors unfortunately, not just warnings) and wxWidgets can have its version compiled into wx libs.
Still better this way than having multiple cmake packaging floating around the internet.
The approach of header-only library seems to me being wrong in case of nanosvg. It is not a C++ template library, it does not have its functions inlined, thus it really should be a traditional, compiled library.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
BTW I've finished checking your branch, it seems to work. I'm building wxWidgets with cmake only and having tests and examples disabled.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #22393.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
This is replaced by #22407, so closing. @tamasmeszaros Please test the other PR to check that it works for you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.