DescentDevelopers / DescentDevelopers/Descent3
Behavior differences between Debug/Release
- Dominant language
- C++
- Stars
- 3k
- Forks
- 285
- PR merge metrics
- No merged PRs in 30d
Description
> OK, but we still need same behavior both for Debug and Release builds. Let's just replace throwing with verbose logging and unify this block for all environments.
_Originally posted by @winterheart in https://github.com/DescentDevelopers/Descent3/pull/527#discussion_r1716846993_
### I think the product requirement that Debug and Release must behave identically is detrimental.
It's a fairly common practice to sprinkle assertions and sanity-checks throughout code that are active only in Debug builds and get compiled out in Release builds, in order to let developers catch problems and bad assumptions but still keep non-developer builds stable and performant. In many cases (including in https://github.com/DescentDevelopers/Descent3/pull/527 stemming this discussion) the error information being reported would be useless at best - and more often actively confusing - to a non-developer, but could offer a critically-important clue to someone with the tools to properly analyze it.
To wit, there are already numerous existing cases in the codebase where additional developer-oriented behavior is enabled when building in Debug mode:
* The commonly-used `DEBUG_BREAK()`, `ASSERT()`, and `Int3()` macros are only operational in Debug builds: https://github.com/DescentDevelopers/Descent3/blob/a78eb53f692520379bde498a2dd96a5781a1d04c/misc/pserror.h#L236-L261
* Chording the "delete" key with various other keys enables extra onscreen information: https://github.com/DescentDevelopers/Descent3/blob/main/ddio/key.cpp#L288-L291
* Level loading includes additional sanity checks: https://github.com/DescentDevelopers/Descent3/blob/a78eb53f692520379bde498a2dd96a5781a1d04c/Descent3/OsirisLoadandBind.cpp#L1050-L1053
* Many more examples are discoverable simply by searching for `#ifdef _DEBUG`
The current advice for addition new developer-specific behavior is to gate the behavior behind a CMake option flag, as demonstrated in https://github.com/DescentDevelopers/Descent3/pull/527#discussion_r1716954861 and https://github.com/DescentDevelopers/Descent3/pull/426#issuecomment-2169770852. I think this is a poor solution however because CMake option flags are, realistically, very rarely set. To use my own ignorance as an example, I only recently understood that `mprintf()` is gated behind the `ENABLE_LOGGING` flag (I had, for months, simply been altering `mono.h` to turn it on or off) and only _this morning_ figured out how to actually properly set the value. While my particular level of idiocy here may indeed be a bit comical, I think it is also likely quite indicative of what to expect from any new developers looking to contribute to the project. (And that class of developer is likely the one we most want to be running extra tests during their workflow.)
### I think we should prioritize fault _discovery_ with the Debug build, and fault _tolerance_ with the Release build.
I expect that consumer-oriented use cases, like actually playing the game, will be primarily concerned with things like playability and performance while developer-oriented use cases such as new feature addition and testing will be concerned with correctness and debugging. For situations where the behavior difference prevents in-debugger reproduction of reported issues, CMake offers a middle-ground build type called `RelWithDebugInfo` that we could take advantage of.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.