feat: update CMakeLists for Visual Studio #4
Conversation
|
Perhaps I should also copy BotConfig into build/bin/Release ??? In case someone starts 'Local Windows Debugger' in Release mode instead of Debug mode? I guess I always assumed Visual Studio defaulted to Debug for me. Maybe I should be using ${project_source_dir} over ${cmake_source_dir}, or vice-versa; I see I am a inconsistent there. I am on my potato right now, I'll do more thorough checking on startup behavior in Visual Studio in a few days. |
Nickrader
force-pushed
the
vsDebugger
branch
3 times, most recently
from
ad366db
to
0b233e4
Compare
|
I think it's fixed up now. Minor style uncertainty. |
There was a problem hiding this comment.
Hey @Nickrader
Thank you for the patch and excude me for long review.
The way you add the files is ok, but there are other minor problems with the suggested approach.
CMakeLists.txt
Outdated
|
|
||
| # Make directory that 'Build' in Visual Studio would, so we can move BotConfig.txt | ||
| file(MAKE_DIRECTORY ${CMAKE_SOURCE_DIR}/build/bin/Debug) | ||
| file(COPY_FILE ${CMAKE_SOURCE_DIR}/bin/BotConfig.txt ${CMAKE_SOURCE_DIR}/build/bin/Debug/BotConfig.txt) |
There was a problem hiding this comment.
The problem with this config is that we move it only at 'project configuration' step, i.e. when build routine was initialized first time. As a result, one needs to know that actually he has two versions of config, but only one of them used in the debug mode. That was solved previously by explicit failure on binary start which notified developer that he needs config of some sort.
I would suggest to solve this problem in a different way if possible:
- Rename original config to BotConfig.template.txt (or BotConfig.example.txt). This step is needed to say explicitly that we do not use it anyhow.
- Rename the bin folder in CommandCenter project to config (to be explicit).
- Copy it during configuration time somewhere inside the build folder.
- Use the same config file for release and debug mode.
There was a problem hiding this comment.
Use the same config file for release and debug mode.
I'll have to play with this some. Seems Visual Studio creates up to 4 folders (Release, Debug, MinSizeRel, RelWithDebInfo).
One thought was to create a symlink in each folder, but then we are stuck with target_link name.
There was a problem hiding this comment.
I think the proper way to solve this issue would introduction of '-c, --config' option with location of the config. Looks much simpler, not requires copying/linking and could help in other scenarous.
| set_target_properties( CommandCenter PROPERTIES | ||
| VS_DEBUGGER_WORKING_DIRECTORY "$<TARGET_FILE_DIR:CommandCenter>" | ||
| VS_DEBUGGER_COMMAND "$<TARGET_FILE:CommandCenter>" | ||
| VS_DEBUGGER_COMMAND_ARGUMENTS "Ladder2019Season3/AcropolisLE.SC2Map" |
There was a problem hiding this comment.
Ah, and this point is also confusing as we are in 2023 now and SC2AI ladder uses different maps. While it is ok to specify map from the past as an example of "start the bot" command in README, currently one should use modern maps.
In other words, for real bot development this setting does not make sense and should be changed.
And if we set here some up2date map, we have to update it each maps rotation (i.e. each new ladder season).
There was a problem hiding this comment.
User can manually update these in the IDE. These are initially blank variables.
It's an initialization, not a permanent change.
An alternative to launching from the command line.
There was a problem hiding this comment.
I mean that this alternative is invalid from the very beginning (which is ok for README as it contains examples) so one should figure out which values should be used instead in any case.
I believe all the defaults should contain working values or do not contains values at all. For the latter documentation should be added. The suggested approach does not provide working defaults.
|
Also, I am not really aware about the ways VS users tend to organize the code inside IDE. Perhaps you can provide some links to projects/screenshots to compare? |
? ??? To show up in Solution Explorer, needs to also be in BotSources??? doh! remove batchfile ughh w remove cmake stuff wip change bin/ to config/ ; Ineed to rename.. renamed botconfig fix space error wip getting error now. Maybe the batchfiles is no bueno, atm. I done many changes. Good news is the foreach() seems to be working well.
CMakeLists.txt
Outdated
|
|
||
| # Make directory that 'Build' in Visual Studio would, so we can move BotConfig.txt | ||
| file(MAKE_DIRECTORY ${CMAKE_SOURCE_DIR}/build/bin/Debug) | ||
| file(COPY_FILE ${CMAKE_SOURCE_DIR}/bin/BotConfig.txt ${CMAKE_SOURCE_DIR}/build/bin/Debug/BotConfig.txt) |
There was a problem hiding this comment.
I think the proper way to solve this issue would introduction of '-c, --config' option with location of the config. Looks much simpler, not requires copying/linking and could help in other scenarous.
| set_target_properties( CommandCenter PROPERTIES | ||
| VS_DEBUGGER_WORKING_DIRECTORY "$<TARGET_FILE_DIR:CommandCenter>" | ||
| VS_DEBUGGER_COMMAND "$<TARGET_FILE:CommandCenter>" | ||
| VS_DEBUGGER_COMMAND_ARGUMENTS "Ladder2019Season3/AcropolisLE.SC2Map" |
There was a problem hiding this comment.
I mean that this alternative is invalid from the very beginning (which is ok for README as it contains examples) so one should figure out which values should be used instead in any case.
I believe all the defaults should contain working values or do not contains values at all. For the latter documentation should be added. The suggested approach does not provide working defaults.
|
I'll take a look at it again. |
|
Closing. Will look into documentation for users new to Visual Studio. |
Added some Visual Studio defaults. Should be able to hit "Local Windows Debugger" after making, to build and run commandcenter bot through the IDE.