Add LunaSVG as an optional replacement for NanoSVG #24475
Conversation
vadz
left a comment
vadz
left a comment
There was a problem hiding this comment.
I didn't have time to look at the build errors yet, do you already know what the problem is or do you need help with finding it?
As for the code itself, it would be nice if we could avoid repeating so much of the existing code, even at the price of having more conditional compilation directives...
| WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifications], wxUSE_LIBNOTIFY) | ||
| 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) |
There was a problem hiding this comment.
We should probably turn this off by default now. And maybe give a warning if both it and LunaSVG are turned on.
.circleci/config.yml
Outdated
| 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 |
There was a problem hiding this comment.
We probably don't need this one any more?
| git submodule update --init 3rdparty/catch 3rdparty/nanosvg 3rdparty/lunasvg src/stc/scintilla src/stc/lexilla | |
| git submodule update --init 3rdparty/catch 3rdparty/lunasvg src/stc/scintilla src/stc/lexilla |
There was a problem hiding this comment.
We still need it for as long as there is an option to use nanosvg instead of lunasvg.
If the wxWidgets/lunasvg fork is used, then perhaps nanosvg could be dropped entirely? Should an issue arise with displaying an SVG file then it could theoretically be fixed in the fork if it doesn't get fixed in the original repository.
There was a problem hiding this comment.
AFAICS nanosvg is not going to be used in (this) CI build any longer, will it?
Generally speaking, I'm not sure what is the logic here: the PR says that it adds LunaSVG as an optional replacement, but AFAICS it seems to be enabled by default, i.e. it is the new default now?
There was a problem hiding this comment.
Ack, I was thinking optional in general, as in an individual building wxWidgets could choose. You are correct that for the CI build, there is no point to getting the nanosvg submodule.
Yes I know what the problem is, but I'm not thrilled with the solution. The underlying issue is that lunasvg is being created as a dll when a non-monolithic shared build is created. That makes sense to me for 3rdparty code like scintilla which is reasonably large, but not for a tiny bit of code like lunasvg. I would rather see it compiled directly into wxCore if wxUSE_LUNASVG is true so that devs don't have to change their setup scripts to install yet another wxWidgets dll in their packages. In any event, the current build issue is that the code is in a library and not being linked. Is there any way to conditionalize the source code in the bakefile scripts so that it could be compiled directly into wxCore? Digressing somewhat, but I did submit a PR to the lunasvg project with all of the plutovg C code converted to C++ so that a separate library was no longer needed. The PR was rejected on the grounds that it is possible the plutovg code might be changed someday, and the changes would have to be merged into the C++ code. That didn't really make sense to me since plutovg is owned/maintained by the same person, but that's not my call. It could be done in wxWidgets, but of course would require a wxWidgets dev to update it should plutovg change. |
I'm not sure why is a shared library used for it when Lexilla/Scintilla are built as static libraries...
I haven't looked at this code at all, so I have no idea about how likely plutovg is to change but you're going to become the (de facto, sorry if you don't want it :-) maintainer of this part of the code, so if you think it's worth merging them, please feel free to do it — I certainly wouldn't object to it if it makes maintaining it simpler. |
|
I took a look only at the source code, as it is based on mine. My code was written to be tested against wxWidgets NanoSVG implementation and as such is not best suited for integration into wxWidgets code base as is (as also noted by Vadim). I would change it to match the bundle implementation using NanoSVG (and thus my credit should be removed from the file header). In other words, the bundle should have just one ctor taking Moreover, it seems that the code uses my old rasterization method, which did not scale non-square SVGs properly. While such SVGs may not be commonly used for GUI elements, I think we should always maintain the aspect ratio, see PBfordev/wxtestsvg2@a8953e9 |
Probably because I misunderstood the .bkl options, in spite of using lexilla.bkl as the basis for lunasvg.bkl. I'll fix it.
I have a vested interest in ensuring that lunasvg works correctly, since my main app replaced all it's PNG files with 160+ newly created SVG files. I'm fine with with being the de facto maintainer of wxlunasvg for as long as I am able to. I assume updating the widgets fork is the same as widgets itself, i.e., submit a PR? |
Thanks! And I know that dealing with bkl is painful, I'd be glad to replace it with something else, it's just that I don't have anything (see dozens of previous emails about it...).
Yes, absolutely. TIA! |
|
I'm in the process of refactoring the NanoSVG needs a |
|
Sorry if I'm missing something, but to me the answer seems rather clear: we should still provide the overload taking |
|
Sorry I wasn't clear -- the question is about the comments themselves: This is true for nanosvg only -- lunasvg does not modify the data. This is true for nanosvg only -- lunasvg does not make a copy of the data. The implication of these two comments is that a user should either provide a pointer to mutable data, or wxWidgets will duplicate the data for them at the cost of double the amount of memory required. While that is true for nanosvg, it is not true for lunasvg. As for the functions themselves, yes both types are provided to provide backwards compatibility. It is the comments themselves that are a misleading for the lunasvg case. Am I being too nit-picky here? |
|
Ah, sorry. I'd just say something like: // Some implementations make a copy of the data that they modify in place and it's more
// efficient to provide them a non-const buffer if it's already available. |
|
Looking at the PR, I see that there is currently no option to link against an external lunasvg (like with nanosvg). I am asking this because not being able to link against an external lunasvg makes it basically impossible to use wx as a static library and also have your own lunasvg lib in the same application. Or are there plans to add a prefix to all symbols in the forked version of lunasvg so that two versions can coexist without getting symbol clashes? |
Nanosvg was added as a submodule of the original repository with no modifications made to it specifically for wxWidgets. Lunasvg is being added as a fork under wxWidgets so that it can be modified with wxWidgets-specific content. If changes are made to add functionality to the wxWidgets version, then switching to an external library might result in a loss of functionality for wxBitmapBundle rendering. I haven't tried this yet, but it should work to simply change the namespace in the wxWidgets version to |
|
Using |
|
|
||
| #if wxUSE_LUNASVG | ||
| #if defined(wxHAS_SVG) && wxUSE_LUNASVG | ||
| #if !(__cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L)) |
There was a problem hiding this comment.
Could be simplified with wxCHECK_CXX_STD macro.
| wx_add_thirdparty_library(wxUSE_LIBTIFF TIFF "use libtiff (TIFF file format)") | ||
| wx_add_thirdparty_library(wxUSE_NANOSVG NanoSVG "use NanoSVG for rasterizing SVG" DEFAULT builtin) | ||
|
|
||
| # wx_add_thirdparty_library(wxUSE_LUNASVG LunaSVG "use lunasvg for rasterizing SVG (C++17 required)") |
There was a problem hiding this comment.
Was the commented-out line kept by accident?
But I am not sure why some 3rd party libraries are added via wx_add_thirdparty_library and some by modifying ${wxTHIRD_PARTY_LIBRARIES} ...
There was a problem hiding this comment.
wx_add_thirdparty_library adds an option with choices off, sys and builtin, so should only be used when the sources are distributed with wxWidgets. Otherwise you can use wx_option to add an option that can be turned on/off.
Some items are added manually to the wxTHIRD_PARTY_LIBRARIES variable, this is only for it to be listed in the CMake summary (Which libraries should wxWidgets use?), it doesn't set any options.
| Note that wxWidgets currently uses [NanoSVG][] library for SVG support and so | ||
| doesn't support all SVG standard features and you may need to simplify or | ||
| tweak the SVG files to make them appear correctly. | ||
| Note that wxWidgets neither the default [NanoSVG][] library nor the newer |
There was a problem hiding this comment.
I don't think the wording is correct, I would probably just remove " wxWidgets".
| #define wxUSE_LIBTIFF 1 | ||
|
|
||
| // Set to 1 for SVG rasterizing support using LunaSVG (C++17 compiler required) | ||
| #define wxUSE_LUNASVG 0 |
There was a problem hiding this comment.
How comes that NanoSVG is the default here as advertised in the docs, but LunaSVG seems to be the default in the actual setup.h files?
By default, it seems OFF in CMake and 1 in config.{gcc|vc)?
|
|
||
| wxBitmap wxBitmapBundleImplSVG::DoRasterize(const wxSize& size) | ||
| { | ||
| if (IsOk()) |
There was a problem hiding this comment.
| if (IsOk()) | |
| if ( IsOk() ) |
Per coding guidelines, the condition in parentheses should be "wrapped" in a space, applies to other cases here.
Also, I personally would just return here, instead of adding a huge indented block below.
| /// @overload | ||
| static wxBitmapBundle FromSVG(const wxByte* data, size_t len, const wxSize& sizeDef); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Is making the two rasterizer-specific functions public the best idea?
For example, we do not allow the NanoSVG rasterizer to be created with NSVGimage...
Add wxURI::SetUserAndPassword(). See wxWidgets#25240.
Improve the existing macros to show the value of the URI if the test fails -- as long as we're using all these macros anyhow, at least make them more useful.
They are not really needed here as the macro arguments are always simple literals but they make CATCH messages given when a comparison fails less readable as they exclude the parentheses.
For consistency with the other tests and also because they do have some added value now (they show the URI in case of failure), use URI_ASSERT_USER_EQUAL() and the newly added URI_ASSERT_USERINFO_EQUAL() in this test too.
Use it to test the full URI string.
Verify that 8-bit characters are handled as expected, i.e. using UTF-8 first and then percent-encoded.
Add helper wxAuiCreate{Close,Pin}ButtonBitmap() functions and reuse them
in tab art classes to avoid duplicating the same close_bitmap_data and
pin_bitmap_data in both files.
This means we don't need to define wxAuiCreateBitmap() overload taking XBM data in the builds with SVG support any longer, so make it conditional on !wxHAS_SVG.
Rename a local variable to avoid shadowing an existing one.
There is no need for a non-const pointer here, as we only use it to find the corresponding page and don't modify the window itself.
This allows newer versions of the program, adding new pages, to still restore data saved by the older versions. By default the pages not described by the deserialized data are simply appended to the main tab control, however this behaviour can be customized by overriding the new HandleOrphanedPage() function. The signature of the new function is rather complex and not very elegant, but this seems to be the least bad solution, as it allows wxAUI itself to provide the default values (main tab control and -1, meaning to append, for the index), making the code of the applications overriding this function simpler. Also note that we really need to get the location for the page in some way, e.g. we couldn't just let HandleOrphanedPage() or a similar function put it into its place on its own because at the moment of its call the wxAuiNotebook invariant saying that each page in m_tabs is present in exactly one wxAuiTabCtrl is broken, and so many wxAuiNotebook functions, including e.g. DeletePage(), don't work as expected.
It can be useful to preserve the last selected page -- and if the application doesn't want to do it, it can avoid it by not saving wxAuiTabLayoutInfo::active.
Stop saving and restoring locked tabs: their semantics dictates that their state can't be changed by the user, and so they must remain locked no matter what, whereas it was possible to turn a locked tab into a normal one via data given to the deserializer (e.g. by editing a config file manually). Also ensure that restoring the pinned tabs doesn't break the control invariant, i.e. that they don't come after any normal tabs. If the deserializer returns the data that puts pinned tabs on invalid positions, just ignore it and don't make them pinned: this is better than ending up in an invalid state and simpler than moving them before the normal tabs.
Verify that the serialized data has the expected form and that deserializing it results in the expected results. Also check that invalid data is handled gracefully.
This is done mostly to avoid a (harmless) warning about unused parameters in this function in the builds where assertions are disabled.
wxQt MDI fixes. See wxWidgets#25332.
|
This is just a quick update so the PR doesn't get purged as inactive. The medical issues that limited my ability to work have been mostly resolved, and I am working regularly again. My own project is way behind, and I had hoped to tie it to the wxWidgets 3.3 release, so I'm currently focusing on that. Given how close it is to the wxWidgets 3.3 release, my inclination is to work on this after the 3.3.0 release, and target it for a potential 3.3.1 release instead. |
|
Thanks for the update, even if I wasn't going to close this one! And I'm very glad to hear good news about your health, this is far more important than any PRs. |
…tyle Don't assert when setting the wxBG_STYLE_TRANSPARENT style before creation because this is how wxGTK handles it. Moreover, this style cannot be unset once it is set.
Using 15.0-snap doesn't seem to work any longer: all jobs fails with "Agent is not responding!" error and without any other information.
wxQt: Don't assert when wxBG_STYLE_TRANSPARENT is set before creation. See wxWidgets#25361. Closes wxWidgets#25311.
This is just a trivial helper hiding the ugly static_cast. No real changes. Closes wxWidgets#25359.
Only one SVG rendering engine can be used. If wxUSE_LUNASVG is used, it will be used no matter what the setting for wxUSE_NANOSVG is.
wxUSE_NANOSVG now defaults to 0, wxUSE_LUNASVG defaults to 1
lunasvg.cmake adds a link to the C library wxplutovg since it is required by wxlunasvg.
When creating a shared library, define LUNASVG_SHARED and LUNASVG_EXPORT
Combines functions common to both implementations. Switch to a single ctor for wxBitmapBundleImplSVG. Add lunasvg versions of FromSVG that take a std::string and a lunasvg::Document pointer. This makes it possible to load/create an SVG image directly into lunasvg before creating a wxBitmapBundle from it.
Given the much more accurate SVG rendering in lunasvg and the fact that nanosvg has been actively maintained for years, it only makes sense to get the lunasvg submodule.
These don't really make sense within the wxWidgets framework -- they are more specific for calling into lunasvg directly.
This can be used to replace nanosvg
The user needs to set their compiler to C++17 before they can build the wxlunasvg library. Currently the default for wxWidgets is C++11, so all the PR build testing fails if lunasvg is enabled.
|
Ack, looks like I pushed the branch on the wrong repository -- meant to be pushing an update on wxWidgets/lunasvg not the main wxWidgets branch. In any event, it does rather highlight that this PR got too stale, and the merge conflicts I have with the main repository are intense -- on top of which lunasvg is now a single submodule instead of two. I've actually been working on a different branch where I've incorporated the code review comments from PBfordev and am currently verifying the latest lunasvg code. After that, I'll start from scratch updating the current build scripts rather than risk breaking something with the merge conflicts. Once that's done, my plan is to delete this PR entirely, and create a new one that's based on the current build script files. |
18 participants
This PR adds lunasvg as an optional replacement of nanosvg when rendering SVG files. See #24216 for a detailed explanation of the advantages of lunasvg (better accuracy, equivalent performance). The implementation is designed to use eithr nanosvg or lunasvg, but not both. The choice is controlled by
wxUSE_LUNASVGin setup.h (defaults to 1).Lunasvg is a submodule consisting of a C++ library (wxlunasvg) and a C library (wxplutovg).
I am not very familiar with either bakefile or the various ways in which wxWidgets can be built. I think I got the various bakefiles changes made correctly, but a closer review from someone more familiar with the build system would be greatly appreciated.
As a side note, I added PB to the copyright authors in bmpsvg.cpp since I essentially used his code for testing lunasvg with some minor modifications when I integrated it into wxWidgets.