FLAMEGPU / FLAMEGPU2

FLAME GPU 2 is a GPU accelerated agent based modelling framework for CUDA C++ and Python
https://flamegpu.com
MIT License
107 stars 22 forks source link

CMake Fixes #1062

Closed ptheywood closed 1 year ago

ptheywood commented 1 year ago

A number of fixes and additions to FLAMEGPU2's CMake encountered when working on a more complex tempalte model.

ptheywood commented 1 year ago

I've got a fix for source groups in place for examples, which always uses the defualt CMAke Source Files and Header Files filters. If files are children of the CMakeProject.txt dir, then they will be placed into the appropraite file structure. If not, they will just be dumped in the root filter (as we don't have a root to create a structure from).

In this case, the fiules main.h and main.cu are from above CMakeLists.txt (i.e. ../main.cu).

image

If they are a child, then a dir would also be applied, i.e. src:

image

This shows the application_icon rc being placed together, rather than being the only thing in Source Files as previously


@Robadob If you're happy with this prefixing src with Source Files (I don't have a better option otherwise anyway) then I'll do the same for flamegpu for consistency? (which will also simplify it), and then this will be ready to review once I've done that.

I will push it with the examples showing it now behaving correctly in the erranous cases, but a couple of commits will then want dropping prior to merge as they add examples that are not really examples. Unit testing CMake stuff is not something we're set up for.

Robadob commented 1 year ago

I've no strong preference, removing the redundant src seems fine.

ptheywood commented 1 year ago

removing the redundant src seems fine.

For examples we can't really do that, as the src dir may not exist (which could cause errors / issues with the source grouping before) and would then potentially cause folder conflicts, if say the user had this (unwise) setup

├── a
│   └── b.h
└── src
    └── a
        └── b.h

Then intentionally removing src would cause both a's to be the same filter, which contain files with the same name which I'm not sure how VS would handle.


We probably could for the flamegpu target, but its probably simpler / more consistent to keep it.

I'll make the change when I'm back in windows at some point.


The CI failure is just from my low-effort testing of the folder structures / CMake, so once I drop the appropraite commit it pass.

Robadob commented 1 year ago

removing the redundant src seems fine.

For examples we can't really do that, as the src dir may not exist (which could cause errors / issues with the source grouping before) and would then potentially cause folder conflicts, if say the user had this (unwise) setup

├── a
│   └── b.h
└── src
    └── a
        └── b.h

Then intentionally removing src would cause both a's to be the same filter, which contain files with the same name which I'm not sure how VS would handle.

We probably could for the flamegpu target, but its probably simpler / more consistent to keep it.

I'll make the change when I'm back in windows at some point.

The CI failure is just from my low-effort testing of the folder structures / CMake, so once I drop the appropraite commit it pass.

As I said, no strong preference do what you think best. It's hard to go wrong.

ptheywood commented 1 year ago

I've now made the source_group change to the FLAMEGPU target, so that it places things into the CMake default filters of Header Files and Source Files, but using tree within the FLAMEGPU_ROOT directory (must use FLAMEGPU_ROOT as include is above src/CMakeLists.txt).

This makes it nest deeper, but is consistent.

image

I'm not strongly opinionated either way, so if you don't like this we can drop the final commit and still merge this anyway.


I've also removed the example(s) which showed the previous CMake failures, as they were messy and I'm not aware of a nice way to have a CMake test suite. I have pushed them to another branch cmake-fixes-rc1-with-example if you would like to test them.

Otherwise this PR is ready for review.

ptheywood commented 1 year ago

I've now dropped the missed "probably don't merge" commit, which was just a test case demonstrating the library use case rather than binary. I'll open an issue to document it on the docs repo and / or use it in a standalone example in the future too.

rebased onto current master as well.