-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LunaSVG as an optional replacement for NanoSVG #24475
base: master
Are you sure you want to change the base?
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@@ -530,6 +530,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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably turn this off by default now. And maybe give a warning if both it and LunaSVG are turned on.
@@ -33,7 +33,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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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_LUNASVG
in 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.