Add LunaSVG as an optional replacement for NanoSVG

[Randalphwa]

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.

[vadz]

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...

[vadz]

We should probably turn this off by default now. And maybe give a warning if both it and LunaSVG are turned on.

[vadz]

We probably don't need this one any more?

Suggested change

	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

[Randalphwa]

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.

[vadz]

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?

[Randalphwa]

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.

[Randalphwa]

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?

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.

[vadz]

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?

I'm not sure why is a shared library used for it when Lexilla/Scintilla are built as static libraries...

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 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.

[PBfordev]

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 lunasvg::Document and the string/file manipulation should be done in the static wxBitmapBundle::From*() methods (perhaps shared for both implementations?) instead of the bundle ctors. Please also note that the current implementation loading from a file is incorrect, not using wxNO_SVG_FILE and possibly (however unlikely) erroring out at build time. All this also means that most wxWidgets/standard includes do not need to be listed separately for each implementation, they should be listed just once inside the wxHAS_SVG block.

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

[Randalphwa]

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?

I'm not sure why is a shared library used for it when Lexilla/Scintilla are built as static libraries...

Probably because I misunderstood the .bkl options, in spite of using lexilla.bkl as the basis for lunasvg.bkl. I'll fix it.

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 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 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?

[vadz]

Probably because I misunderstood the .bkl options, in spite of using lexilla.bkl as the basis for lunasvg.bkl. I'll fix it.

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...).

I assume updating the widgets fork is the same as widgets itself, i.e., submit a PR?

Yes, absolutely. TIA!

[Randalphwa]

I'm in the process of refactoring the wxBitmapBundleImplSVG source code as per PB's suggestions, and I have a question about conflicting comments in the header file.

NanoSVG needs a FromSVG(char* data method because it modifies that data, and the other variants of this function make copies of the data. LunaSVG only takes a const char* so it will never directly modify the data passed to it. Therefore all the comments about making a copy and needing to modify the data only apply to NanoSVG. Fortunately the interface file doesn't mention this, but I'm not sure how best to reflect the differences between the two systems in the header file in terms of comments.

[vadz]

Sorry if I'm missing something, but to me the answer seems rather clear: we should still provide the overload taking char* in the public API, but it could just do the same thing as the overload taking const char* internally.

[Randalphwa]

Sorry I wasn't clear -- the question is about the comments themselves:

    // Notice that the data here is non-const because it can be temporarily
    // modified while parsing it.
    wxNODISCARD static wxBitmapBundle FromSVG(char* data, const wxSize& sizeDef);

This is true for nanosvg only -- lunasvg does not modify the data.

    // This overload currently makes a copy of the data.
    wxNODISCARD static wxBitmapBundle FromSVG(const char* data, const wxSize& sizeDef);

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?

[vadz]

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.

[rcane]

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?

[Randalphwa]

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 wxlunasvg instead of lunasvg. That way you could link to both wxlunasvg.lib and lunasvg.lib in the same project.

[rcane]

Using wxlunasvg as the namespace sound like a good idea in any case. It makes it clear that the fork is connected to wx. But I was actually more concerned about plutovg which is a C library. You mentioned that you had a PR to integrate plutovg into lunasvg. This would fix this problem. But as you and Vadim already realized, it would mean that the burden of maintaining this would fall on the wx community.