Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake Fixes #1062

Merged
merged 8 commits into from
Sep 22, 2023
Merged

CMake Fixes #1062

merged 8 commits into from
Sep 22, 2023

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Apr 21, 2023

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

@ptheywood
Copy link
Member Author

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
Copy link
Member

Robadob commented Sep 12, 2023

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

@ptheywood
Copy link
Member Author

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
Copy link
Member

Robadob commented Sep 13, 2023

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
Copy link
Member Author

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 ptheywood marked this pull request as ready for review September 21, 2023 17:00
Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake: Demonstrate use of flamegpu_add_library on the host_fucntions example. Maybe don't merge?

Would require example readme discussing, or delete commit.

Rest of the commits look fine, though I didn't really fully process the changes to the static lib refactor the diff was a bit of a mess.

…main CMake project

I.e. linting flamegpu2 is not relevant to consumers of this repo via add_subdirectory, so only make the target if a user explicitly requests it

Closes #1045
…ts.txt

This does change the filters to always place things in 'Header Files' and 'Source Files',
matching CMake's implicit placement of application_icon.rc in 'Source Files'.

If files are above the CMakeLists.txt, they are just flat placed in the appropraite header / source files filter
@ptheywood
Copy link
Member Author

ptheywood commented Sep 22, 2023

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.

Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ptheywood ptheywood merged commit dd86e00 into master Sep 22, 2023
18 checks passed
@ptheywood ptheywood deleted the cmake-fixes-rc1 branch September 22, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants