Read per-texture properties from a new TXPR chunk in POF files

code/cfile/cfile.cpp

@@ -34,6 +34,7 @@
 #include "cfilesystem.h"
 
 
+#include <algorithm>
 #include <limits>
 
 char Cfile_root_dir[CFILE_ROOT_DIRECTORY_LEN] = "";
@@ -1107,26 +1108,37 @@ void cfread_string(char *buf, int n, CFILE *file)
 
 void cfread_string_len(char *buf, int buffer_size, CFILE *file)
 {
-	int len;
-	len = cfread_int(file);
-	Assertion( (len <= buffer_size), "String cannot fit in buffer, len: %i, buffer: %i", len, buffer_size);
-	if (len) {
-		cfread(buf, len, 1, file);
-		Assertion(len != buffer_size || buf[len - 1] == 0, "Unterminated string does not fit in buffer");
+	Assertion(buffer_size > 0, "cfread_string_len needs room for at least the terminator, buffer: %i", buffer_size);
+
+	int len = cfread_int(file);
+	int to_read = std::clamp(len, 0, buffer_size - 1);
+
+	if (to_read > 0 && cfread(buf, to_read, 1, file) != 1) {
+		to_read = 0;
 	}
+	buf[to_read] = 0;
 
-	buf[MIN(len, buffer_size - 1)] = 0;
+	if (len > to_read) {
+		if (len > buffer_size) {
+			mprintf(("cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s\n", len, buffer_size, cf_get_filename(file)));
+		}
+		// skip what didn't fit so the next read starts after this string
+		cfseek(file, len - to_read, CF_SEEK_CUR);
+	}
 }
 
 SCP_string cfread_string_len(CFILE *file)
 {
 	int len = cfread_int(file);
+	// a corrupt length must not make us allocate more than the file can still hold
+	len = std::clamp(len, 0, std::max(cfilelength(file) - cftell(file), 0));
 
 	SCP_string str;
 	str.resize(len);
 
-	if (len)
-		cfread(&str[0], len, 1, file);
+	if (len > 0 && cfread(str.data(), len, 1, file) != 1) {
+		str.clear();
+	}
 
 	return str;
 }

code/cfile/cfile.h

@@ -320,8 +320,9 @@ void cfread_string(char *buf,int n, CFILE *file);
 /**
  * @brief Read a fixed length string that is not null-terminated, with the length stored in file.
  *
- * If it is not null-terminated, the length of the string *must* be less than the size of the buffer (to make room for one)
- * 
+ * Reads at most buffer_size - 1 characters and skips the rest of the string in the file, so the next read
+ * starts after it. A negative length reads nothing.
+ *
  * @param buf Pre-allocated array to store string
  * @param buffer_size Size of pre-allocated array
  * @param file File to read from
@@ -332,6 +333,9 @@ void cfread_string_len(char *buf,int buffer_size, CFILE *file);
 
 /**
  * @brief Read a string from the file where the length is stored in the file
+ *
+ * A negative length reads nothing, and a length past the end of the file reads only what is left.
+ *
  * @param file The file to read the string from
  * @return The string that was read
  */

code/model/model.h

@@ -904,6 +904,7 @@ class polymodel
 	// texture maps for model
 	int n_textures;
 	texture_map	maps[MAX_MODEL_TEXTURES];
+	SCP_vector<SCP_string> texture_properties;
 
 	std::shared_ptr<bsp_info[]> submodel;							// an array of size n_models of submodel info.
 

code/model/modelread.cpp

@@ -2937,6 +2937,24 @@ modelread_status read_model_file_no_subsys(polymodel * pm, const char* filename,
 				}
 				break;
 
+			case ID_TXPR: {
+				int n = pof_read_count(fp, filename, "texture properties", MAX_MODEL_TEXTURES);
+				pm->texture_properties.clear();
+				for (i = 0; i < n && cftell(fp) < next_chunk; i++) {
+					auto tex_props = cfread_string_len(fp);
+					if (cftell(fp) > next_chunk) {
+						Warning(LOCATION,
+							"Model %s has a texture properties string that runs past the end of its chunk!",
+							filename);
+						break;
+					}
+					// POF writers pad strings with zero bytes for alignment
+					tex_props.resize(strlen(tex_props.c_str()));
+					pm->texture_properties.push_back(std::move(tex_props));
+				}
+				break;
+			}
+
 			// autocentering info
 			case ID_ACEN:
 				cfread_vector(&pm->autocenter, fp);
@@ -2965,6 +2983,14 @@ modelread_status read_model_file_no_subsys(polymodel * pm, const char* filename,
 		Warning(LOCATION, "Model %s has %d textures, but only %d are supported!", filename, n_textures_in_file, MAX_MODEL_TEXTURES);
 		return modelread_status::FAIL;
 	}
+	if (!pm->texture_properties.empty() && static_cast<int>(pm->texture_properties.size()) != pm->n_textures) {
+		Warning(LOCATION,
+			"Model %s has texture properties for %d textures, but it has %d textures!",
+			filename,
+			static_cast<int>(pm->texture_properties.size()),
+			pm->n_textures);
+	}
+	pm->texture_properties.resize(pm->n_textures);
 	for (i = 0; i < pm->n_detail_levels; i++) {
 		if (pm->detail[i] < 0 || pm->detail[i] >= pm->n_models) {
 			Warning(LOCATION, "Model %s detail %d is %d which is not a valid submodel!", pm->filename, i, pm->detail[i]);

code/model/modelreplace.cpp

@@ -414,6 +414,7 @@ void VirtualPOFOperationAddSubmodel::process(polymodel* pm, model_read_deferred_
 				return;
 			}
 			pm->n_textures = new_n_textures;
+			pm->texture_properties.resize(new_n_textures);
 
 			int old_n_submodel = reallocate_and_copy_array(pm->submodel, pm->n_models, to_copy_submodels.size());
 
@@ -477,6 +478,9 @@ void VirtualPOFOperationAddSubmodel::process(polymodel* pm, model_read_deferred_
 			//Actually copy textures
 			for (const auto& usedTexture : textureIDReplace) {
 				pm->maps[usedTexture.second] = appendingPM->maps[usedTexture.first];
+				// polygons can name a texture index the source POF never listed
+				if (usedTexture.first < static_cast<int>(appendingPM->texture_properties.size()))
+					pm->texture_properties[usedTexture.second] = appendingPM->texture_properties[usedTexture.first];
 				appendingPMholder->keepTexture(usedTexture.first);
 			}
 

code/model/modelsinc.h

@@ -41,6 +41,7 @@ class polymodel;
 #define ID_OBJ2 0x324a424f			// 2JBO (OBJ2): Subobject header
 
 #define ID_TXTR 0x52545854				// RTXT (TXTR): Texture filename list
+#define ID_TXPR 0x52505854				// RPXT (TXPR): Per-texture properties, one string per TXTR entry
 #define ID_INFO 0x464e4950				// FNIP (PINF): POF file information, like command line, etc
 #define ID_GRID 0x44495247				// DIRG (GRID): Grid information
 #define ID_SPCL 0x4c435053				// LCPS (SPCL): Special object -- like a gun, missile, docking point, etc.

test/src/cfile/cfile.cpp

@@ -1,4 +1,5 @@
 
+#include <cfile/cfile.h>
 #include <cfile/cfilesystem.h>
 #include <graphics/font.h>
 #include <gtest/gtest.h>
@@ -212,3 +213,119 @@ TEST_F(CFileTest, subfolder_list)
 	ASSERT_EQ(2, cf_get_file_list(table_files, CF_TYPE_TABLES, "*\\*.tbl", CF_SORT_NAME));
 	ASSERT_TRUE(table_files.back().substr(0, 6) == "folder");
 }
+
+namespace {
+// Builds the bytes cfwrite_string_len writes (a little-endian length, then the characters) as an in-memory file.
+// The file reads from this object's buffer, so the object has to outlive the CFILE.
+class StringLenFile {
+  public:
+	StringLenFile& add_int(int value)
+	{
+		for (int i = 0; i < 4; ++i) {
+			_bytes.push_back(static_cast<char>((value >> (8 * i)) & 0xFF));
+		}
+		return *this;
+	}
+
+	StringLenFile& add_chars(const char* chars, size_t count)
+	{
+		_bytes.insert(_bytes.end(), chars, chars + count);
+		return *this;
+	}
+
+	CFILE* open() const
+	{
+		CFileLocation res(true);
+		res.name_ext = "string_len.bin";
+		res.full_name = "string_len.bin";
+		res.data_ptr = _bytes.data();
+		res.size = _bytes.size();
+		return cfopen_special(res, "rb");
+	}
+
+  private:
+	SCP_vector<char> _bytes;
+};
+} // namespace
+
+TEST_F(CFileTest, string_len_fits)
+{
+	StringLenFile file;
+	file.add_int(3).add_chars("abc", 3).add_int(42);
+	auto fp = file.open();
+	ASSERT_NE(fp, nullptr);
+
+	char buf[8];
+	cfread_string_len(buf, sizeof(buf), fp);
+	EXPECT_STREQ(buf, "abc");
+	EXPECT_EQ(cfread_int(fp), 42);
+
+	cfclose(fp);
+}
+
+TEST_F(CFileTest, string_len_exact_fit_with_stored_terminator)
+{
+	StringLenFile file;
+	file.add_int(4).add_chars("abc\0", 4).add_int(42);
+	auto fp = file.open();
+	ASSERT_NE(fp, nullptr);
+
+	char buf[4];
+	cfread_string_len(buf, sizeof(buf), fp);
+	EXPECT_STREQ(buf, "abc");
+	EXPECT_EQ(cfread_int(fp), 42);
+
+	cfclose(fp);
+}
+
+TEST_F(CFileTest, string_len_longer_than_buffer)
+{
+	StringLenFile file;
+	file.add_int(10).add_chars("0123456789", 10).add_int(42);
+	auto fp = file.open();
+	ASSERT_NE(fp, nullptr);
+
+	char buf[8];
+	memset(buf, 'X', sizeof(buf));
+	cfread_string_len(buf, 4, fp);
+	EXPECT_STREQ(buf, "012");
+	for (int i = 4; i < 8; ++i) {
+		EXPECT_EQ(buf[i], 'X') << "wrote past the buffer at " << i;
+	}
+	EXPECT_EQ(cfread_int(fp), 42);
+
+	cfclose(fp);
+}
+
+TEST_F(CFileTest, string_len_zero_and_negative)
+{
+	StringLenFile file;
+	file.add_int(0).add_int(-5).add_int(42);
+	auto fp = file.open();
+	ASSERT_NE(fp, nullptr);
+
+	char buf[8] = "old";
+	cfread_string_len(buf, sizeof(buf), fp);
+	EXPECT_STREQ(buf, "");
+
+	strcpy_s(buf, "old");
+	cfread_string_len(buf, sizeof(buf), fp);
+	EXPECT_STREQ(buf, "");
+	EXPECT_EQ(cfread_int(fp), 42);
+
+	cfclose(fp);
+}
+
+TEST_F(CFileTest, string_len_scp_string)
+{
+	StringLenFile file;
+	file.add_int(3).add_chars("abc", 3).add_int(-5).add_int(1000000).add_chars("de", 2);
+	auto fp = file.open();
+	ASSERT_NE(fp, nullptr);
+
+	EXPECT_EQ(cfread_string_len(fp), "abc");
+	EXPECT_EQ(cfread_string_len(fp), "");
+	EXPECT_EQ(cfread_string_len(fp), "de");
+
+	cfclose(fp);
+}

test/src/model/test_texture_properties.cpp

@@ -0,0 +1,117 @@
+#include <gtest/gtest.h>
+#include <model/model.h>
+#include <model/modelreplace.h>
+
+#include "util/FSTestFixture.h"
+
+// The fixtures are a three-texture cube saved by POF-Tools; txpr_count_*.pof and txpr_overrun.pof are byte-patched
+// copies of txpr_cube.pof (count or last string length changed, an unknown chunk appended) that POF-Tools can't write.
+
+class ModelTexturePropertiesTest : public test::FSTestFixture {
+public:
+	ModelTexturePropertiesTest() : test::FSTestFixture(INIT_CFILE | INIT_GRAPHICS) {
+		pushModDir("model");
+	}
+
+protected:
+	void SetUp() override {
+		test::FSTestFixture::SetUp();
+		// model_init() may only run once per process; this initializes on first use and otherwise frees
+		model_free_all();
+	}
+
+	void TearDown() override {
+		model_free_all();
+		test::FSTestFixture::TearDown();
+	}
+
+	static SCP_string long_properties() {
+		SCP_string props = "$curve=";
+		for (int i = 0; i < 1199; i++) {
+			props += "0.5, ";
+		}
+		return props + "x";
+	}
+
+	static const SCP_vector<SCP_string>& load_properties(const char* filename) {
+		int model_num = model_load(filename, nullptr, ErrorType::NONE);
+		EXPECT_GE(model_num, 0);
+		return model_get(model_num)->texture_properties;
+	}
+};
+
+TEST_F(ModelTexturePropertiesTest, read_from_txpr) {
+	auto& props = load_properties("txpr_cube.pof");
+
+	ASSERT_EQ(props.size(), 3u);
+	EXPECT_EQ(props[0], "$hull=1");
+	EXPECT_EQ(props[1], "");
+	EXPECT_EQ(props[2].size(), 6003u);
+	EXPECT_EQ(props[2], long_properties());
+}
+
+TEST_F(ModelTexturePropertiesTest, empty_without_txpr) {
+	auto& props = load_properties("no_txpr_cube.pof");
+
+	ASSERT_EQ(props.size(), 3u);
+	EXPECT_TRUE(props[0].empty());
+	EXPECT_TRUE(props[1].empty());
+	EXPECT_TRUE(props[2].empty());
+}
+
+TEST_F(ModelTexturePropertiesTest, count_below_textures_warns_and_pads) {
+#ifndef NDEBUG
+	GTEST_SKIP() << "in debug test builds Warning() throws mid-load and leaves parsing paused for later tests";
+#else
+	int warnings = Global_warning_count;
+	auto& props = load_properties("txpr_count_low.pof");
+
+	EXPECT_EQ(Global_warning_count, warnings + 1);
+	ASSERT_EQ(props.size(), 3u);
+	EXPECT_EQ(props[0], "$hull=1");
+	EXPECT_TRUE(props[1].empty());
+	EXPECT_TRUE(props[2].empty());
+#endif
+}
+
+TEST_F(ModelTexturePropertiesTest, count_above_strings_stops_at_chunk_end) {
+	int warnings = Global_warning_count;
+	auto& props = load_properties("txpr_count_high.pof");
+
+	EXPECT_EQ(Global_warning_count, warnings);
+	ASSERT_EQ(props.size(), 3u);
+	EXPECT_EQ(props[0], "$hull=1");
+	EXPECT_EQ(props[1], "");
+	EXPECT_EQ(props[2], long_properties());
+}
+
+TEST_F(ModelTexturePropertiesTest, string_past_chunk_end_is_dropped) {
+#ifndef NDEBUG
+	GTEST_SKIP() << "in debug test builds Warning() throws mid-load and leaves parsing paused for later tests";
+#else
+	int warnings = Global_warning_count;
+	auto& props = load_properties("txpr_overrun.pof");
+
+	EXPECT_EQ(Global_warning_count, warnings + 2);
+	ASSERT_EQ(props.size(), 3u);
+	EXPECT_EQ(props[0], "$hull=1");
+	EXPECT_EQ(props[1], "");
+	EXPECT_TRUE(props[2].empty());
+#endif
+}
+
+TEST_F(ModelTexturePropertiesTest, virtual_pof_keeps_added_textures_properties) {
+	virtual_pof_init();
+	int model_num = model_load("vp_texprops.pof", nullptr, ErrorType::NONE);
+	ASSERT_GE(model_num, 0);
+	auto pm = model_get(model_num);
+
+	ASSERT_EQ(pm->n_textures, 6);
+	ASSERT_EQ(pm->texture_properties.size(), 6u);
+	EXPECT_TRUE(pm->texture_properties[0].empty());
+	EXPECT_TRUE(pm->texture_properties[1].empty());
+	EXPECT_TRUE(pm->texture_properties[2].empty());
+	EXPECT_EQ(pm->texture_properties[3], "$hull=1");
+	EXPECT_EQ(pm->texture_properties[4], "");
+	EXPECT_EQ(pm->texture_properties[5], long_properties());
+}