Behavior differences between Debug/Release

[tophyr]

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 #527 (comment)

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 #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:

  Descent3/misc/pserror.h

  Lines 236 to 261 in a78eb53

  	#include "SDL.h"
  	// For some reason Linux doesn't like the \ continuation character, so I have to uglify this
  	#define DEBUG_BREAK() \
  	do { \
  	if (DebugBreak_callback_stop) \
  	(*DebugBreak_callback_stop)(); \
  	debug_break(); \
  	if (DebugBreak_callback_resume) \
  	(*DebugBreak_callback_resume)(); \
  	} while (0)
  	#define ASSERT(x) SDL_assert(x)
  	#define Int3() \
  	do { \
  	mprintf(0, "Int3 at %s line %d.\n", __FILE__, __LINE__); \
  	if (Debug_break) \
  	DEBUG_BREAK(); \
  	else \
  	Int3MessageBox(__FILE__, __LINE__); \
  	} while (0)
  	#define HEAPCHECK()
  	#endif
  	#else
  	#define DEBUG_BREAK()
  	#define ASSERT(x)
  	#define Int3()
  	#define HEAPCHECK()

• 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:

  Descent3/Descent3/OsirisLoadandBind.cpp

  Lines 1050 to 1053 in a78eb53

  	#ifdef OSIRISDEBUG
  	ASSERT(osm->RefRoot == NULL);
  	osm->RefRoot = NULL;
  	#endif

• 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 #527 (comment) and #426 (comment). 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.

[tophyr]

@winterheart, @Lgt2x - particularly interested in your thoughts.

[Lgt2x]

Hey, thanks for opening this, there is definitely a discussion to have on the topic.

I think we should prioritize fault discovery with the Debug build, and fault tolerance with the Release build.

Absolutely, Release builds should strive to be as fault-tolerant as possible : recover as much as possible, exiting the program should really be a last resort. Even then, there should be an user-facing error message to explain clearly why the game exited : missing required game files, etc. OpenGL errors can usually be recovered and ignored, to the price of wrong rendering (ie a faulty fragment shader may not render anything, but that's it). Speed should also be a concern, as we also target low-power 64-bit devices.

On the other hand, Debug builds should prioritize developer experience over speed, including (of course) usable debug symbols. It should allow to debug the user-facing app (release) as accurately as possible, which is only possible when both builds differ as little as possible.

Now, the harder question: what does that mean in practice, and what actual differences should there be between both builds?

For context, a very nasty bug @winterheart spent time fixing this week-end involved a movie only played on release builds. I assume he had that in mind when commenting on #527 . In this case, the difference between debug and release did not make sense: it was probably a developer that got annoyed by the movie playing every time when creating a new pilot, and disabled it for debug builds. Some 25 years later, this caused a race condition that only occurred in Release builds.

There are probably other occurrences over the code base of code added or removed at compile-time that we should definitely eliminate to help with game testing and debugging (what's that?)

Now, some debug/release checks make sense, such as those you pointed with DEBUG_BREAK, asserts, etc. Testing pre-conditions and post-conditions with assertions is a sane thing to do to catch errors early, but should be disabled for release.
They should not have side-effects effectively changing code behavior. The example you pointed is interesting:

Descent3/Descent3/OsirisLoadandBind.cpp

Lines 1050 to 1053 in a78eb53

	#ifdef OSIRISDEBUG
	ASSERT(osm->RefRoot == NULL);
	osm->RefRoot = NULL;
	#endif

There, the assert should not be guarded with #ifdef OSIRISDEBUG, as ASSERT is no-op in RELEASE, assuming OSIRISDEBUG is defined in debug mode. It's also curious to recover with osm->RefRoot = NULL; afterwards, effectively masking the error in OSIRISDEBUG mode and creating an undesired side-effect that changes behavior compared to builds that do not define OSIRISDEBUG : if RefRoot is null here, it would cause problems to the release build.

Chording the "delete" key with various other keys enables extra onscreen information: https://github.com/DescentDevelopers/Descent3/blob/main/ddio/key.cpp#L288-L291

This is yet another interesting one. DEBUG provides an entire subsystem with new keybindings allowing cheats that is totally disabled in Release. This could be controversial, but I'd allow it through a menu option in both builds. If the player wants to cheat, they should be able to do it whatever build they choose.

[Lgt2x]

The current advice for addition new developer-specific behavior is to gate the behavior behind a CMake option flag, as demonstrated in #527 (comment) and #426 (comment). 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.)

Build flags to select options are something stupid library devs (I include myself in this category) use extensively, because you may need certain features and not others from a library. This is great, because you can get minimal builds suited exactly for your needs. My brain is not wired for gamedev enough just yet, so it appeared natural to use them for this project as well. Now that logging does not require a third-party anymore, the flag option does not make sense anymore. It should definitely be a runtime option usable for all builds.

We could keep options for features that require optional third-parties, eg SDK for a particular platform, or experimental features so we can compare easily with the previous version without committing too much into it, eg render refactor (not saying that you should do it)

Does all this make sense to you? I'd happily change my mind on any point made.

[tophyr]

Agree with pretty much everything! The "debug keys" is a weird thing to gate on actual debug build, for sure, and the Osiris logic seems not-really-fully-comprehensive-at-best in either configuration. I definitely also like your idea of using build flags to enable building esoteric or customized settings. For example, as soon as I get D3 for Android to some semblance of usability, I'm going to immediately begin work on D3 for Oculus and that will entail integrating OpenXR - something that @JeodC and his RPi work will likely find outright incompatible.

I'd like Debug and Release to have as-similar-as-possible gameplay/graphics/UX, but also set the expectation that Debug will not be as reliably "playable" due to the reliability and performance hits of added runtime checks. Or, to put it another way, I think we should expect the Debug build to be run under a debugger - if someone says they were running a Debug build not under a debugger, our first thoughts should be "...why?" We could also offer build flags to control the behavior of some of these features, but for example I think that developers should have to turn ASAN off in Debug builds, not have to turn it on.

For @winterheart's recent movie-playing nightmare, I see the RelWithDebugInfo build as the tool to use in that situation. I wholeheartedly agree that it shouldn't have occurred in the first place, but RelWithDebugInfo would be the perfect tool to examine release-build behavior, under a debugger.

[Lgt2x]

The debug keys are, well, a debug facility. If we integrate them into a release build as an option, we'll have to make sure the feature is stable and tested, as opposed to "quickly hacked together to get specific things debugged easily"

[JeodC]

I've actually never used a rpi, so I'm not sure if it's similar enough to be in the same category as what I built it for.

As for your goals, I have zero complaints, though I'm sure others would appreciate that specific platform deviations that introduce incompatibilities with other platforms be confined to preprocessor definitions.

[tophyr]

I've actually never used a rpi, so I'm not sure if it's similar enough to be in the same category as what I built it for.

Oh haha well bad example then but basically the VR stuff I want to add will be irrelevant-at-best, incompatible-at-worst for a good number of platforms. For that, a build flag (in addition to a platform check) would be a good fit.

[tophyr]

This likely should have been a Discussion, now that I'm more familiar with that functionality of GitHub. Oh well 🤷‍♂️

@Lgt2x, sounds like you and I are fairly aligned on the goal of keeping Release builds focused on stability and runtime speed, and Debug builds focused on correctness and development speed. @winterheart, does that sound like something you'd align with as well?

For concrete debate points, here are some project decisions/directions I'd like to take that this general goal would affect:

• Debug builds should include most, if not all, possible correctness asserts available to the platform - MSVC's iterator checks, for example, or LLVM's "Extensive" or even "Debug" Hardening Mode.
• Eventual use of ASAN on-by-default in Debug builds
  • I say "eventual" because ASAN pretty much immediately asserts upon startup - significant cleanup will be needed to get to the point where an ASAN build could be considered "usable". Once we reach that point though, I think ASAN-on should become the default for Debug so that developers have to explicitly turn it off (via a build flag) if they want to skip it.
• Code cleanup efforts to isolate assertions and various other checks out of Release (and RelWithDebInfo) builds, and adding recovery-attempt logic (or nice error reporting UI) in places that make sense
• Handling OpenGL errors via fallback and graceful degradation in Release builds, but assertions in Debug builds
• etc

How contentious do these ideas sound to you guys?