Chlumsky / msdfgen

Multi-channel signed distance field generator
MIT License
3.91k stars 404 forks source link

Modernize cmake and allow installation #104

Closed gracicot closed 4 years ago

gracicot commented 4 years ago

This patch modernize the CMake setup.

The main things that are changed:

Things I'm not so sure about and can be fixed or changed:

gracicot commented 4 years ago

To make includes uniform whether it is used as a subdirectory of a project or installed on the system will require some change in the directory structure. I have made the changes to test it and I think I'll add them to the pull request.

Chlumsky commented 4 years ago

Let me know when you're done.

gracicot commented 4 years ago

@Chlumsky I think this was the last change it needed. If I notice something unusual, I'll let you know and open a new pull request to fix it.

Chlumsky commented 4 years ago

Why did you delete this part https://github.com/Chlumsky/msdfgen/pull/104/commits/82bc488d01ab0428c0a8bec8986b8e8965ecd45f#diff-af3b638bc2a3e6c650974192a53c7291L7?

gracicot commented 4 years ago

I replaced it with target_compile_features(msdfgen PUBLIC cxx_std_11). Using this feature we don't have to manually check for flags, and enable C++11 in a cross platform manner.

Chlumsky commented 4 years ago

But where do you define the macro?

gracicot commented 4 years ago

Oops, my bad. I'll add back. Is the C++11 support is still optional? If so I'll add another option at the top.

gracicot commented 4 years ago

It should be better now. Tell me if there's anything else!

You can try the installation using make install or ninja install to see if everything is good.

Chlumsky commented 4 years ago

Alright, so I tried running it (in CMake 3.9 as it is the declared minimum version) and there is this error:

CMake Error at CMakeLists.txt:72 (add_library):
  Target "msdfgen-ext" links to target "Freetype::Freetype" but the target
  was not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

To make it work, I replaced Freetype::Freetype with ${FREETYPE_LIBRARIES}, otherwise it wouldn't link. The include directory for Freetype is also misssing, so I had to add ${FREETYPE_INCLUDE_DIRS} to msdfgen-ext.

Finally, I got this error from Visual Studio: LNK1149: output filename matches input filename 'build\Debug\msdfgen.lib', so maybe rename the msdfgen module to msdfgen-core. I also don't like msdfgentools at all, so maybe make it msdfgen-standalone.

When all this is resolved, I'd like someone else to confirm that it works too.

gracicot commented 4 years ago

You're right about freetype. It requires CMake 3.10 for the target to be exported by the find module. I'll update it.

For the names, I renamed them for msdfgen::msdfgen to work when linking to it. Most projects names the default target to work like this. It is also analogous to the header names msdfgen.h and msdfgen-ext.h.

I'll gladly rename the target name for the executable. I'll also make some manual tests on Windows as I've been doing then on Linux only for now.

psalz commented 4 years ago

Well that's a nice surprise! I was just about to roll up my sleeves and get started on improving the CMake setup, only to find out that someone already did all the heaving lifting :)

I just built this successfully on Windows using Freetype installed through vcpkg. It worked pretty much flawlessly right out of the box (also linking and running it from an external project) - great job!

The only thing I'd like to suggest is adding the ability to install both release and debug versions of the library side by side, which can be done (e.g.) by changing the name of the debug version:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 846c274..237a525 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -51,7 +51,10 @@ folderize_sources(msdfgen-ext_SOURCES ${CMAKE_SOURCE_DIR})

 add_library(msdfgen ${msdfgen_SOURCES} ${msdfgen_HEADERS} "./msdfgen.h")
 add_library(msdfgen::msdfgen ALIAS msdfgen)
-set_target_properties(msdfgen PROPERTIES PUBLIC_HEADER "${msdfgen_HEADERS}")
+set_target_properties(msdfgen PROPERTIES
+       PUBLIC_HEADER "${msdfgen_HEADERS}"
+       DEBUG_POSTFIX "d"
+)
 target_include_directories(msdfgen INTERFACE
        $<INSTALL_INTERFACE:include>
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../>
@@ -73,6 +76,7 @@ add_library(msdfgen-ext ${msdfgen-ext_SOURCES} ${msdfgen-ext_PUBLIC_HEADERS} ${m
 add_library(msdfgen::msdfgen-ext ALIAS msdfgen-ext)
 set_target_properties(msdfgen-ext PROPERTIES
        PUBLIC_HEADER "${msdfgen-ext_PUBLIC_HEADERS}"
+       DEBUG_POSTFIX "d"
 )
 target_link_libraries(msdfgen-ext PUBLIC msdfgen::msdfgen Freetype::Freetype)
 target_include_directories(msdfgen-ext

I then bashed my head against the table a couple of times until I found this little bugger, which breaks linking against the (now differently named) debug version of the library. I don't think this line should be needed anymore..?

Chlumsky commented 4 years ago

There is still the problem with same input and output filename in Visual Studio, but this line fixes it:

set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive)
gracicot commented 4 years ago

Thank you for merging and fixing the remaining issues!