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

Added hot reload support to CMakeLists.txt #1330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pimms
Copy link

@pimms pimms commented Dec 9, 2023

Add support for hot reloading when building godot-cpp using CMake.

CMake projects can enable the reloading by setting GODOT_ENABLE_HOT_RELOAD to ON before including the godot-cpp subdirectory.

Example:

set(GODOT_ENABLE_HOT_RELOAD ON)
add_subdirectory("${CMAKE_CURRENT_SOURCE_DIR}/godot-cpp/" build-godot-cpp)

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.

@pimms pimms requested a review from a team as a code owner December 9, 2023 14:23
@AThousandShips
Copy link
Member

AThousandShips commented Dec 9, 2023

This does not match what the code does for scons, it defaults to true if you are not on a release build:

if env.get("use_hot_reload") is None:
    env["use_hot_reload"] = env["target"] != "template_release"

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

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

@pimms
Copy link
Author

pimms commented Dec 10, 2023

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 if (...)/else().

@pimms
Copy link
Author

pimms commented Dec 19, 2023

@AThousandShips

@AThousandShips
Copy link
Member

AThousandShips commented Dec 19, 2023

Why are you pinging me with no question or anything? Please don't just ping people to boost things 🙁

@AThousandShips AThousandShips requested a review from a team December 19, 2023 10:21
@pimms
Copy link
Author

pimms commented Dec 20, 2023

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.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 20, 2023

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

Copy link
Contributor

@dsnopek dsnopek left a 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")
Copy link
Contributor

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:

Suggested change
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")
Copy link
Contributor

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.

Copy link

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 $

@dsnopek dsnopek added cmake topic:buildsystem Related to the buildsystem or CI setup cherrypick:4.2 labels Jan 3, 2024
@dsnopek dsnopek added this to the 4.x milestone Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 cmake topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants