[Feature Request]: internal: GameObject::draw() should be const

[mstoeckl]

Feature Details

The virtual function GameObject::draw() should be made a const member function, as should all implementations of it. Currently classes inheriting GameObject can (but almost never do) update parts of their internal state in draw() instead of doing it all inside update(); making ::draw() be const would (weakly, C++ has escape hatches) prevent them from doing update logic inside draw().

Feature Purpose

As a general principle, drawing an object should not affect its state. The rendering in SuperTux is partially decoupled from the game logic: while often one frame is drawn after each game logic update, frame drawing can be skipped when rendering is slower (due to vsync or lag) than the game's logical update rate, and frames may be drawn more often if the frame prediction option or the debug option to generate duplicate frames is enabled. These conditions should not affect how the game itself evolves and reacts to input events. If frame drawing does affect the game logic, the game may behave non-deterministically, making gameplay hard to repeat and bugs hard to reproduce.

Since GameObject::draw() is not currently const, it is easy to accidentally mutate the object implementing ::draw(). Sometimes this is benign (has no impact, or just makes animations display oddly at low/high frame rate), but it need not always be. Cases of mutation that I've noticed:

• YetiStalactite/LiveFireAsleep/LiveFireDormant: benign, these call MovingSprite::set_action() to choose an image when the Editor is active, which has side effects of modifying collision hitboxes.
• PulsingLight: benign, temporarily modifies its color to share three lines of code with Light::draw.
• TextScroller: benign: only sets m_finished after a ::draw call occurs, but nothing reads m_finished
• PlayerStatusHUD: benign, coin display is updated by draw call and will go faster at higher FPS
• SecretAreaTrigger: benign, object destruction waits until message timer completes and a draw call is made, and there are no object removal listeners for it
• Player: m_was_crawling_before_slide, m_idle_stage, m_idle_timer, and m_kick_timer are modified in draw, but only used by draw, all benign.
• Player: m_skidding_timer: is updated in ::draw(), because m_skidding_timer.check() has a side effect of resetting the timer after time has elapsed and making following calls to .check() return false. While m_skidding_timer is also used in Player::handle_input to determine when the player is currently skidding, this is benign because only m_skidding_timer.started() is called which is unaffected by .check() calls.
• Camera: m_screensize, which affects camera position calculations, is set inside draw(), as a function of the current camera scale . This makes camera update behavior depend on whether there was a draw() since the last update(); I haven't figured out what problems exactly this could cause, but suspect the camera may temporarily exceed boundaries or move more jerkily than necessary when the frame rate is below the logical rate (e.g., when vsync'd to 30 or 60 fps) so that draw() calls are skipped, and the output scale is changing. (On the other hand, SuperTux only really needs to use Camera when drawing -- some parameters are exposed to squirrel, but I don't think any of them are actually used.) This also makes understanding what exactly Camera is doing much more difficult. (Edit: another issue was that Camera::m_screen_size could be out of date by a frame just after the window is resized; see Update camera's screen size on update() instead of previous draw() #3175.)

The benign state mutations in ::draw() are a mixture of easy to fix (or mark as mutable), and sometimes fixing them has minor benefits. Long term, I think making ::draw const has minor costs (because drawing code almost never has side effects in practice, and if one really needs them, mutable variables and const_cast are easy to use) and minor benefits (it may prevent a rare class of non-determinism bugs; and makes it harder to write animations whose speed depends on the frame rate.)

Note: I have not fully tried to make this change, so there may be other complications to making GameObject::draw() be const that I have not thought of.

Guidelines For Reporting Issues

• I have read https://github.com/SuperTux/supertux/blob/master/CONTRIBUTING.md#bug-reports.
• I have verified this isn't a request that's already been submitted as an issue.
• I have verified this isn't a discussion, or an issue with the game, but rather an actual feature request - a currently non-existent, but desired feature.
• In this request, I have only included details about one (1) desired feature.
• If I make a mistake while submitting this request, I agree to use the "Edit" feature to correct it, instead of closing this issue and opening a new one.

[tobbi]

Instead of marking arbitrary data members as mutable, couldn't we introduce virtual before_draw and after_draw methods to the GameObject class that get then overridden in the particular child classes?

[mstoeckl]

couldn't we introduce virtual before_draw and after_draw methods to the GameObject class that get then overridden in the particular child classes?

I would not recommend this.

• All of the cases that I found can be rewritten to not mutate any state during ::draw(), although it may take a bit of time to figure out how best to resolve them. Using mutable should be a temporary measure (unless there really is a case where nondeterministic behavior based on draw calls is needed). The mutable specifier admittedly is rarely used in C++, but this makes it easy to search for any remaining instances, and unlike many other parts of C++, the rules for it are not horribly complicated.
• Having more virtual functions (or more of anything, really) makes the code more complicated and harder to refactor.
• To split draw() into before_draw(),draw() const, and after_draw(), without using mutable, requires extracting those few cases of state mutation noted above out of draw(). If one does not use mutable or some other temporary "escape hatch", figuring out how to do this is the main barrier to making the code compile when ::draw() is const. But then instead of putting the state mutation part into before_draw() or after_draw(), one could just add it into update().