Added Log and Fix None Materials #118
Conversation
|
""" WalkthroughThe changes introduce a centralized logging system for the glTF exporter, adding new utility classes for logging and configuration tracking. Logging is integrated into the export process, with log entries marking key events such as window opening, export start/finish, and configuration saving. The configuration directory path is now managed centrally via a static field. Material handling and color processing are refactored for clarity and schema awareness. Minor code formatting, initialization, and exception logging improvements are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant ExportLog
participant LogConfiguration
participant GlTFExportContext
User->>MainWindow: Open application
MainWindow->>ExportLog: StartLog()
MainWindow->>ExportLog: Write("Open Window")
User->>MainWindow: Initiate Export
MainWindow->>LogConfiguration: SaveConfig()
LogConfiguration->>ExportLog: Write(config key-value pairs)
MainWindow->>GlTFExportContext: Start()
GlTFExportContext->>ExportLog: Write("Export Started")
GlTFExportContext->>GlTFExportContext: (Export process with material reuse)
GlTFExportContext->>ExportLog: Write("Export Finished")
MainWindow->>ExportLog: EndLog()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
Common_glTF_Exporter/Utils/glTFExportUtils.cs (1)
68-68: Minor formatting change - consider consistency.Adding a blank line for readability is fine, but ensure this formatting style is consistent across the codebase.
Common_glTF_Exporter/Model/Links.cs (1)
15-15: Consider using a property instead of a public field.While centralizing the configuration directory path is excellent, consider making this a property instead of a public field for better encapsulation and potential future flexibility.
-public static string configDir = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Leia"); +public static string ConfigDir => Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Leia");Common_glTF_Exporter/Utils/LogConfiguration.cs (1)
9-38: Consider refactoring for better maintainability.The current implementation has repetitive code that could be made more maintainable and less error-prone by using a loop structure.
public static void SaveConfig() { - string format = SettingsConfig.GetValue("format"); - string flipAxis = SettingsConfig.GetValue("flipAxis"); - string normals = SettingsConfig.GetValue("normals"); - string relocateTo0 = SettingsConfig.GetValue("relocateTo0"); - string materials = SettingsConfig.GetValue("Materials"); - string units = SettingsConfig.GetValue("units"); - string batchId = SettingsConfig.GetValue("batchId"); - string levels = SettingsConfig.GetValue("levels"); - string properties = SettingsConfig.GetValue("properties"); - string grids = SettingsConfig.GetValue("grids"); - string compression = SettingsConfig.GetValue("compression"); - string release = SettingsConfig.GetValue("release"); - string runs = SettingsConfig.GetValue("runs"); - - ExportLog.Write($"format: {format}"); - ExportLog.Write($"flipAxis: {flipAxis}"); - ExportLog.Write($"normals: {normals}"); - ExportLog.Write($"relocateTo0: {relocateTo0}"); - ExportLog.Write($"materials: {materials}"); - ExportLog.Write($"units: {units}"); - ExportLog.Write($"batchId: {batchId}"); - ExportLog.Write($"levels: {levels}"); - ExportLog.Write($"properties: {properties}"); - ExportLog.Write($"grids: {grids}"); - ExportLog.Write($"compression: {compression}"); - ExportLog.Write($"release: {release}"); - ExportLog.Write($"runs: {runs}"); + var configKeys = new[] { + "format", "flipAxis", "normals", "relocateTo0", "materials", + "units", "batchId", "levels", "properties", "grids", + "compression", "release", "runs" + }; + + foreach (var key in configKeys) + { + string value = SettingsConfig.GetValue(key); + ExportLog.Write($"{key}: {value}"); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Common_glTF_Exporter/Common_glTF_Exporter.projitems(1 hunks)Common_glTF_Exporter/Core/GlTFExportContext.cs(9 hunks)Common_glTF_Exporter/Model/Links.cs(2 hunks)Common_glTF_Exporter/Utils/ExportLog.cs(1 hunks)Common_glTF_Exporter/Utils/LogConfiguration.cs(1 hunks)Common_glTF_Exporter/Utils/SettingsConfig.cs(1 hunks)Common_glTF_Exporter/Utils/glTFExportUtils.cs(1 hunks)Common_glTF_Exporter/Windows/MainWindow.xaml.cs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
Common_glTF_Exporter/Utils/SettingsConfig.cs (1)
Common_glTF_Exporter/Model/Links.cs (1)
Links(9-16)
Common_glTF_Exporter/Utils/ExportLog.cs (1)
Common_glTF_Exporter/Model/Links.cs (1)
Links(9-16)
Common_glTF_Exporter/Utils/LogConfiguration.cs (2)
Common_glTF_Exporter/Utils/SettingsConfig.cs (3)
SettingsConfig(9-132)SettingsConfig(19-28)GetValue(30-49)Common_glTF_Exporter/Utils/ExportLog.cs (2)
ExportLog(7-57)Write(27-38)
🔇 Additional comments (10)
Common_glTF_Exporter/Model/Links.cs (1)
3-3: Good addition for Path operations.The System.IO namespace import is necessary for the Path.Combine operation below.
Common_glTF_Exporter/Common_glTF_Exporter.projitems (1)
68-68: New logging utilities added to project structure.The addition of ExportLog.cs and LogConfiguration.cs to the project compilation list is appropriate and follows the existing structure.
Note: The actual implementation quality of these logging utilities would need to be reviewed separately when those files are provided.
Also applies to: 70-70
Common_glTF_Exporter/Utils/SettingsConfig.cs (2)
15-15: Good use of centralized configuration directory.Using
Links.configDirinstead of the previous_configDirfield eliminates code duplication and centralizes path management.
22-23: Consistent use of centralized configuration directory.The directory existence check and creation now uses the centralized
Links.configDir, maintaining consistency with the updated file path.Common_glTF_Exporter/Windows/MainWindow.xaml.cs (3)
52-53: Good integration of logging initialization.Starting the export log and logging the window opening provides good traceability for the export session.
65-67: Configuration logging before export is well-placed.Logging the configuration before starting the export process ensures the settings are captured for debugging purposes.
86-86: Appropriate logging for export validation.Logging when no valid elements are found helps with troubleshooting export issues.
Common_glTF_Exporter/Core/GlTFExportContext.cs (3)
90-90: Good logging integration for export lifecycle tracking.The logging calls at key export lifecycle points (Start, Finish, LinkBegin, LinkEnd) provide good visibility into the export process flow.
Also applies to: 142-142, 385-385, 399-399
115-116: Good initialization of default material.The creation and addition of a default GLTF material during export start ensures there's always a fallback material available.
428-432: Good conditional material handling in OnRPC.The conditional assignment and addition of
currentMaterialbased on material preferences aligns well with the material handling logic used elsewhere in the class.
| public static void EndLog() | ||
| { | ||
| File.AppendAllText(logFilePath, $"[END] Export ended at {DateTime.Now}\n"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add directory existence check for consistency.
The EndLog method should ensure the directory exists before writing, similar to StartLog.
public static void EndLog()
{
+ if (!Directory.Exists(Links.configDir))
+ {
+ Directory.CreateDirectory(Links.configDir);
+ }
File.AppendAllText(logFilePath, $"[END] Export ended at {DateTime.Now}\n");
}🤖 Prompt for AI Agents
In Common_glTF_Exporter/Utils/ExportLog.cs around lines 22 to 25, the EndLog
method writes to a file without checking if the directory exists, unlike
StartLog. Add a check to verify the directory of logFilePath exists before
appending text, and create it if it does not, to maintain consistency and
prevent errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
16-16: Remove unused import.The
System.Diagnosticsimport is not used in this file and should be removed to keep the code clean.-using System.Diagnostics;
51-51: Remove unnecessary empty line.The empty line serves no purpose and should be removed for cleaner code formatting.
-Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
3-4: Remove unused imports.The imports
System.IO.PortsandSystem.Xml.Linqare not used in this file and should be removed to keep the code clean.-using System.IO.Ports; -using System.Xml.Linq;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Common_glTF_Exporter/Core/GlTFExportContext.cs(8 hunks)Common_glTF_Exporter/ExternalCommand.cs(1 hunks)Common_glTF_Exporter/Materials/AssetProperties.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialProperties.cs(2 hunks)Common_glTF_Exporter/Materials/MaterialTextures.cs(3 hunks)Common_glTF_Exporter/Materials/RevitMaterials.cs(2 hunks)Common_glTF_Exporter/Utils/ExportLog.cs(1 hunks)Common_glTF_Exporter/Windows/MainWindow.xaml.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Common_glTF_Exporter/Windows/MainWindow.xaml.cs
- Common_glTF_Exporter/Core/GlTFExportContext.cs
- Common_glTF_Exporter/Utils/ExportLog.cs
🧰 Additional context used
🧬 Code Graph Analysis (3)
Common_glTF_Exporter/ExternalCommand.cs (1)
Common_glTF_Exporter/Utils/ExportLog.cs (2)
ExportLog(7-55)WriteException(38-49)
Common_glTF_Exporter/Materials/MaterialTextures.cs (1)
Common_glTF_Exporter/Materials/AssetProperties.cs (4)
Asset(24-39)AssetPropertiesUtils(10-207)GetTexturePath(81-112)GetFade(196-206)
Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (1)
SrgbToLinear(129-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
Common_glTF_Exporter/ExternalCommand.cs (1)
42-42: LGTM! Exception logging properly integrated.The addition of
ExportLog.WriteException(ex)correctly integrates with the new logging system, maintaining proper error handling flow while providing detailed exception information for debugging.Common_glTF_Exporter/Materials/MaterialTextures.cs (4)
40-44: Good defensive programming for schema validation.The early return when
BaseSchemais null prevents processing materials without valid schema information, which aligns well with the new schema-aware approach.
46-47: Schema-aware texture retrieval properly implemented.The extraction of
schemaNameand its usage inGetDiffuseBitmapcorrectly implements the new schema-aware approach for material processing.
56-59: Excellent fallback logic for missing textures.The fallback to
GetAppearanceColorwhen textures are unavailable provides graceful degradation and maintains consistency with the schema-aware approach.
75-75: Clean separation of concerns.Removing the
BaseColorassignment fromSetTexturePropertiesand handling it in the main method's fallback logic provides better separation of concerns.Common_glTF_Exporter/Materials/RevitMaterials.cs (3)
31-31: Simplified material retrieval improves maintainability.Direct retrieval of materials from the document eliminates caching complexity and ensures current material state is always used.
42-44: Good separation of PBR property initialization.The explicit creation of
GLTFPBRobject and delegation toMaterialProperties.SetPropertiesprovides clear separation of concerns.
45-50: Proper conditional texture processing.The conditional check for both material existence and user preferences ensures textures are only processed when appropriate, improving performance and respecting user settings.
Common_glTF_Exporter/Materials/AssetProperties.cs (3)
12-22: Excellent schema-based property mapping.The
DiffusePropertyMapprovides a clean, maintainable approach to map material schemas to their corresponding diffuse properties using proper Revit API constants.
24-39: Robust implementation with proper error handling.The
GetDiffuseBitmapmethod includes comprehensive null checks, dictionary lookup validation, and connected property verification, making it resilient to various edge cases.
41-79: Consistent schema-based color retrieval with proper conversion.The
ColorPropertyMapandGetAppearanceColormethod follow the same robust pattern as the diffuse bitmap handling, with proper color value conversion from doubles to bytes.Common_glTF_Exporter/Materials/MaterialProperties.cs (7)
29-50: Excellent refactoring improves code clarity.The refactored color processing logic with helper methods (
RgbToUnit,BlendColour,GetLinearColour) significantly improves readability and maintainability while preserving the original functionality.
60-63: Simple and effective default color implementation.The
GetDefaultColourmethod provides a clean white default with proper opacity handling for textured materials.
65-73: Correct multiplicative color blending implementation.The
BlendColourmethod properly implements multiplicative blending for tint operations with clean tuple-based syntax.
75-82: Proper sRGB to linear color space conversion.The
GetLinearColourmethod correctly converts sRGB colors to linear space, which is essential for accurate glTF color representation.
84-92: Standard RGB byte to unit float conversion.The
RgbToUnitmethod correctly normalizes RGB byte values to the 0-1 float range with proper tuple-based return values.
94-99: Standard sRGB to linear gamma correction implementation.The
SrgbToLinearmethod correctly implements the standard gamma correction formula for sRGB to linear color space conversion, consistent with the existing codebase patterns.
54-54: Appropriate neutral base color for textured materials.Using
GetDefaultColourfor textured materials provides the correct neutral white base that won't interfere with texture appearance.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes