fix(housing): register doors, and stop crashing on client component sheets (#2557)
## Summary
Players could not place **any door** while customizing a house, and placing other pieces could disconnect them outright. Staff saw neither problem: `HouseFoundation.Designer_Build` only enforces `ValidPiece` below `GameMaster`.
Original report and diagnosis by @SynPDX.
## Root cause 1 — no door is ever registered
The retail client's `doors.txt` separates its header rows with lines of **bare tabs** (it is the only sheet that does):
```
int<TAB>int<TAB>...<TAB>string
<TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB> <-- 10 tabs, not an empty line
Category<TAB>Piece1<TAB>...<TAB>FeatureMask<TAB>Comment
<TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB><TAB>
0<TAB>1657<TAB>1659<TAB>...
```
`Spreadsheet.ReadLine` skipped a line only when `line.Length > 0`. A 10-tab line has length 10, so it was returned as the **names row** — every column ended up named `""`, `GetColumnID("Piece1")` and friends returned `-1`, and not one of the 230 door graphics was registered. Unregistered item IDs keep the `-1` sentinel, and `CheckValidity` rejects those, so `ValidPiece` refused every door.
ClassicUO skips these lines (`string.IsNullOrWhiteSpace` in `HouseCustomizationManager.ParseFile`), which is why the client happily offers doors the server then rejects.
Measured against a retail 7.0.x `doors.txt` using the shipped `Spreadsheet`:
| | `FeatureMask` column | door graphics registered |
|---|---|---|
| before | `-1` | **0** |
| after | `9` | **230** |
## Root cause 2 — `IndexOutOfRangeException` out of the packet handler
Every sheet ends in a cosmetic `Comment` column that ModernUO never reads, and client sheets write an empty comment as a plain newline with no trailing tab. `Split('\t')` then returns one field fewer than the header declares, and the parser indexed past the end:
```
System.IndexOutOfRangeException: Index was outside the bounds of the array.
at Server.Multis.Spreadsheet..ctor(String path)
at Server.Multis.ComponentVerification.LoadSpreadsheet(...)
at Server.Multis.ComponentVerification.IsItemValid(Int32 itemID)
at Server.Multis.HouseFoundation.ValidPiece(Int32 itemID, Boolean roof)
at Server.Multis.HouseFoundation.Designer_Build(NetState state, ...)
```
The client's own parser only requires the columns up to `FeatureMask` — ClassicUO's `CustomHouseMisc.Parse` guards on `scanf.Length >= 12` for a 13-column `misc.txt` — so such a row is valid data listing real pieces. Missing trailing fields are now treated as empty rather than dropping the row, which would unregister every piece the row lists and reproduce the door symptom.
`EnsureLoaded` also set `_loaded` before loading, so once the throw escaped, an all `-1` table stayed cached and rejected everything for players from then on — the same player-visible symptom as #2500.
## Also made explicit rather than accidental
- **Named the table sentinels.** `NotAComponent` (-1) is the anti-cheat guard and the initial state; `NoFeatureRequired` (0) is a piece with no expansion gate — how `walls.txt` encodes pre-AOS base pieces and what `housing.bin` collapses to under `HousingTierMask` (#2500).
- **A sheet with no `FeatureMask` column is refused and logged.** `GetInt32` on a missing column returns 0 = `NoFeatureRequired`, which would have silently marked every piece in that sheet unconditionally placeable regardless of expansion. This was previously only harmless by accident.
- **A sheet matching none of its expected tile columns is refused and logged** — that is what `doors.txt` was doing silently. Individual missing columns stay tolerated, since older sheets predate columns such as `walls.txt`'s `SecondAltWindowS`/`E`.
- **Catch per sheet**, so one unreadable file no longer costs the other six.
- **Header guards**: an empty file or a types-only file raised a `NullReferenceException`; a names row shorter than the types row indexed past the end.
- **Fall back to the component sheets when `housing.bin` cannot be read**, instead of passing `null` into a `SpanReader`.
`_loaded` is still set before loading, deliberately: this runs from the design packet handler, and retrying would re-read every sheet on each subsequent placement attempt.
Sheet precedence is **unchanged** — the client's copies stay authoritative and `Data/Components` remains the fallback.
## Verification
- Retail 7.0.x client `doors.txt` through the shipped `Spreadsheet`: 0 door graphics before, 230 after.
- 5 new tests in `SpreadsheetTests` covering the tab separators, the omitted trailing field, per-row recovery, and both header guards. All 5 fail against `main` and pass here.
- `dotnet build` clean (0 warnings, 0 errors); `UOContent.Tests` 642/642.
This commit is contained in:
SynPDX
GitHub
parent
aae173a797
commit
86df62fd3e
2 changed files with 220 additions and 11 deletions
114
Projects/UOContent.Tests/Tests/Multis/SpreadsheetTests.cs
Normal file
114
Projects/UOContent.Tests/Tests/Multis/SpreadsheetTests.cs
Normal file
|
|
@ -0,0 +1,114 @@
|
|||
using System;
|
||||
using System.IO;
|
||||
using Server.Multis;
|
||||
using Xunit;
|
||||
|
||||
namespace UOContent.Tests;
|
||||
|
||||
public class SpreadsheetTests : IDisposable
|
||||
{
|
||||
// misc.txt's shape: 13 columns, the last being the Comment the client never reads.
|
||||
private const string Types = "int\tint\tint\tint\tint\tint\tint\tint\tint\tint\tint\tint\tstring";
|
||||
|
||||
private const string Names =
|
||||
"Category\tStyle\tTID\tPiece1\tPiece2\tPiece3\tPiece4\tPiece5\tPiece6\tPiece7\tPiece8\tFeatureMask\tComment";
|
||||
|
||||
private readonly string _path = Path.Combine(Path.GetTempPath(), $"muo-sheet-{Guid.NewGuid():N}.txt");
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
if (File.Exists(_path))
|
||||
{
|
||||
File.Delete(_path);
|
||||
}
|
||||
}
|
||||
|
||||
private Spreadsheet Write(params string[] rows)
|
||||
{
|
||||
var lines = new string[rows.Length + 2];
|
||||
lines[0] = Types;
|
||||
lines[1] = Names;
|
||||
rows.CopyTo(lines, 2);
|
||||
|
||||
File.WriteAllLines(_path, lines);
|
||||
return new Spreadsheet(_path);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void RowMissingTrailingCommentIsStillRead()
|
||||
{
|
||||
// An empty Comment written without a trailing tab leaves the row one field short.
|
||||
var ss = Write("0\t0\t1060056\t44\t0\t41\t40\t42\t0\t43\t29\t8");
|
||||
|
||||
var record = Assert.Single(ss.Records);
|
||||
|
||||
Assert.Equal(44, record.GetInt32(ss.GetColumnID("Piece1")));
|
||||
Assert.Equal(29, record.GetInt32(ss.GetColumnID("Piece8")));
|
||||
Assert.Equal(8, record.GetInt32(ss.GetColumnID("FeatureMask")));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ShortRowDoesNotDropLaterRows()
|
||||
{
|
||||
var ss = Write(
|
||||
"0\t0\t1060056\t44\t0\t41\t40\t42\t0\t43\t29\t0",
|
||||
"0\t1\t1060057\t45\t0\t0\t0\t0\t0\t0\t0\t0\tFieldstone Arches"
|
||||
);
|
||||
|
||||
Assert.Equal(2, ss.Records.Length);
|
||||
Assert.Equal(44, ss.Records[0].GetInt32(ss.GetColumnID("Piece1")));
|
||||
Assert.Equal(45, ss.Records[1].GetInt32(ss.GetColumnID("Piece1")));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TabOnlySeparatorLinesAreNotMistakenForHeaderRows()
|
||||
{
|
||||
// The retail client's doors.txt separates its header rows this way.
|
||||
var separator = new string('\t', 12);
|
||||
|
||||
File.WriteAllLines(
|
||||
_path,
|
||||
[
|
||||
Types,
|
||||
separator,
|
||||
Names,
|
||||
separator,
|
||||
"0\t0\t1060056\t44\t0\t41\t40\t42\t0\t43\t29\t0\tFieldstone Archways"
|
||||
]
|
||||
);
|
||||
|
||||
var ss = new Spreadsheet(_path);
|
||||
|
||||
Assert.Equal(3, ss.GetColumnID("Piece1"));
|
||||
Assert.Equal(11, ss.GetColumnID("FeatureMask"));
|
||||
|
||||
var record = Assert.Single(ss.Records);
|
||||
Assert.Equal(44, record.GetInt32(ss.GetColumnID("Piece1")));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MissingHeaderRowsThrowsInsteadOfNullReference()
|
||||
{
|
||||
File.WriteAllLines(_path, [Types]);
|
||||
|
||||
Assert.Throws<InvalidDataException>(() => new Spreadsheet(_path));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void HeaderWithFewerNamesThanTypesIsTolerated()
|
||||
{
|
||||
File.WriteAllLines(
|
||||
_path,
|
||||
[
|
||||
Types,
|
||||
"Category\tStyle\tTID\tPiece1",
|
||||
"0\t0\t1060056\t44\t0\t41\t40\t42\t0\t43\t29\t0\tFieldstone Archways"
|
||||
]
|
||||
);
|
||||
|
||||
var ss = new Spreadsheet(_path);
|
||||
|
||||
Assert.Equal(44, ss.Records[0].GetInt32(ss.GetColumnID("Piece1")));
|
||||
Assert.Equal(-1, ss.GetColumnID("FeatureMask"));
|
||||
}
|
||||
}
|
||||
|
|
@ -4,11 +4,14 @@ using System.Collections.Generic;
|
|||
using System.IO;
|
||||
using System.IO.Compression;
|
||||
using Server.Compression;
|
||||
using Server.Logging;
|
||||
|
||||
namespace Server.Multis;
|
||||
|
||||
public static class ComponentVerification
|
||||
{
|
||||
private static readonly ILogger logger = LogFactory.GetLogger(typeof(ComponentVerification));
|
||||
|
||||
private static int[] _itemTable;
|
||||
private static int[] _multiTable;
|
||||
private static bool _loaded;
|
||||
|
|
@ -20,6 +23,15 @@ public static class ComponentVerification
|
|||
// encodes them) while AOS/SE/ML/... line up unchanged.
|
||||
private const int HousingTierMask = (int)HousingFlags.HousingEJ;
|
||||
|
||||
// Table sentinels. Slots start as NotAComponent, which CheckValidity rejects, so a piece that
|
||||
// never gets registered can never be placed. NoFeatureRequired is a piece with no expansion
|
||||
// requirement: walls.txt encodes pre-AOS base pieces as 0, housing.bin collapses to 0 under
|
||||
// HousingTierMask.
|
||||
private const int NotAComponent = -1;
|
||||
private const int NoFeatureRequired = 0;
|
||||
|
||||
private const string FeatureMaskColumn = "FeatureMask";
|
||||
|
||||
public static bool IsItemValid(int itemID)
|
||||
{
|
||||
EnsureLoaded();
|
||||
|
|
@ -33,7 +45,8 @@ public static class ComponentVerification
|
|||
}
|
||||
|
||||
private static bool CheckValidity(int val) =>
|
||||
val != -1 && (val == 0 || ((int)ExpansionInfo.CoreExpansion.HousingFlags & val) != 0);
|
||||
val != NotAComponent &&
|
||||
(val == NoFeatureRequired || ((int)ExpansionInfo.CoreExpansion.HousingFlags & val) != 0);
|
||||
|
||||
private static void EnsureLoaded()
|
||||
{
|
||||
|
|
@ -42,22 +55,50 @@ public static class ComponentVerification
|
|||
return;
|
||||
}
|
||||
|
||||
// Set before loading: this runs from the design packet handler, so a bad file must not
|
||||
// re-read every sheet on each later placement attempt. Sheets below fail independently.
|
||||
_loaded = true;
|
||||
|
||||
_itemTable = CreateTable(TileData.MaxItemValue);
|
||||
_multiTable = CreateTable(0x4000);
|
||||
|
||||
var housingPath = MultiData.HousingUOPPath;
|
||||
if (housingPath != null)
|
||||
if (housingPath != null && TryLoadFromHousingBin(housingPath))
|
||||
{
|
||||
var entry = MultiData.HousingEntry;
|
||||
LoadFromHousingBin(ReadUOPEntry(housingPath, entry));
|
||||
return;
|
||||
}
|
||||
|
||||
LoadFromTxtFiles();
|
||||
}
|
||||
|
||||
private static bool TryLoadFromHousingBin(string path)
|
||||
{
|
||||
try
|
||||
{
|
||||
var data = ReadUOPEntry(path, MultiData.HousingEntry);
|
||||
if (data != null)
|
||||
{
|
||||
LoadFromHousingBin(data);
|
||||
return true;
|
||||
}
|
||||
|
||||
logger.Warning(
|
||||
"Could not decompress housing.bin from {Path}. Falling back to the component sheets",
|
||||
path
|
||||
);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
logger.Warning(
|
||||
ex,
|
||||
"Failed to read housing.bin from {Path}. Falling back to the component sheets",
|
||||
path
|
||||
);
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
private static byte[] ReadUOPEntry(string path, UOPEntry entry)
|
||||
{
|
||||
using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read);
|
||||
|
|
@ -223,16 +264,55 @@ public static class ComponentVerification
|
|||
return;
|
||||
}
|
||||
|
||||
var ss = new Spreadsheet(path);
|
||||
Spreadsheet ss;
|
||||
|
||||
try
|
||||
{
|
||||
ss = new Spreadsheet(path);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// One unreadable sheet must not take the others down with it.
|
||||
logger.Error(ex, "Could not read house components from {Path}", path);
|
||||
return;
|
||||
}
|
||||
|
||||
// GetInt32 on a missing column yields NoFeatureRequired, which would register every piece in
|
||||
// the sheet as unconditionally placeable. Refuse the sheet instead.
|
||||
var featureCID = ss.GetColumnID(FeatureMaskColumn);
|
||||
if (featureCID < 0)
|
||||
{
|
||||
logger.Error(
|
||||
"House component sheet {Path} has no {Column} column. Its pieces will not be registered",
|
||||
path,
|
||||
FeatureMaskColumn
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// An individual missing column is expected - older sheets predate walls.txt's
|
||||
// SecondAltWindowS/E - but a sheet matching none of them is not the sheet we expect.
|
||||
var tileCIDs = new int[tileColumns.Length];
|
||||
var matchedColumns = 0;
|
||||
|
||||
for (var i = 0; i < tileColumns.Length; ++i)
|
||||
{
|
||||
tileCIDs[i] = ss.GetColumnID(tileColumns[i]);
|
||||
|
||||
if (tileCIDs[i] >= 0)
|
||||
{
|
||||
matchedColumns++;
|
||||
}
|
||||
}
|
||||
|
||||
var featureCID = ss.GetColumnID("FeatureMask");
|
||||
if (matchedColumns == 0)
|
||||
{
|
||||
logger.Error(
|
||||
"House component sheet {Path} has none of its expected tile columns. Its pieces will not be registered",
|
||||
path
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
for (var i = 0; i < ss.Records.Length; ++i)
|
||||
{
|
||||
|
|
@ -260,7 +340,7 @@ public static class ComponentVerification
|
|||
|
||||
for (var i = 0; i < table.Length; ++i)
|
||||
{
|
||||
table[i] = -1;
|
||||
table[i] = NotAComponent;
|
||||
}
|
||||
|
||||
return table;
|
||||
|
|
@ -277,11 +357,18 @@ public class Spreadsheet
|
|||
var types = ReadLine(ip);
|
||||
var names = ReadLine(ip);
|
||||
|
||||
if (types == null || names == null)
|
||||
{
|
||||
throw new InvalidDataException($"House component sheet '{path}' is missing its header rows.");
|
||||
}
|
||||
|
||||
m_Columns = new ColumnInfo[types.Length];
|
||||
|
||||
for (var i = 0; i < m_Columns.Length; ++i)
|
||||
{
|
||||
m_Columns[i] = new ColumnInfo(i, types[i], names[i]);
|
||||
// A names row shorter than the types row leaves the extras unnamed, so nothing resolves
|
||||
// to them.
|
||||
m_Columns[i] = new ColumnInfo(i, types[i], i < names.Length ? names[i] : "");
|
||||
}
|
||||
|
||||
var records = new List<DataRecord>();
|
||||
|
|
@ -294,10 +381,15 @@ public class Spreadsheet
|
|||
{
|
||||
var ci = m_Columns[i];
|
||||
|
||||
// Client sheets write an empty trailing Comment as a plain newline, leaving the row
|
||||
// one field short. The client only requires the columns up to FeatureMask, so treat
|
||||
// the missing field as empty rather than dropping a row that lists real pieces.
|
||||
var value = ci.m_DataIndex < values.Length ? values[ci.m_DataIndex] : null;
|
||||
|
||||
data[i] = ci.m_Type switch
|
||||
{
|
||||
"int" => Utility.ToInt32(values[ci.m_DataIndex]),
|
||||
"string" => values[ci.m_DataIndex],
|
||||
"int" => Utility.ToInt32(value),
|
||||
"string" => value,
|
||||
_ => data[i]
|
||||
};
|
||||
}
|
||||
|
|
@ -327,7 +419,10 @@ public class Spreadsheet
|
|||
{
|
||||
while (ip.ReadLine() is { } line)
|
||||
{
|
||||
if (line.Length > 0)
|
||||
// Whitespace-only, not merely empty: the retail client's doors.txt separates its header
|
||||
// rows with lines of bare tabs, and accepting one as the names row leaves every column
|
||||
// unnamed, so no door resolves. The client skips them the same way.
|
||||
if (!string.IsNullOrWhiteSpace(line))
|
||||
{
|
||||
return line.Split('\t');
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue