Skip to content

Improve CMake for IDE Projects (Visual Studio) #13704

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

playmer
Copy link
Contributor

@playmer playmer commented Aug 7, 2025

Some relatively small changes to the CMake to make generated IDE Projects more usable. I primarily use and hack on SDL from Visual Studio, and often using vcxprojs created by the "Visual Studio *" CMake generators. As it is, VS doesn't know about the headers because they're not added as "sources", so things that rely on the vcxproj info can't display them. It makes it difficult to jump around and look for headers.

Additionally there's a change to make the Solution Explorer have more detail than just "source" and "header" filters, instead setting up filters that match the file structure.

Description

For adding the headers, two small changes were needed. In the glob where we look for sources, we needed to add globs for headers, which gets us the internal headers. For the public headers, we were already tracking these in a variable, and on Apple we were already doing what needed to be done, I just moved that small piece outside of the Apple only check. It might be that there's a better place for it, but it needs to be after we've appended everything to it.

The "filters" problem was more difficult, exclusively due to how android builds SDL and how the JNI sources aren't within it. Ideally we'd essentially just call source_group(TREE ${${PROJECT_SOURCE_DIR} ...) inside sdl_glob_sources and sdl_sources, but this fails due to those JNI sources being in a sibling directory to the PROJECT_SOURCE_DIR and source_group not having any way to ignore pathing errors. I ended up just writing a function to filter out paths that weren't within our project dir, and called it a day. It's slightly more complex than it needs to be since we're using CMake 3.16, but it should be okay.

Anyway, thanks for considering this change :)

@slouken slouken requested a review from madebr August 7, 2025 06:25
Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

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

Looks good!
I think you missed some Windows sources/headers.

@@ -1191,34 +1191,63 @@ endif()
# General source files
sdl_glob_sources(
"${SDL3_SOURCE_DIR}/src/*.c"
"${SDL3_SOURCE_DIR}/src/*.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some more sdl_glob_sources under if(WINDOWS).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. wasn't able to tinker on this for most of the day. Looking at it more closely, there's actually a ton more that I've missed. Now I wonder if I should change approach and augment sdl_glob_sources to get the directories of the files passed in and add additional *.h globs in. What do you think? It'd certainly be less work to maintain it, (For however much work it really is to add a few extra lines of CMake to account for headers is.) Though it might be better for it to accept directories rather than files...I dunno!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not make the globs too smart.
We're already not following cmake best practices by not explicitly listing the sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do much prefer being explicit, but I know that doesn't work for everyone. I'll do a thorough pass tomorrow and try not to miss any.

As a tangential question, is there a reason the GLOBs aren't using CONFIGURE_DEPENDS?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about that option.
If performance does not suffer, it makes sense to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'll try to spend some time testing it over the weekend. I glob relatively sparingly in my own codebases, so I don't know their performance characteristics, but folks I trust were championing that flag early on and suggested they didn't see a noticeable difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you end up measuring the impact of CONFIGURE_DEPENDS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants