Gamedata initial load exception catch#113
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWraps per-file ZIP processing in GetGamedataFile.LoadGamedata with a try/catch so ZIP read/iteration errors are logged and shown via GenericInfoPopup while allowing the loader to continue with remaining files; minor formatting/whitespace tweaks elsewhere, no public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as GetGamedataFile.LoadGamedata
participant ZIP as ZipArchive
participant Repo as GamedataRepository
participant UI as GenericInfoPopup
participant Log as Debug.LogError
Loader->>ZIP: Open(zipPath) (inside try)
ZIP-->>Loader: Return entries
loop for each entry
Loader->>Loader: normalize path (lowercase, "\\" -> "/")
alt path starts with env/ or textures/ or units/
Loader->>Repo: InsertOrReplace(entry)
else
Loader-->>Loader: skip entry
end
end
alt exception during open/iteration
ZIP-->>Loader: throws
Loader->>Log: LogError(exception)
Loader->>UI: ShowInfo(userMessage)
Loader-->>Loader: continue next filePath
end
Loader->>Loader: update gamedataLoaded / finalize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Assets/Scripts/Ozone SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs (1)
129-131: Aggregate startup error popups instead of one per file.Line 130 enqueues a popup for every failed archive. During preload this can flood the popup queue and degrade startup UX. Prefer collecting failures and showing one summary message after the loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs around lines 129 - 131, The current loop calls Debug.LogError(...) and GenericInfoPopup.ShowInfo(...) for every failed archive (see Debug.LogError, GenericInfoPopup.ShowInfo, filePath inside the loop in GetGamedataFile_Bytes), which floods the UI; instead, accumulate failed filePath entries (or error messages) into a local list during the loop and replace the per-file GenericInfoPopup.ShowInfo call with a single summary popup after the loop completes that lists/counts the failures and logs the aggregated details with Debug.LogError or a single call including the collected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs:
- Around line 88-132: The code opens a FileStream (fs) and constructs a ZipFile
(zipFile) but never disposes them on exception paths; wrap the FileStream and
ZipFile lifecycles in using blocks or ensure a try/finally that calls
zipFile.Close()/Dispose() and fs.Dispose() so both are always released on
success or error. Specifically, change the logic around File.OpenRead(filePath)
and new ZipFile(fs) (symbols: fs and zipFile) to either: 1) use nested using
(var fs = File.OpenRead(...)) { using (var zipFile = new ZipFile(fs)) { ... } }
or 2) initialize zipFile = null and in a finally block call zipFile?.Close() and
fs?.Dispose(); keep existing gamedata/GamedataZipEntry handling unchanged.
Ensure no early continue/catch exits leave fs/zipFile undisposed.
- Around line 127-140: The code sets gamedataLoaded = true unconditionally even
when exceptions occur in the catch inside GetGamedataFile_Bytes; change this by
tracking completion status: introduce a local bool (e.g., allFilesLoaded = true)
before the file-loading loop, set allFilesLoaded = false inside the catch (where
Debug.LogError and GenericInfoPopup.ShowInfo are called), and after the loop
assign gamedataLoaded = allFilesLoaded so gamedataLoaded is only true when no
load errors occurred; alternatively return early on fatal error instead of
unconditionally setting gamedataLoaded.
---
Nitpick comments:
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs:
- Around line 129-131: The current loop calls Debug.LogError(...) and
GenericInfoPopup.ShowInfo(...) for every failed archive (see Debug.LogError,
GenericInfoPopup.ShowInfo, filePath inside the loop in GetGamedataFile_Bytes),
which floods the UI; instead, accumulate failed filePath entries (or error
messages) into a local list during the loop and replace the per-file
GenericInfoPopup.ShowInfo call with a single summary popup after the loop
completes that lists/counts the failures and logs the aggregated details with
Debug.LogError or a single call including the collected errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e505c129-18a1-4500-9fbb-fdd31c1d7e9a
📒 Files selected for processing (1)
Assets/Scripts/Ozone SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
| try | ||
| { | ||
| FileStream fs = File.OpenRead(filePath); | ||
| ZipFile zipFile = new ZipFile(fs); | ||
| int addedFiles = 0; | ||
| foreach (ZipEntry ze in zipFile) | ||
| { | ||
| if (!ze.IsFile) | ||
| continue; | ||
|
|
||
| //string entryPath = ze.Name.ToLower().Replace("\\", "/"); | ||
| string entryPath = ze.Name.ToLower().Replace("\\", "/"); | ||
|
|
||
| if (entryPath.StartsWith("env/") || entryPath.StartsWith("textures/") || entryPath.StartsWith("units/")) | ||
| { // Ignore other paths because they will never be used | ||
|
|
||
| if (gamedata.ContainsKey(entryPath)) | ||
| { | ||
| gamedata[entryPath] = new GamedataZipEntry(zipFile, ze); | ||
| addedFiles++; | ||
| } | ||
| else | ||
| { | ||
| gamedata.Add(entryPath, new GamedataZipEntry(zipFile, ze)); | ||
| addedFiles++; | ||
| //Debug.Log(ze.Name); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| if (addedFiles > 0) | ||
| { | ||
| // Debug.Log("Load SCD: " + filePath + ", added subfiles: " + addedFiles); | ||
| } | ||
| else | ||
| { | ||
| zipFile.Close(); | ||
| } | ||
| } | ||
| catch (System.Exception e) | ||
| { | ||
| Debug.LogError("Error loading file: " + filePath + "\n" + e); | ||
| GenericInfoPopup.ShowInfo("Can't load file: " + filePath + " not found in gamedata!"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Close stream/zip resources on failure paths.
Line 90 opens a FileStream, but if new ZipFile(fs) (Line 91) or ZIP iteration throws, the catch+continue path never disposes that stream/zip object. With per-file recovery, this can accumulate open handles.
🔧 Proposed fix
foreach (string filePath in files)
{
- bool isNx2 = filePath.EndsWith(".nx2");
-
-
- try
+ bool isNx2 = filePath.EndsWith(".nx2");
+ FileStream fs = null;
+ ZipFile zipFile = null;
+ int addedFiles = 0;
+ try
{
- FileStream fs = File.OpenRead(filePath);
- ZipFile zipFile = new ZipFile(fs);
- int addedFiles = 0;
+ fs = File.OpenRead(filePath);
+ zipFile = new ZipFile(fs);
foreach (ZipEntry ze in zipFile)
{
...
}
- if (addedFiles > 0)
+ if (addedFiles > 0)
{
...
}
else
{
zipFile.Close();
+ zipFile = null;
+ fs = null;
}
}
catch (System.Exception e)
{
Debug.LogError("Error loading file: " + filePath + "\n" + e);
GenericInfoPopup.ShowInfo("Can't load file: " + filePath + " not found in gamedata!");
continue;
}
+ finally
+ {
+ // Dispose when archive was not kept for lazy entry reads
+ if (zipFile != null && addedFiles == 0)
+ zipFile.Close();
+ else if (fs != null && zipFile == null)
+ fs.Dispose();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
around lines 88 - 132, The code opens a FileStream (fs) and constructs a ZipFile
(zipFile) but never disposes them on exception paths; wrap the FileStream and
ZipFile lifecycles in using blocks or ensure a try/finally that calls
zipFile.Close()/Dispose() and fs.Dispose() so both are always released on
success or error. Specifically, change the logic around File.OpenRead(filePath)
and new ZipFile(fs) (symbols: fs and zipFile) to either: 1) use nested using
(var fs = File.OpenRead(...)) { using (var zipFile = new ZipFile(fs)) { ... } }
or 2) initialize zipFile = null and in a finally block call zipFile?.Close() and
fs?.Dispose(); keep existing gamedata/GamedataZipEntry handling unchanged.
Ensure no early continue/catch exits leave fs/zipFile undisposed.
| catch (System.Exception e) | ||
| { | ||
| Debug.LogError("Error loading file: " + filePath + "\n" + e); | ||
| GenericInfoPopup.ShowInfo("Can't load file: " + filePath + " not found in gamedata!"); | ||
| continue; | ||
| } | ||
|
|
||
|
|
||
| } | ||
| } | ||
|
|
||
| //TODO Mods | ||
|
|
||
| gamedataLoaded = true; |
There was a problem hiding this comment.
Don’t set gamedataLoaded to true after caught load errors.
Lines 127-132 intentionally recover and continue, but Line 140 still sets gamedataLoaded = true unconditionally. That blocks non-reload retries later, while callers like Assets/Scripts/Common/Preload.cs (Lines 21-38) and Assets/Scripts/Ozone SCMAP Code/MapLuaParser.cs (Lines 78-89) proceed without validating load completeness.
🔧 Proposed fix
public static void LoadGamedata(bool reload = false)
{
Initialize();
+ bool hadErrors = false;
if (gamedataLoaded && !reload)
return;
@@
catch (System.Exception e)
{
+ hadErrors = true;
Debug.LogError("Error loading file: " + filePath + "\n" + e);
GenericInfoPopup.ShowInfo("Can't load file: " + filePath + " not found in gamedata!");
continue;
}
@@
- gamedataLoaded = true;
+ gamedataLoaded = !hadErrors;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
around lines 127 - 140, The code sets gamedataLoaded = true unconditionally even
when exceptions occur in the catch inside GetGamedataFile_Bytes; change this by
tracking completion status: introduce a local bool (e.g., allFilesLoaded = true)
before the file-loading loop, set allFilesLoaded = false inside the catch (where
Debug.LogError and GenericInfoPopup.ShowInfo are called), and after the loop
assign gamedataLoaded = allFilesLoaded so gamedataLoaded is only true when no
load errors occurred; alternatively return early on fatal error instead of
unconditionally setting gamedataLoaded.
|
please do something about the whitespace changes |
Changed whitespaces to tabs
3bcf45a to
d8464d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Assets/Scripts/Ozone SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs (2)
88-125:⚠️ Potential issue | 🟠 MajorDispose
FileStream/ZipFileon failure paths too.This still leaves handles open when
File.OpenRead,new ZipFile(fs), or ZIP iteration throws before the explicit close branch runs. The per-file recovery is good, but the archive/stream lifetime still needs afinally/usingpath.Also applies to: 127-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs around lines 88 - 125, The try block that opens FileStream fs and constructs ZipFile zipFile (used when building GamedataZipEntry) can leak handles if File.OpenRead, new ZipFile(fs), or the ZIP iteration throws; wrap the FileStream and ZipFile lifetimes in proper using blocks or a try/finally that disposes/Closes fs and zipFile (or calls zipFile.Close()) on all exit paths (including exceptions and the branch where addedFiles == 0), ensuring the existing per-file logic that creates new GamedataZipEntry(zipFile, ze) still receives a valid open ZipFile while callers never leak the stream.
127-140:⚠️ Potential issue | 🟠 MajorDon’t mark partial loads as fully loaded.
Once any file hits the catch block,
gamedatais incomplete, but Line 140 still setsgamedataLoaded = true. That prevents a later non-reload retry while callers likeAssets/Scripts/Common/Preload.csandAssets/Scripts/Ozone SCMAP Code/MapLuaParser.cscontinue with partial data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs around lines 127 - 140, The method currently sets gamedataLoaded = true unconditionally even if any file load hit the catch, leaving gamedata partially loaded; modify the logic in GetGamedataFile_Bytes so that you track if any exception occurred (e.g., a local bool like hadLoadError) while iterating file loads and only set gamedataLoaded = true if no errors were encountered, otherwise leave it false (so callers like Preload.cs and MapLuaParser.cs can retry); update any existing early-continue/catch handling to flip the error flag when an exception is caught and ensure gamedataLoaded is only assigned after the loop based on that flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs:
- Around line 129-130: In GetGamedataFile_Bytes (inside the loop that currently
calls Debug.LogError and GenericInfoPopup.ShowInfo per file), stop calling
GenericInfoPopup.ShowInfo for every failed archive; instead collect failed file
names/paths into a local List<string> (e.g. failedFiles) when catching the
exception (where Debug.LogError is used), continue logging each file as you do,
and after the loop show a single aggregated popup via GenericInfoPopup.ShowInfo
summarizing the failures (count and joined file names) so only one popup is
queued; ensure you add the collection and the final ShowInfo call in the same
method that contains the loop.
- Around line 45-52: The ReadBytes method currently calls Stream.Read once which
may return fewer bytes than requested; update GetGamedataFile_Bytes.ReadBytes to
loop reading from the input stream (zipFile.GetInputStream(zipEntry)) into the
allocated byte[] (using zipEntry.Size for length), accumulating an offset and
repeating s.Read until either the buffer is filled (bytesRead == length) or Read
returns 0 (EOF); after the loop, if fewer bytes were read than expected handle
the short-read (e.g., throw or resize) and rely on the existing using block to
close the stream.
---
Duplicate comments:
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs:
- Around line 88-125: The try block that opens FileStream fs and constructs
ZipFile zipFile (used when building GamedataZipEntry) can leak handles if
File.OpenRead, new ZipFile(fs), or the ZIP iteration throws; wrap the FileStream
and ZipFile lifetimes in proper using blocks or a try/finally that
disposes/Closes fs and zipFile (or calls zipFile.Close()) on all exit paths
(including exceptions and the branch where addedFiles == 0), ensuring the
existing per-file logic that creates new GamedataZipEntry(zipFile, ze) still
receives a valid open ZipFile while callers never leak the stream.
- Around line 127-140: The method currently sets gamedataLoaded = true
unconditionally even if any file load hit the catch, leaving gamedata partially
loaded; modify the logic in GetGamedataFile_Bytes so that you track if any
exception occurred (e.g., a local bool like hadLoadError) while iterating file
loads and only set gamedataLoaded = true if no errors were encountered,
otherwise leave it false (so callers like Preload.cs and MapLuaParser.cs can
retry); update any existing early-continue/catch handling to flip the error flag
when an exception is caught and ensure gamedataLoaded is only assigned after the
loop based on that flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afcf2ba0-e1df-444f-8042-4f851291bf85
📒 Files selected for processing (1)
Assets/Scripts/Ozone SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
| public byte[] ReadBytes() | ||
| { | ||
| //byte[] FinalBytes = new byte[4096]; // 4K is optimum | ||
| byte[] FinalBytes = new byte[zipEntry.Size]; | ||
| using (Stream s = zipFile.GetInputStream(zipEntry)) | ||
| { | ||
| s.Read(FinalBytes, 0, FinalBytes.Length); | ||
| s.Close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does System.IO.Stream.Read in C#/.NET guarantee that it fills the requested buffer length, or can it return fewer bytes even before EOF?
💡 Result:
No, System.IO.Stream.Read in C#/.NET does not guarantee that it fills the requested buffer length. It can return fewer bytes than requested even before EOF. The method returns the total number of bytes read, which can be less than the requested count if fewer bytes are available, regardless of whether the end of the stream has been reached. It only returns 0 when the end of the stream is reached.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read?view=net-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read?view=net-6.0
- 3: https://msdn.microsoft.com/library/system.io.stream.read
- 4: https://markheath.net/post/stream-read-guidelines
Read the ZIP entry until the buffer is full.
Stream.Read does not guarantee it fills the entire requested buffer—it can return fewer bytes even before EOF. The current single-call approach can deliver truncated asset data. Loop until the buffer is completely filled or EOF is reached.
🔧 Proposed fix
public byte[] ReadBytes()
{
- //byte[] FinalBytes = new byte[4096]; // 4K is optimum
- byte[] FinalBytes = new byte[zipEntry.Size];
- using (Stream s = zipFile.GetInputStream(zipEntry))
- {
- s.Read(FinalBytes, 0, FinalBytes.Length);
- s.Close();
- }
- return FinalBytes;
+ //byte[] FinalBytes = new byte[4096]; // 4K is optimum
+ byte[] FinalBytes = new byte[zipEntry.Size];
+ using (Stream s = zipFile.GetInputStream(zipEntry))
+ {
+ int offset = 0;
+ while (offset < FinalBytes.Length)
+ {
+ int read = s.Read(FinalBytes, offset, FinalBytes.Length - offset);
+ if (read == 0)
+ throw new EndOfStreamException("Unexpected end of stream while reading " + zipEntry.Name);
+ offset += read;
+ }
+ }
+ return FinalBytes;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public byte[] ReadBytes() | |
| { | |
| //byte[] FinalBytes = new byte[4096]; // 4K is optimum | |
| byte[] FinalBytes = new byte[zipEntry.Size]; | |
| using (Stream s = zipFile.GetInputStream(zipEntry)) | |
| { | |
| s.Read(FinalBytes, 0, FinalBytes.Length); | |
| s.Close(); | |
| public byte[] ReadBytes() | |
| { | |
| //byte[] FinalBytes = new byte[4096]; // 4K is optimum | |
| byte[] FinalBytes = new byte[zipEntry.Size]; | |
| using (Stream s = zipFile.GetInputStream(zipEntry)) | |
| { | |
| int offset = 0; | |
| while (offset < FinalBytes.Length) | |
| { | |
| int read = s.Read(FinalBytes, offset, FinalBytes.Length - offset); | |
| if (read == 0) | |
| throw new EndOfStreamException("Unexpected end of stream while reading " + zipEntry.Name); | |
| offset += read; | |
| } | |
| } | |
| return FinalBytes; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
around lines 45 - 52, The ReadBytes method currently calls Stream.Read once
which may return fewer bytes than requested; update
GetGamedataFile_Bytes.ReadBytes to loop reading from the input stream
(zipFile.GetInputStream(zipEntry)) into the allocated byte[] (using
zipEntry.Size for length), accumulating an offset and repeating s.Read until
either the buffer is filled (bytesRead == length) or Read returns 0 (EOF); after
the loop, if fewer bytes were read than expected handle the short-read (e.g.,
throw or resize) and rely on the existing using block to close the stream.
| Debug.LogError("Error loading file: " + filePath + "\n" + e); | ||
| GenericInfoPopup.ShowInfo("Can't load file: " + filePath + " not found in gamedata!"); |
There was a problem hiding this comment.
Avoid queuing one popup per broken archive.
GenericInfoPopup.ShowInfo appends to a global queue, so multiple bad .scd/.nx2 files will create a long chain of startup popups. Prefer logging each file and showing one aggregated summary after the loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Ozone` SCMAP Code/GetGamedataFile/GetGamedataFile_Bytes.cs
around lines 129 - 130, In GetGamedataFile_Bytes (inside the loop that currently
calls Debug.LogError and GenericInfoPopup.ShowInfo per file), stop calling
GenericInfoPopup.ShowInfo for every failed archive; instead collect failed file
names/paths into a local List<string> (e.g. failedFiles) when catching the
exception (where Debug.LogError is used), continue logging each file as you do,
and after the loop show a single aggregated popup via GenericInfoPopup.ShowInfo
summarizing the failures (count and joined file names) so only one popup is
queued; ensure you add the collection and the final ShowInfo call in the same
method that contains the loop.
You mean spaces instead of tabs? Most companies I work with require spaces, forgot about that. |
Summary by CodeRabbit