-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
Add hot reload support to CMakeLists.txt #1330
Conversation
This does not match what the code does for scons, it defaults to true if you are not on a release build:
This should be reflected here |
CMakeLists.txt
Outdated
@@ -43,6 +43,7 @@ project(godot-cpp LANGUAGES CXX) | |||
option(GENERATE_TEMPLATE_GET_NODE "Generate a template version of the Node class's get_node." ON) | |||
option(GODOT_CPP_SYSTEM_HEADERS "Expose headers as SYSTEM." ON) | |||
option(GODOT_CPP_WARNING_AS_ERROR "Treat warnings as errors" OFF) | |||
option(GODOT_ENABLE_HOT_RELOAD "Build with hot reload support" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should document this option above along with the rest
Thanks for the comments! I've made changes accordingly, but I was not able to set the default value in a particularly elegant manner. My CMake skills are quite basic, and I couldn't get generator expressions to work as I wanted, hence the ugly |
Why are you pinging me with no question or anything? Please don't just ping people to boost things 🙁 |
I'm not sure how reviews and merging in Godot typically goes, but I fixed the comments 10 days ago and left a comment about it. The ping was just a friendly notification in case you didn't see it. |
Understood, but then please make that clear, just pinging someone without any details isn't very friendly 🙂 The review process can be slow at times, especially at this time of year, I mainly gave some minor details and would have approved had it been something I was ready to do, I get updates when things change so no worries, no need to remind Also for the future, if you want a new review, just push the little icon next to the reviewer to re-request review (don't do it for me here though, as I have nothing to add here) It can be confusing I know 🙂, but I hope you can see that just tagging someone without actually stating your intent (and assuming it's clear from assumptions) is a bit like just yelling "hey!" at someone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I don't know cmake very well, but it seems to be working as expected in my testing. I only have the one tiny bit of review, otherwise this seems fine to me. Ideally, it'd be nice for someone who is more knowledgeable with cmake to review it, but we don't have too many folks who are.
@@ -116,6 +124,10 @@ else() | |||
endif() | |||
endif() | |||
|
|||
if (GODOT_ENABLE_HOT_RELOAD) | |||
set(GODOT_COMPILE_FLAGS "${GODOT_COMPILE_FLAGS} -D HOT_RELOAD_ENABLED") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other defines are added to the command-line without the space after -D
, it'd be nice for that to be consistent:
set(GODOT_COMPILE_FLAGS "${GODOT_COMPILE_FLAGS} -D HOT_RELOAD_ENABLED") | |
set(GODOT_COMPILE_FLAGS "${GODOT_COMPILE_FLAGS} -DHOT_RELOAD_ENABLED") |
@@ -57,6 +58,13 @@ if("${CMAKE_BUILD_TYPE}" STREQUAL "") | |||
set(CMAKE_BUILD_TYPE Debug) | |||
endif() | |||
|
|||
# Hot reload is enabled by default in Debug-builds | |||
if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, given that it looks like "Debug" and "Release" are the only options here. With scons, there's also the possibility of making editor builds, which is why the logic there is "not release" as opposed to checking directly for debug. I guess if we ever have editor builds via cmake, though, we'll need to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider also Multiconfig generators like "Visual Studio ..." or NinjaMultiConfig where there is no CMAKE_BUILD_TYPE. Then you can use generator expression $
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a tiny white space thing pointed out, but it's not critical. I think we should go for this PR - the cmake stuff is in dire need of a clean-up, which can happen later.
Cherry-picked for 4.2 in PR #1570 |
Add support for hot reloading when building
godot-cpp
using CMake.CMake projects can enable the reloading by setting
GODOT_ENABLE_HOT_RELOAD
toON
before including thegodot-cpp
subdirectory.Example:
I believe I've done all of this correctly — I'm super new to both Godot and Scons, but from how I understand the Scons-support for hot reloading in #1200, I believe this to be correct.