search

erincatto / box2d

Box2D is a 2D physics engine for games
https://box2d.org
MIT License
8.26k stars 1.54k forks source link

Shared library #631

Closed jube closed 4 years ago

jube commented 4 years ago

This pull request add support for building Box2D as a shared library. It fixes #621. By default, the library is built as a static library, you have to add -DBUILD_SHARED_LIBS=ON to the cmake invocation to build the shared library.

If you merge #630 at the same time, you will have to uncomment some lines in src/CMakeLists.txt so that the library can work properly once installed. Anyway, merging both should result in conflicts. If you plan to merge both, merge #630 first, and I will make the necessary modifications to this PR so that it can be merged smoothly.

erincatto commented 4 years ago

I think we would need some test to make sure this works. It would be cool if the testbed optionally can run Box2D as a DLL/shared lib.

jube commented 4 years ago

I have tested both configurations (shared and static) and the testbed works in both cases. I work on a Debian 10 (stable).

That's why I had to declare some variables that were not declared in headers but that were used in the testbed. With the CMAKE_CXX_VISIBILITY_PRESET hidden, all symbols that are not properly exported are not visible (that's the default behavior on Windows, but not on Linux). So I am pretty confident it works well with Box2D as a shared library.

If someone else can try this patch, it would be nice. Maybe @limburgher?

If you want to test it on Windows yourself, you have to run cmake-gui in the build folder of Box2D and check the BUILD_SHARED_LIBS option (and then, press Configure and Generate). Normally, it will trigger a whole reconfiguration/recompile of the library in Visual Studio. I will be able to test this patch on Windows tonight maybe.

svenstaro commented 4 years ago

FWIW I tested and approve these changes. I'd appreciate if they were upstreamed.

jube commented 4 years ago

I just resolved the merge conflict after #630 has been merged in master.

svenstaro commented 4 years ago

This needs a rebase again.

jube commented 4 years ago

Rebased. Hope it works.

svenstaro commented 4 years ago

@erincatto can this be merged? It would address my downstream maintainer needs. :)

erincatto commented 4 years ago

The diffs are showing changes from other PRs. Is there a way to clean this up or view the diffs of just this PR?

jube commented 4 years ago

My bad, I messed up with the latest rebase. I will try to fix it as soon as possible.

@erincatto you still have to decide about b2_api.h. In this PR, the file is generated by cmake at configuration time. It is then installed in the include directory next to the other files. The other option would be to define this file directly in the repository. You can see the discussion with @bschatt above. I find it easier to let cmake generate it but I can understand @bschatt's arguments and I could change the PR if necessary. It's up to you.

jube commented 4 years ago

This time, it should be ok.

erincatto commented 4 years ago

Thanks. I will test this soon.

erincatto commented 4 years ago

Tested this finally. It looks like box2d.dll is not in the path of testbed.exe image

erincatto commented 4 years ago

It looks like you are using CMake to generate a file that must be included, b2_api.h. This goes against my goal of people being able to build Box2D without CMake.

jube commented 4 years ago

b2_api.h is not generated anymore by CMake, a file has been created with the same features in the include/box2d directory.

As for the other problem, the solution is to put all the binaries (executables and libraries) in the same build folder. So now, everything is built in a bin folder in the build directory. The only exception is the unit_test binary as it has been put in the build directory manually. But I think it should be put in the same bin directory (delete the lines 17-18 in unit-test/CMakeLists.txt).

erincatto commented 4 years ago

@jube thanks. Please modify the unit test too as you mentioned.

Somehow unit_test links without B2_SHARED being defined. testbed needs this added.

jube commented 4 years ago

Done! I handled the B2_SHARED too.

I think it would be better to squash all the commits before merging to avoid pollution in the main branch.

Nek- commented 4 years ago

Also isn't the install script missing @jube ?

jube commented 4 years ago

@Nek- Install instruction are spread in CMakeLists.txt and src/CMakeLists.txt. It seems to work. What is missing?

Nek- commented 4 years ago

@jube idk I'm not an expert in C++ and Cmake 😅 I just was not able to run the installation. But the build of the .so worked perfectly with the correct option set.

svenstaro commented 4 years ago

Any chance for a new release including this change? :)

erincatto commented 4 years ago

@svenstaro I'm working on that right now