Remove leftover terrain-based movement cost code.
Description
Details
Event TimelineComment Actions While working on #6513, we stumbled over this issue. [01:42:34] Langbart could we do it like wraitii and make an if statement ? [01:42:36] Langbart https://code.wildfiregames.com/D4459#change-tcKZz2mtyCmK [01:44:26] elexis ehm [01:45:21] elexis that was before bbs commit [01:45:53] elexis if that texture manager commit is not reverted, then you possibly might not only may but have to add that if statement ... [01:58:54] Langbart I am at rP26583 and the -autostart-nonvisual is running correctly. [02:01:04] elexis did you test with a random map or skirmish/sceario? [02:01:28] Langbart binaries/system/pyrogenesis -conf=mod.enabledmods:"public" -autostart="scenarios/arcadia" -autostart-nonvisual [02:02:30] elexis this line [02:02:30] elexis ENSURE(CTerrainTextureManager::IsInitialised()); // we need this for the terrain properties (even when graphics are disabled) [02:02:30] elexis totally looks like 26269 should have changed that [02:03:59] elexis oh [02:04:06] elexis try a random map [02:04:13] elexis I tried the units demo map in that replay [02:04:36] elexis that wasnt a random map actually, hm [02:04:46] elexis a skirmish map that places the entities from a JS simulation script [02:05:21] elexis so the question now is why does our code run CXMLReader::ReadTerrain but at rP26583 it doesnt [02:08:53] elexis grep -R 'Terrain' -l | grep xml | sort [02:09:00] elexis shows that only few maps have this element [02:09:10] elexis units_demo.xml one of them [02:09:22] elexis arcadia not [02:09:41] elexis so try units_demo if you can ... [02:11:55] Langbart arcadia works [02:11:58] Langbart units dem does not [02:12:05] Langbart i am at rP26583 [02:12:15] elexis so you can raise a concern there on that particular line [02:12:23] Langbart do u want the lldb ? [02:12:34] elexis it will be the same [02:12:39] elexis the error is clear now [02:12:48] elexis wraitii removed some lines but not all Line 561 in MapReader.cpp Comment Actions Can you be more specific about that issue ? Are we running into that Ensure ? And if so why ?
Comment Actions -autostart-nonvisual was partially broken before rP26584 was committed. Causing a seg fault. elexis advised to raise a concern here. Comment Actions elexis suggested: diff --git a/source/graphics/MapReader.cpp b/source/graphics/MapReader.cpp index fe6d7df54d..1c4e0b39d7 100644 --- a/source/graphics/MapReader.cpp +++ b/source/graphics/MapReader.cpp @@ -560,8 +560,9 @@ void CXMLReader::ReadTerrain(XMBElement parent) m_MapReader.m_PatchesPerSide = patches; // Load the texture - ENSURE(CTerrainTextureManager::IsInitialised()); // we need this for the terrain properties (even when graphics are disabled) - CTerrainTextureEntry* texentry = g_TexMan.FindTexture(texture); + CTerrainTextureEntry* texentry = nullptr; + if (CTerrainTextureManager::IsInitialised()) + texentry = g_TexMan.FindTexture(texture); m_MapReader.pTerrain->Initialize(patches, NULL); diff --git a/source/graphics/TerrainProperties.cpp b/source/graphics/TerrainProperties.cpp index 5444f42c92..1aa31b6358 100644 --- a/source/graphics/TerrainProperties.cpp +++ b/source/graphics/TerrainProperties.cpp @@ -109,6 +109,12 @@ void CTerrainProperties::LoadXml(XMBElement node, CXeromyces *pFile, const VfsPa boost::char_separator<char> sep(", "); typedef boost::tokenizer<boost::char_separator<char> > tokenizer; tokenizer tok(attr.Value, sep); + // TODO: + // CTerrainProperties::LoadXml also uses g_TexMan without check, + // Is CTerrainProperties used anywhere? + // If no, why wasnt it deleted? + // If yes, is it missing the nonvisual check? + // TODO: Audit all other texture manager uses for(tokenizer::iterator it = tok.begin(); it != tok.end(); ++it) m_Groups.push_back(g_TexMan.FindGroup(*it)); } but this alone will not fix the issue, unless you are below rP26584. Comment Actions CTerrainProperties is used to get the angle and the size of the zoom on the texture for visual mode still. Comment Actions I'm a little confused, what is failing on svn exactly? Or was something failing temporarily in some revision range? It does seem like that call was missed in the revision and should be if-ed away. But that sounds like it should fix the issue? CTerrainProperties is a purely graphical thing now and shouldn't be loaded unless in visual mode, so the use there seems innocuous. No other usages of g_texMan look particularly suspicious to me. Comment Actions
Yes, it should.
If you are below rP26584 you will not be able to run -autostart-nonvisual on a EDIT 30/Apr/22 [04:19:05] elexis also about the texture manager crash, its not random maps that break, but skirmish/scenario maps that define terrain properties | ||||||||||||||||||