-
Notifications
You must be signed in to change notification settings - Fork 61
Add Addressables Report support #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for importing Unity Addressables build reports by parsing JSON build layout files and writing their data into new SQLite tables.
- Introduce
WriteAddressablesBuild
inSQLiteWriter
and register all related commands. - Add dozens of
AbstractCommand
subclasses to support addressables tables. - Update database schema (
Init.sql
) and analyzer tool to handle.json
Addressables build reports.
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
Analyzer/SQLite/SQLiteWriter.cs | Register and use new Addressables build commands; implement WriteAddressablesBuild . |
Analyzer/SQLite/Commands/*.cs | Add command classes for each addressables-related table. |
Analyzer/Resources/Init.sql | Create all addr_build_* tables for addressables data. |
Analyzer/AnalyzerTool.cs | Detect .json files and route to ProcessAddressablesBuild . |
Analyzer/Analyzer.csproj | Add Newtonsoft.Json package for JSON deserialization. |
Comments suppressed due to low confidence (2)
Analyzer/SQLite/SQLiteWriter.cs:62
- [nitpick] The field name 'm_AddressablesDataFromOtherAsset' is missing 'Build' after 'Addressables'. Consider renaming to 'm_AddressablesBuildDataFromOtherAsset' for consistency.
private AddressablesBuildDataFromOtherAsset m_AddressablesDataFromOtherAsset = new AddressablesBuildDataFromOtherAsset();
Analyzer/SQLite/SQLiteWriter.cs:259
- This new method contains substantial logic for parsing and inserting data; consider adding unit tests to cover key code paths and error handling.
public void WriteAddressablesBuild(string filename, BuildLayout buildLayout)
{ | ||
{ "name", SqliteType.Text }, | ||
{ "build_target", SqliteType.Integer }, | ||
{ "start_time", SqliteType.Integer }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field mapping for 'start_time' is defined as SqliteType.Integer, but the table schema declares it as TEXT. Consider changing it to SqliteType.Text to match the schema and avoid type mismatches.
{ "start_time", SqliteType.Integer }, | |
{ "start_time", SqliteType.Text }, |
Copilot uses AI. Check for mistakes.
Analyzer/SQLite/SQLiteWriter.cs
Outdated
using UnityDataTools.FileSystem.TypeTreeReaders; | ||
using UnityDataTools.Analyzer.Build; | ||
using static System.Runtime.InteropServices.JavaScript.JSType; | ||
using System.Xml.Linq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'System.Xml.Linq' namespace is not used in this file; you can remove this import to reduce clutter.
using System.Xml.Linq; |
Copilot uses AI. Check for mistakes.
Analyzer/SQLite/SQLiteWriter.cs
Outdated
using UnityDataTools.FileSystem; | ||
using UnityDataTools.FileSystem.TypeTreeReaders; | ||
using UnityDataTools.Analyzer.Build; | ||
using static System.Runtime.InteropServices.JavaScript.JSType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static import of 'JSType' is not used; removing it will improve clarity.
using static System.Runtime.InteropServices.JavaScript.JSType; |
Copilot uses AI. Check for mistakes.
Analyzer/SQLite/SQLiteWriter.cs
Outdated
m_AddressablesBuildBundleFile.CreateCommand(m_Database); | ||
|
||
// Data From Other Asset Tables | ||
// Data From Other Asset Tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This comment is duplicated on the previous line. Removing one occurrence will keep the code cleaner.
// Data From Other Asset Tables |
Copilot uses AI. Check for mistakes.
Analyzer/AnalyzerTool.cs
Outdated
EraseProgressLine(); | ||
Console.Error.WriteLine(); | ||
Console.Error.WriteLine($"Error processing file: {file}"); | ||
Console.WriteLine($"{e.GetType()}: {e.Message}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.json is a standard file, and there will be .json files in a player output.
For those we should ideally continue to be silent or a minor warning about ignoring it.
Maybe there is a way to first just sanity check that it really is a Addressables build report, perhaps by peeking at the start of the file to check for clear expected formatting?
Or attempting to parsing it but only logging the full error if further analysis proves it is a Addressable file?
Analyzer/Resources/Init.sql
Outdated
PRIMARY KEY (id) | ||
); | ||
|
||
create table addr_build_bundles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the short form will be immediately apparent to everyone "addr_build" as short for for Addressables Build.
create table addr_build_bundles | |
create table addressables_bundles |
I wonder if there is any easy way to make all these tables optional, e.g. only created if we actually find Addressables file? I"m thinking now the dropdown of Tables will start with quite a long list of tables that will be all empty for some builds. Not the end of the world but if they are self contained maybe this can be a sql file that is only run when we discover the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took an initial peek at the code (but didn't try it out)
So far just a few suggestions about organizing the new functionality with a bit more structure, which probably could pay off as establishing a pattern for other build-pipeline-specific tables we might add in future.
{ | ||
internal class AddressablesBuild : AbstractCommand | ||
{ | ||
protected override string TableName { get => "addr_builds"; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected override string TableName { get => "addr_builds"; } | |
protected override string TableName { get => "addressables_builds"; } |
Analyzer/SQLite/SQLiteWriter.cs
Outdated
m_AddressablesBuildSubFile.SetValue("size", reference.data.Size); | ||
m_AddressablesBuildSubFile.ExecuteNonQuery(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQLiteWriter.cs starts to get quite large. Maybe the new Addressables support code can be in a helper class that has its own source file. I can imagine similar pattern would be followed in the future if we add some Player, AssetBundle .manifest, or ContentFile specific information
Maybe AI can help do the leg work for that refactoring :)
4ea43ea
to
6206b19
Compare
This refactors the code to have separate parsers for Asset Bundles and Addressables Build Layout files. The parser implements a CanParse and Parse method to handle applicability. Currently there's no overlap so we won't be double-parsing, but it's possible in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 87 out of 89 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- Analyzer/Properties/Resources.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
-- Find cache name for an addressables bundle | ||
SELECT cached_name | ||
FROM addr_build_cached_bundles | ||
WHERE catalog_name = 'packedassets7_assets_all_61d3358060e969d3aad2d9c5c3a7d69b.bundle'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing SQL comment block. Add '```' after line 104 to properly close the SQL code block.
WHERE catalog_name = 'packedassets7_assets_all_61d3358060e969d3aad2d9c5c3a7d69b.bundle'; | |
WHERE catalog_name = 'packedassets7_assets_all_61d3358060e969d3aad2d9c5c3a7d69b.bundle'; |
Copilot uses AI. Check for mistakes.
f4832e3
to
8303ae0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 87 out of 89 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- Analyzer/Properties/Resources.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
#### `addr_build_bundles` | ||
Bundle-level information including asset counts and file sizes. | ||
|
||
#### `addr_build_bundle_dependent_bundles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing backtick in table name reference.
#### `addr_build_bundle_dependent_bundles | |
#### `addr_build_bundle_dependent_bundles` |
Copilot uses AI. Check for mistakes.
{ | ||
protected override string TableName => "addr_build_sub_files"; | ||
|
||
protected override string DDLSource => Properties.Resources.AddrBuildSubFiles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent resource namespace usage. Other command classes use Properties.Resources
while handlers use Resources
. This should be Resources.AddrBuildSubFiles
to match the pattern established in the codebase.
protected override string DDLSource => Properties.Resources.AddrBuildSubFiles; | |
protected override string DDLSource => Resources.AddrBuildSubFiles; |
Copilot uses AI. Check for mistakes.
if (parser.CanParse(file)) | ||
{ | ||
foundParser = true; | ||
Console.Error.WriteLine(file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug output should be conditional on verbose mode or removed before production. This will clutter output for every processed file.
Console.Error.WriteLine(file); | |
if (m_Verbose) | |
Console.Error.WriteLine(file); |
Copilot uses AI. Check for mistakes.
private bool ProcessFile(string file, string path, SQLiteWriter writer, int i, int length) | ||
{ | ||
// Check whether a file is a Unity Archive (AssetBundle) by looking for known signatures at the start of the file. | ||
// "UnifyFS" is the current signature, but some older formats of the file are still supported | ||
string[] signatures = { "UnityFS", "UnityWeb", "UnityRaw", "UnityArchive" }; | ||
int maxLen = 12; // "UnityArchive".Length | ||
byte[] buffer = new byte[maxLen]; | ||
|
||
using (var fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read)) | ||
{ | ||
int read = fs.Read(buffer, 0, buffer.Length); | ||
foreach (var sig in signatures) | ||
{ | ||
if (read >= sig.Length) | ||
{ | ||
bool match = true; | ||
for (int i = 0; i < sig.Length; ++i) | ||
{ | ||
if (buffer[i] != sig[i]) | ||
{ | ||
match = false; | ||
break; | ||
} | ||
} | ||
if (match) | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
throw new NotImplementedException(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code should be removed. This method is no longer used after the refactoring to the parser architecture.
Copilot uses AI. Check for mistakes.
This PR adds support for reading Addressables build reports and adding them to the Analyzer sqlite database. This provides data that is only available at build time and can be useful when cross-referenced with built asset bundles.
It does a fairly major refactor of the code to add in a concept of a parser that can read different data types and then add their data to the database. SerializedFileParser encapsulates the old functionality and AddressablesBuildLayoutParser adds support for Addressables build reports.
You can point the analyzer at a mixture of build reports and asset bundles. An example of this sort of mixed directory: