Cataclysm-dda: Invalid terrain id error when moving to/off movement-slowing vehicle tiles

[Miner239]

Describe the bug

Movement-slowing vehicle tiles results in invalid terrain id error upon moving to/off the tile.

Steps To Reproduce

1. Move into a movement-slowing vehicle tile.

Expected behavior

No error.

Screenshots

Versions and configuration

Version: 0.E-1946-ged9f3b9 (tiles)
Mods: dda

Additional context

Presuming that the message string function is not handling vehicle tile name conversion properly.

[nphyx]

I can confirm this exists. Were you able to narrow it down to only the slow-moving tiles?

Bug exists in generic_factory.h. I took a quick look but don't know much about this part of the code and have not attempted to fix yet.

This debug log has several examples: https://pastebin.com/yxP3B567

[Miner239]

Moving to aisles doesn't produce the error. I'm manually chasing function calls and am stuck looking for vehicle part info_cache and the sfx class...

[kevingranade]

This issue has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/bug-vehicle-tile-bug-at-cataclysmdda-0-e-windows-x64-tiles-b10684/23725/2

[anothersimulacrum]

Turns out I introduced this bug.
This bug only occurs when you have a soundpack enabled, and happens because I'm unconditionally converting the id passed to sfx::do_obstacle() into a terrain id, even if it might not be.
Before #40304, it just checked that string against a list of hardcoded terrain ids.
Proposed solution: implement int_id<ter_t>::is_valid(), then apply this patch:

diff --git a/src/sounds.cpp b/src/sounds.cpp
index 39f5357ea4..8f22eb2a17 100644
--- a/src/sounds.cpp
+++ b/src/sounds.cpp
@@ -1435,8 +1435,8 @@ void sfx::do_obstacle( const std::string &obst )
     int heard_volume = sfx::get_heard_volume( g->u.pos() );
     if( sfx::has_variant_sound( "plmove", obst ) ) {
         play_variant_sound( "plmove", obst, heard_volume, 0, 0.8, 1.2 );
-    } else if( ter_id( obst )->has_flag( TFLAG_SHALLOW_WATER ) ||
-               ter_id( obst )->has_flag( TFLAG_DEEP_WATER ) ) {
+    } else if( ter_str_id( obst ).is_valid() && ( ter_id( obst )->has_flag( TFLAG_SHALLOW_WATER ) ||
+               ter_id( obst )->has_flag( TFLAG_DEEP_WATER ) ) ) {
         play_variant_sound( "plmove", "walk_water", heard_volume, 0, 0.8, 1.2 );
     } else {
         play_variant_sound( "plmove", "clear_obstacle", heard_volume, 0, 0.8, 1.2 );

EDIT: Turns out int_id<ter_t>::is_valid() is implemented, but I get linker errors trying to use it, so the task is actually to figure that out.
EDIT2: Thanks for the correction, this patch should work (if there aren't linker errors)

[floyza]

It seems that the error is not occurring from ter_id::has_flag, but the constructor for ter_id, so ter_id( obst ).is_valid() won't work.

[anothersimulacrum]

It will. The error is occurring when the game is trying to turn the terrain id into a terrain and coming up empty handed.

[floyza]

But the constructor for ter_t calls string_id::id, which in turn calls terrain_data.convert, which is what causes the error. I ran through it with a debugger, and the error occurred during the constructor. It seems that ter_id expects a valid terrain as an input, whether or not this is intended, since generic_factory::convert generates an error when given an invalid terrain.

[anothersimulacrum]

You're right, my bad, I was misremembering the debugging I did. These (or the one validity is tested on) should be ter_str_ids. I'm not sure if that resolves the linker errors I was getting.