Refactor: move collide2 out as a library #1041

BenjamenMeyer · 2025-02-27T04:46:24Z

Add a new libraries directory in parallel to the engine for our various library components, and add the collide2 code there as the first library component as it is the most independent of the various libraries.

NOTE: There is no real code change outside of header includes

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Have the PR Validation Tests been run? See https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation -- I have done some play testing; there's not really any code changes outside of some header adaptions so this shouldn't play any different from master.
This is a documentation change only

Issues:

Restructure the source #56

Purpose:

What is this pull request trying to do? Breaking down the code structure into smaller, independent components
What release is this for? 0.10.x
Is there a project or milestone we should apply this to? 0.10.x

Add a new `libraries` directory in parallel to the `engine` for our various library components, and add the `collide2` code there as the first library component as it is the most independent of the various libraries. NOTE: There is no real code change outside of header includes

royfalk

Looks pretty straightforward. Not sure how meaningful it is without encapsulating it from the rest of the code though.

royfalk · 2025-02-27T05:24:56Z

libraries/collide2/CMakeLists.txt

+    # collide2 headers
+    ${CMAKE_CURRENT_SOURCE_DIR}
+    # VS engine headers
+    #${CMAKE_SOURCE_DIR}/engine


Please open an issue for refactoring VS out of collide library. It's no good separating it out if it refers to VS all over.

collide2 is generally under a difference license (public domain from what I can tell... or rather the UNLICENSE) so at a minimum we gain that clarity.

agreed - there's a few things (gfx/quaternion.h) that seem to have gotten spread out. it'd be nice to make it more independent that way.

Filed #1042

Looking at the build failures (#1041 (comment)) I'll have to address some of this somehow... but my goal is to be minimal right now. Not sure if I'll fix up #1041 immediately or just move some checks around in CMake for the moment so Mac/Win builds succeed - more likely just move stuff around b/c it'll probably make more sense long term with other libraries being split out any way.

royfalk · 2025-02-27T05:29:39Z

CMakeLists.txt

+
+#Find Math
+INCLUDE(CheckSymbolExists)
+IF(NOT POW_FUNCTION_EXISTS AND NOT NEED_LINKING_AGAINST_LIBM)


I was unable to find a lot of information about this. Is there really a chance modern systems don't have POW? (whatever that is)
I'm asking because a lot of stuff in our cmake was really legacy from 2001.

I doubt it. Created #1043 to follow up on that.

We need pow. This block of code tells us where to find it (i.e., what library). I don't think this code should be removed.

BenjamenMeyer · 2025-02-27T06:32:18Z

NOTE: I reviewed the build failures:

The Mac build failed due to not finding the endian.h system header included by our endianness.h header
The Windows builds failed due to not finding expat.h

Both of these were while building the the collide2 library. Interestingly, Linux has no issue with either of these. Probably means that some of the library searches we do in the VS engine need to be more universal for now - at least until #1041 can be completed.

royfalk

OK. Make it build and run and ship it.

vincele · 2025-03-09T13:27:35Z

engine/CMakeLists.txt

@@ -52,6 +53,9 @@ LIST(APPEND
    ${CMAKE_CURRENT_SOURCE_DIR}/src/resource
    ${CMAKE_CURRENT_SOURCE_DIR}/src/python/base_computer
    ${CMAKE_CURRENT_SOURCE_DIR}/src/python/config
+    # Library Headers
+    ${CMAKE_SOURCE_DIR}/libraries/collide2


Why not just ${CMAKE_SOURCE_DIR}/libraries ? That would remove the need to modify a lot of the #include

Consistency between the libraries. Honestly I'd like to get the headers paths down to a lot more limit set long term so this is the first step along a longer path trajectory.

The library itself should use its own directory as the import path so its self-contained. Similarly the code linking against it needs to use the same path so that they can pull in the headers for a library.

In my own code I split out headers as their own thing:

project project\include project\src

and then external sources linking link against the project\include. We haven't agreed to that here right now and given our state I'm not ready to push that question; we have a lot of other things that are more important; but that would also change this to:

${CMAKE_SOURCE_DIR}/libraries/collide2

That said, I'd also like to find a CMake way to do this automatically by searching for the directories under ${CMAKE_SOURCE_DIR}/libraries and then adding them; however I haven't been able to find a directive that would allow that yet - it finds source files easily, but not directories or at least not filtered to a single level.

Move the dependency detection to the root so it can span the libraries. Rename `TST_LIBS` and `TST_INCLUDES` in the engine to have a VSE_ prefix so that it doesn't mess up other libraries.

BenjamenMeyer · 2025-03-09T20:07:40Z

Moved a chunk of the dependency searching to the root; this should fix Windows. Not sure about the endian one, but need a check and see how the CI system does with this change before debugging further.

Play tested a little to see how it was doing, and also brought up-to-date with master.

Initialize the TST_INCLUDES and TST_LIBS variables for CMake Add TST_INCLUDES to the include list for CMake

BenjamenMeyer · 2025-03-10T02:06:25Z

@stephengtuggy I need some help figuring out the Windows side.

Mac is still misbehaving but I think I can figure that out for the moment. I filed #1059 as a long term issue for what's going on with Mac but will try to get it working in the immediate here.

BenjamenMeyer · 2025-03-10T02:59:41Z

NOTE: for some reason MacOS wasn't falling into the BSD/FreeBSD logic for the Endianness header... added a __APPLE__ case too.

BenjamenMeyer · 2025-03-10T03:17:03Z

welll....traded up one Mac error (endian.h) for another (boost/move/utility_core.hpp)...neither really makes sense for why they're happening in this PR in particular when they not in others.

BenjamenMeyer added the enhancement label Feb 27, 2025

BenjamenMeyer added this to the 0.10.x milestone Feb 27, 2025

BenjamenMeyer requested review from royfalk, evertvorster, stephengtuggy and kheckwrecker February 27, 2025 04:46

BenjamenMeyer self-assigned this Feb 27, 2025

royfalk reviewed Feb 27, 2025

View reviewed changes

This was referenced Feb 27, 2025

Make collide2 more independent #1042

Open

Do we really need to check if pow exists in CMake? #1043

Open

royfalk approved these changes Feb 28, 2025

View reviewed changes

BenjamenMeyer mentioned this pull request Mar 8, 2025

Cmake - Remove pow & some libm & math.h handling #1051

Open

2 tasks

vincele reviewed Mar 9, 2025

View reviewed changes

BenjamenMeyer mentioned this pull request Mar 9, 2025

Finish updating copyleft headers - 0.9.x #1030

Merged

1 task

BenjamenMeyer added 2 commits March 9, 2025 15:27

Bug Fix

2bb4893

Move the dependency detection to the root so it can span the libraries. Rename `TST_LIBS` and `TST_INCLUDES` in the engine to have a VSE_ prefix so that it doesn't mess up other libraries.

Merge branch 'master' into refactor_collide2-as-library

d3c36a0

Bug Fix

89c0439

Initialize the TST_INCLUDES and TST_LIBS variables for CMake Add TST_INCLUDES to the include list for CMake

Bug Fix - Mac Endian detection

51dca49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: move collide2 out as a library #1041

Refactor: move collide2 out as a library #1041

BenjamenMeyer commented Feb 27, 2025

royfalk left a comment

royfalk Feb 27, 2025

BenjamenMeyer Feb 27, 2025

BenjamenMeyer Feb 27, 2025

royfalk Feb 27, 2025

BenjamenMeyer Feb 27, 2025

stephengtuggy Mar 3, 2025

BenjamenMeyer commented Feb 27, 2025

royfalk left a comment

vincele Mar 9, 2025 •

edited

Loading

BenjamenMeyer Mar 9, 2025

BenjamenMeyer commented Mar 9, 2025

BenjamenMeyer commented Mar 10, 2025 •

edited

Loading

BenjamenMeyer commented Mar 10, 2025

BenjamenMeyer commented Mar 10, 2025

Refactor: move collide2 out as a library #1041

Are you sure you want to change the base?

Refactor: move collide2 out as a library #1041

Conversation

BenjamenMeyer commented Feb 27, 2025

royfalk left a comment

Choose a reason for hiding this comment

royfalk Feb 27, 2025

Choose a reason for hiding this comment

BenjamenMeyer Feb 27, 2025

Choose a reason for hiding this comment

BenjamenMeyer Feb 27, 2025

Choose a reason for hiding this comment

royfalk Feb 27, 2025

Choose a reason for hiding this comment

BenjamenMeyer Feb 27, 2025

Choose a reason for hiding this comment

stephengtuggy Mar 3, 2025

Choose a reason for hiding this comment

BenjamenMeyer commented Feb 27, 2025

royfalk left a comment

Choose a reason for hiding this comment

vincele Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

BenjamenMeyer Mar 9, 2025

Choose a reason for hiding this comment

BenjamenMeyer commented Mar 9, 2025

BenjamenMeyer commented Mar 10, 2025 • edited Loading

BenjamenMeyer commented Mar 10, 2025

BenjamenMeyer commented Mar 10, 2025

vincele Mar 9, 2025 •

edited

Loading

BenjamenMeyer commented Mar 10, 2025 •

edited

Loading