Conversation
WalkthroughThis pull request performs a comprehensive refactoring of the glTF exporter codebase, replacing custom GLTF data types with a standardized external Changes
Sequence Diagram(s)sequenceDiagram
participant GlTFExportContext
participant ProcessGeometry
participant GLTFBinaryDataUtils
participant glTF.Manipulator
GlTFExportContext->>GlTFExportContext: OnElementBegin (Element)
GlTFExportContext->>GlTFExportContext: OnMaterial, OnPolymesh
GlTFExportContext->>GlTFExportContext: Collect geometry data
GlTFExportContext->>GlTFExportContext: OnElementEnd (Element)
Note over GlTFExportContext: Delegate to ProcessGeometry
GlTFExportContext->>ProcessGeometry: ProcessNode(nodes, rootNode, meshes, ...)
ProcessGeometry->>glTF.Manipulator: Create Node with extras
ProcessGeometry->>glTF.Manipulator: Create Mesh
loop For each GeometryDataObject
ProcessGeometry->>GLTFBinaryDataUtils: ExportVertices(geomData, globalBuffer, ...)
GLTFBinaryDataUtils->>glTF.Manipulator: Create Accessor, BufferView
GLTFBinaryDataUtils->>ProcessGeometry: Return accessor index
ProcessGeometry->>GLTFBinaryDataUtils: ExportNormals, ExportUVs, ExportBatchId, ExportFaces
GLTFBinaryDataUtils->>ProcessGeometry: Return accessor indices
ProcessGeometry->>glTF.Manipulator: Assign to MeshPrimitive attributes/indices
ProcessGeometry->>ProcessGeometry: Add primitive to mesh
end
ProcessGeometry->>ProcessGeometry: Clear geometry data, return
GlTFExportContext->>GlTFExportContext: Continue with next element
sequenceDiagram
participant FileExport
participant GltfJson
participant System.Text.Json as JSON
FileExport->>GltfJson: Get(scenes, nodes, meshes, materials, ...)
GltfJson->>GltfJson: Create glTF root model with assets
Note over GltfJson: Use System.Text.Json instead of Newtonsoft.Json
alt Materials & Textures enabled
GltfJson->>GltfJson: transformMaterials(BaseMaterial[])
Note over GltfJson: Convert to glTF.Material with PBR
GltfJson->>GltfJson: cleanImages(glTFImage[])
Note over GltfJson: Filter/retain specific fields
end
GltfJson->>JSON: JsonSerializer.Serialize with options
Note over JSON: NullValueHandling via IgnoreNullValues, no naming policy
JSON-->>GltfJson: JSON string
GltfJson-->>FileExport: JSON string
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Common_glTF_Exporter/Version/LatestVersion.cs (2)
15-47: Resource leak:HttpClientnot disposed on exception paths.If an exception occurs between lines 22-44,
client.Dispose()at line 47 is never called. Consider usingusingstatements for bothHttpClientandHttpContent:- HttpClient client = new HttpClient(); - client.BaseAddress = new Uri("https://APIAutoUpdate"); - - string version = SettingsConfig.currentVersion; - string urlParameters = "?inputVersion=" + version + - "&&folderName=" + "e-verse/LeiaGltfExporter"; - - try { - HttpResponseMessage result = client.GetAsync(urlParameters, HttpCompletionOption.ResponseHeadersRead).Result; - - if (result.IsSuccessStatusCode) - { - - HttpContent content = result.Content; - string myContent = await content.ReadAsStringAsync(); + using (HttpClient client = new HttpClient()) + { + client.BaseAddress = new Uri("https://APIAutoUpdate"); + + string version = SettingsConfig.currentVersion; + string urlParameters = "?inputVersion=" + version + + "&&folderName=" + "e-verse/LeiaGltfExporter"; + + try + { + using (HttpResponseMessage result = await client.GetAsync(urlParameters, HttpCompletionOption.ResponseHeadersRead)) + { + if (result.IsSuccessStatusCode) + { + string myContent = await result.Content.ReadAsStringAsync();
23-23: Avoid.Resulton async calls—potential deadlock.Calling
.Resulton an async operation inside anasyncmethod can cause deadlocks in certain synchronization contexts. Useawaitinstead:- HttpResponseMessage result = client.GetAsync(urlParameters, HttpCompletionOption.ResponseHeadersRead).Result; + HttpResponseMessage result = await client.GetAsync(urlParameters, HttpCompletionOption.ResponseHeadersRead);Revit_glTF_Exporter_2020/Revit_glTF_Exporter_2020.csproj (1)
148-148: Fix typo in PostBuildEvent that will break debug deployment.Line 148 contains
$(AAPPDATA)instead of$(APPDATA). This typo will cause the copy command to fail during debug builds, preventing the DLL from being deployed to the correct location.Apply this diff to fix the typo:
- copy "$(ProjectDir)$(OutputPath)*.dll" "$(AAPPDATA)\Autodesk\ApplicationPlugins\leia.bundle\Contents\2020\" + copy "$(ProjectDir)$(OutputPath)*.dll" "$(APPDATA)\Autodesk\ApplicationPlugins\leia.bundle\Contents\2020\"
♻️ Duplicate comments (6)
Revit_glTF_Exporter_2022/Revit_glTF_Exporter_2022.csproj (1)
76-78: LGTM - Consistent dependency migration.The addition of glTF.Manipulator aligns with the broader refactoring effort. The package version verification requested for the 2024 variant applies here as well.
Revit_glTF_Exporter_2021/Revit_glTF_Exporter_2021.csproj (1)
76-78: LGTM - Consistent dependency migration.The package reference update is consistent with other Revit year variants in this PR.
Revit_glTF_Exporter_2026/Revit_glTF_Exporter_2026.csproj (1)
78-78: LGTM - Consistent dependency migration.The glTF.Manipulator package addition is consistent with the broader refactoring effort across all Revit year variants.
Revit_glTF_Exporter_2020/Revit_glTF_Exporter_2020.csproj (1)
126-128: LGTM - Consistent dependency migration.The glTF.Manipulator package addition aligns with the project-wide refactoring effort.
Revit_glTF_Exporter_2023/Revit_glTF_Exporter_2023.csproj (1)
78-80: LGTM - Consistent dependency migration.The glTF.Manipulator package addition is consistent with the project-wide refactoring effort.
Revit_glTF_Exporter_2019/Revit_glTF_Exporter_2019.csproj (1)
76-78: Verify the glTF.Manipulator package version.Same verification as the 2025 project is needed here.
Please refer to the verification script in
Revit_glTF_Exporter_2025/Revit_glTF_Exporter_2025.csproj.
🧹 Nitpick comments (23)
Common_glTF_Exporter/Core/IndexedDictionary.cs (1)
163-179: Consider adding XML documentation for the newClonemethod.The method lacks the XML documentation that other public methods in this class have. Also, note that
CurrentKeyin the cloned dictionary will be set to an arbitrary key (the last one iterated), which may differ from the original'sCurrentKey.+ /// <summary> + /// Creates a deep clone of this IndexedDictionary using the provided clone function. + /// </summary> + /// <param name="cloneFunc">Function to clone each element.</param> + /// <returns>A new IndexedDictionary with cloned elements.</returns> public IndexedDictionary<T> Clone(Func<T, T> cloneFunc) {Common_glTF_Exporter/EportUtils/ElementValidations.cs (1)
11-14: Potential unused imports.
System.TextandSystem.Linqdon't appear to be used in this file. Consider removing them if static analysis confirms they're unnecessary.Common_glTF_Exporter/Core/ProcessGeometry.cs (2)
11-13: Remove unused imports.
System.Text,System.Windows.Forms, andSystem.Xml.Linqappear unused in this file.using System; using System.Collections.Generic; using System.Linq; -using System.Text; -using System.Windows.Forms; -using System.Xml.Linq;
19-21: Consider introducing a parameter object for this method.The method signature has 10 parameters, which impacts readability and maintainability. A context object or builder pattern could simplify this.
Common_glTF_Exporter/Windows/ProgressBarWindow.xaml.cs (1)
34-39: Remove redundant local variable.The
maxValueDuplicatedvariable is an unnecessary copy of themaxValueparameter with no transformations applied. This reduces code clarity without providing any benefit.Apply this diff to simplify the code:
- double maxValueDuplicated = maxValue; - ProgressBarWindow.ViewModel.ProgressBarGraphicValue = maxValueDuplicated * 0.07; + ProgressBarWindow.ViewModel.ProgressBarGraphicValue = maxValue * 0.07; ProgressBarWindow.ViewModel.ProgressBarValue = currentValue; ProgressBarWindow.ViewModel.Message = message; ProgressBarWindow.ViewModel.Action = "Cancel"; - ProgressBarWindow.ViewModel.ProgressBarMax = maxValueDuplicated; + ProgressBarWindow.ViewModel.ProgressBarMax = maxValue;Common_glTF_Exporter/ViewModel/ProgressBarWindowViewModel.cs (1)
64-72: Eliminate duplicate percentage calculation.The percentage is calculated twice: once on line 64 and again on line 71. The
elsebranch should reuse thepercentageProgressvariable.Apply this diff:
double percentageProgress = (value / ProgressBarMax) * 100; if (percentageProgress > 99) { ProgressBarPercentage = 100; } else { - ProgressBarPercentage = (value / ProgressBarMax) * 100; + ProgressBarPercentage = percentageProgress; }Common_glTF_Exporter/Export/Draco.cs (3)
374-381: Empty catch blocks silently swallow exceptions.While ignoring file deletion failures during cleanup may be intentional, completely empty catch blocks make debugging difficult if something unexpected happens.
Consider logging or at minimum adding a comment explaining why failures are ignored:
try { if (File.Exists(path)) File.Delete(path); } - catch + catch (IOException) { + // File may be locked; safe to ignore during cleanup }
564-574: Same empty catch pattern for file deletion.Consistent with the previous comment, this catch block should at minimum specify the exception type being ignored.
try { if (File.Exists(x)) File.Delete(x); } - catch + catch (IOException) { - // ignore + // File locked or in use; safe to ignore during temp cleanup }
605-607: Same pattern for File.Delete exception handling.- try { File.Delete(dst); } - catch { /* ignore */ } + try { File.Delete(dst); } + catch (IOException) { /* File locked; will fail on Move anyway */ }Common_glTF_Exporter/Export/BinFile.cs (1)
1-7: Unused using directives.
System.Collections.Generic,System.Text, andCommon_glTF_Exporter.Coreappear unused in this file.using System; -using System.Collections.Generic; using System.IO; -using System.Text; -using Common_glTF_Exporter.Core; -using Common_glTF_Exporter.Windows.MainWindow; using glTF.Manipulator.GenericSchema;Common_glTF_Exporter/Export/RevitGrids.cs (1)
32-36: Existing TODO: Arc grid support is not implemented.The code only handles
Linecurves and silently skips arc-based grids. This is pre-existing behavior.Would you like me to open an issue to track arc grid support, or help implement it?
Common_glTF_Exporter/Materials/MaterialTextures.cs (3)
9-12: Unused using directive.
System.Windows.Interopdoes not appear to be used in this file.using System.Drawing.Imaging; using System.IO; using System.Linq; -using System.Windows.Interop; using Material = Autodesk.Revit.DB.Material;
46-48: Inconsistent variable naming convention.
Fadevalue,TintColour, andBaseColoruse PascalCase, but local variables in C# conventionally use camelCase.- Autodesk.Revit.DB.Color tintColour = AssetPropertiesUtils.GetTint(theAsset); - Autodesk.Revit.DB.Color baseColor = AssetPropertiesUtils.GetAppearanceColor(theAsset, schemaName); - double Fadevalue = AssetPropertiesUtils.GetFade(theAsset); + Autodesk.Revit.DB.Color tintColor = AssetPropertiesUtils.GetTint(theAsset); + Autodesk.Revit.DB.Color baseColor = AssetPropertiesUtils.GetAppearanceColor(theAsset, schemaName); + double fadeValue = AssetPropertiesUtils.GetFade(theAsset);
94-100: Redundant list traversal -Any()followed byFindIndex().The list is scanned twice: once by
Any()and again byFindIndex(). UseFindIndex()alone and check if result is-1.- bool checkIfImageExists = images.Any(x => x.uuid == texturePath); - - if (checkIfImageExists) - { - int index = images.FindIndex(x => x.uuid == texturePath); - - return index; - } + int existingIndex = images.FindIndex(x => x.uuid == texturePath); + if (existingIndex >= 0) + { + return existingIndex; + }Common_glTF_Exporter/Materials/MaterialProperties.cs (3)
3-4: Unused imports detected.
System.IO.Ports(serial port communication) andSystem.Linqappear to be unused in this file. Consider removing them to keep imports clean.-using System.IO.Ports; -using System.Linq;
18-24: Unused parameternodeinSetProperties.The
MaterialNode nodeparameter is never used within the method body. If this is intentional for API consistency, consider documenting it or using a discard pattern.-public static void SetProperties(MaterialNode node, float opacity, ref BaseMaterial material) +public static void SetProperties(float opacity, ref BaseMaterial material)Alternatively, if the parameter must remain for interface consistency, suppress the warning or document the intent.
26-52: Inconsistent indentation in method body.The method body has extra indentation that doesn't match the method declaration. This appears to be a leftover from the refactor.
public static List<float> SetMaterialColour(MaterialNode node, float opacity, Autodesk.Revit.DB.Color baseColor, Autodesk.Revit.DB.Color tintColor) { - - (float, float, float) baseColours; + (float, float, float) baseColours; - if (baseColor == null) - { - baseColours = RgbToUnit(node.Color); - } - else - { - baseColours = RgbToUnit(baseColor); - } + if (baseColor == null) + { + baseColours = RgbToUnit(node.Color); + } + else + { + baseColours = RgbToUnit(baseColor); + } - if (tintColor == null) - { - return GetLinearColour(baseColours, opacity); - } - else - { - (float, float, float) baseTintColour = RgbToUnit(tintColor); - (float, float, float) blendColour = BlendColour(baseColours, baseTintColour); + if (tintColor == null) + { + return GetLinearColour(baseColours, opacity); + } + else + { + (float, float, float) baseTintColour = RgbToUnit(tintColor); + (float, float, float) blendColour = BlendColour(baseColours, baseTintColour); - return GetLinearColour(blendColour, opacity); - } + return GetLinearColour(blendColour, opacity); + } }Common_glTF_Exporter/Export/GltfJson.cs (2)
86-101: Method naming doesn't follow C# conventions.
cleanImagesshould beCleanImagesto follow C# PascalCase naming conventions for public methods.-public static List<glTFImage> cleanImages(List<glTFImage> images) +public static List<glTFImage> CleanImages(List<glTFImage> images)Also update the call site on line 65.
103-142: Method naming and minor formatting.
transformMaterialsshould beTransformMaterialsto follow C# PascalCase naming conventions. Additionally, there are extra blank lines (e.g., line 114) that could be cleaned up.-public static List<glTF.Manipulator.Schema.Material> transformMaterials(List<BaseMaterial> baseMaterials) +public static List<glTF.Manipulator.Schema.Material> TransformMaterials(List<BaseMaterial> baseMaterials)The transformation logic itself is sound—properly mapping BaseMaterial properties to the glTF schema Material type with PBR and texture extension support.
Common_glTF_Exporter/Materials/RevitMaterials.cs (2)
10-10: Unused import.
System.IO.Portsis unrelated to material/glTF processing and appears unused.-using System.IO.Ports;
72-85: RedundantbaseColorFactorassignment when texture is present.When
hasTextureis true,baseColorFactoris set to the default color at line 75 (inside the texture block) and again at line 80 (inside thehasTexturecheck). The assignment on line 75 is redundant since it will be overwritten by line 80.if (revitMaterial != null && preferences.materials == MaterialsEnum.textures) { baseNTintColour = MaterialTextures.SetMaterialTextures(revitMaterial, material, doc, opacity, textures, images); - material.baseColorFactor = MaterialProperties.GetDefaultColour(opacity); } if (material.hasTexture) { material.baseColorFactor = MaterialProperties.GetDefaultColour(opacity); } else { material.baseColorFactor = MaterialProperties.SetMaterialColour(node, opacity, baseNTintColour.Item1, baseNTintColour.Item2); }Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (2)
65-67: Remove unused class-level constants.The class-level constants
VEC3_STR,POSITION_STR(lines 66-67) andNORMAL_STR(line 132) are shadowed by local declarations inside the methods and are never used. Additionally, line 132 uses"NORMALS"(plural) which would be inconsistent with the glTF spec if it were used.- - - const string VEC3_STR = "VEC3"; - const string POSITION_STR = "POSITION"; - public static int ExportVertices(- - - const string NORMAL_STR = "NORMALS"; - public static int ExportNormals(Also applies to: 132-132
153-192: Consider translating Spanish comments to English for consistency.The codebase appears to use English for comments elsewhere. These Spanish comments ("SOLO para estas normales", "Append normals al buffer global", etc.) should be translated for consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
Common_glTF_Exporter/Common_glTF_Exporter.projitems(1 hunks)Common_glTF_Exporter/Core/GLTFAccessor.cs(0 hunks)Common_glTF_Exporter/Core/GLTFAttribute.cs(0 hunks)Common_glTF_Exporter/Core/GLTFBinaryData.cs(0 hunks)Common_glTF_Exporter/Core/GLTFBuffer.cs(0 hunks)Common_glTF_Exporter/Core/GLTFBufferView.cs(0 hunks)Common_glTF_Exporter/Core/GLTFExtras.cs(0 hunks)Common_glTF_Exporter/Core/GLTFImage.cs(0 hunks)Common_glTF_Exporter/Core/GLTFMaterial.cs(0 hunks)Common_glTF_Exporter/Core/GLTFMesh.cs(0 hunks)Common_glTF_Exporter/Core/GLTFMeshPrimitive.cs(0 hunks)Common_glTF_Exporter/Core/GLTFNode.cs(0 hunks)Common_glTF_Exporter/Core/GLTFScene.cs(0 hunks)Common_glTF_Exporter/Core/GLTFTextureInfo.cs(0 hunks)Common_glTF_Exporter/Core/GLTFVersion.cs(0 hunks)Common_glTF_Exporter/Core/GlTFExportContext.cs(10 hunks)Common_glTF_Exporter/Core/IndexedDictionary.cs(2 hunks)Common_glTF_Exporter/Core/ProcessGeometry.cs(1 hunks)Common_glTF_Exporter/Core/glTF.cs(0 hunks)Common_glTF_Exporter/Core/glTFPBR.cs(0 hunks)Common_glTF_Exporter/EportUtils/ElementValidations.cs(1 hunks)Common_glTF_Exporter/EportUtils/GLTFNodeActions.cs(0 hunks)Common_glTF_Exporter/Export/BinFile.cs(1 hunks)Common_glTF_Exporter/Export/BufferConfig.cs(2 hunks)Common_glTF_Exporter/Export/Draco.cs(13 hunks)Common_glTF_Exporter/Export/FileExport.cs(2 hunks)Common_glTF_Exporter/Export/GlbBinInfo.cs(1 hunks)Common_glTF_Exporter/Export/GlbFile.cs(1 hunks)Common_glTF_Exporter/Export/GltfJson.cs(3 hunks)Common_glTF_Exporter/Export/RevitGrids.cs(3 hunks)Common_glTF_Exporter/Materials/MaterialProperties.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialTexture.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialTextures.cs(1 hunks)Common_glTF_Exporter/Materials/RevitMaterials.cs(2 hunks)Common_glTF_Exporter/Model/GeometryDataObject.cs(0 hunks)Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs(1 hunks)Common_glTF_Exporter/Utils/MaterialUtils.cs(1 hunks)Common_glTF_Exporter/Utils/glTFExportUtils.cs(2 hunks)Common_glTF_Exporter/Version/LatestVersion.cs(2 hunks)Common_glTF_Exporter/ViewModel/ProgressBarWindowViewModel.cs(1 hunks)Common_glTF_Exporter/Windows/MainWindow.xaml.cs(2 hunks)Common_glTF_Exporter/Windows/ProgressBarWindow.xaml.cs(1 hunks)GltfInstaller/Program.cs(8 hunks)Revit_glTF_Exporter_2019/Revit_glTF_Exporter_2019.csproj(1 hunks)Revit_glTF_Exporter_2020/Revit_glTF_Exporter_2020.csproj(1 hunks)Revit_glTF_Exporter_2021/Revit_glTF_Exporter_2021.csproj(1 hunks)Revit_glTF_Exporter_2022/Revit_glTF_Exporter_2022.csproj(1 hunks)Revit_glTF_Exporter_2023/Revit_glTF_Exporter_2023.csproj(1 hunks)Revit_glTF_Exporter_2024/Revit_glTF_Exporter_2024.csproj(1 hunks)Revit_glTF_Exporter_2025/Revit_glTF_Exporter_2025.csproj(1 hunks)Revit_glTF_Exporter_2026/Revit_glTF_Exporter_2026.csproj(1 hunks)
💤 Files with no reviewable changes (18)
- Common_glTF_Exporter/Core/GLTFMesh.cs
- Common_glTF_Exporter/Core/GLTFImage.cs
- Common_glTF_Exporter/Core/GLTFNode.cs
- Common_glTF_Exporter/Core/GLTFVersion.cs
- Common_glTF_Exporter/Core/GLTFBuffer.cs
- Common_glTF_Exporter/Core/GLTFAttribute.cs
- Common_glTF_Exporter/Model/GeometryDataObject.cs
- Common_glTF_Exporter/Core/GLTFBufferView.cs
- Common_glTF_Exporter/Core/GLTFAccessor.cs
- Common_glTF_Exporter/Core/GLTFExtras.cs
- Common_glTF_Exporter/Core/GLTFBinaryData.cs
- Common_glTF_Exporter/EportUtils/GLTFNodeActions.cs
- Common_glTF_Exporter/Core/GLTFScene.cs
- Common_glTF_Exporter/Core/GLTFMeshPrimitive.cs
- Common_glTF_Exporter/Core/GLTFMaterial.cs
- Common_glTF_Exporter/Core/GLTFTextureInfo.cs
- Common_glTF_Exporter/Core/glTFPBR.cs
- Common_glTF_Exporter/Core/glTF.cs
🧰 Additional context used
🧬 Code graph analysis (14)
Common_glTF_Exporter/Version/LatestVersion.cs (1)
Common_glTF_Exporter/Model/Payload.cs (1)
Payload(9-18)
Common_glTF_Exporter/Windows/MainWindow.xaml.cs (4)
Common_glTF_Exporter/Utils/ExportLog.cs (2)
ExportLog(7-56)Write(26-37)Common_glTF_Exporter/Windows/ProgressBarWindow.xaml.cs (3)
ProgressBarWindow(13-87)ProgressBarWindow(15-21)ProgressBarWindow(29-44)Common_glTF_Exporter/Export/BinFile.cs (1)
Create(14-25)Common_glTF_Exporter/Export/GlbFile.cs (1)
Create(15-26)
Common_glTF_Exporter/EportUtils/ElementValidations.cs (1)
Common_glTF_Exporter/Core/IndexedDictionary.cs (2)
IndexedDictionary(12-180)IndexedDictionary(163-179)
Common_glTF_Exporter/Core/IndexedDictionary.cs (1)
Common_glTF_Exporter/Export/GltfJson.cs (2)
List(86-101)List(103-142)
Common_glTF_Exporter/Materials/MaterialTexture.cs (2)
Common_glTF_Exporter/Utils/MaterialUtils.cs (1)
Autodesk(14-26)Common_glTF_Exporter/Materials/MaterialTextures.cs (1)
Autodesk(20-62)
Common_glTF_Exporter/Export/RevitGrids.cs (2)
Common_glTF_Exporter/Core/IndexedDictionary.cs (2)
IndexedDictionary(12-180)IndexedDictionary(163-179)Common_glTF_Exporter/Utils/Util.cs (2)
Dictionary(254-312)Util(10-344)
Common_glTF_Exporter/Export/FileExport.cs (3)
Common_glTF_Exporter/Export/GltfJson.cs (2)
List(86-101)List(103-142)Common_glTF_Exporter/Core/IndexedDictionary.cs (2)
IndexedDictionary(12-180)IndexedDictionary(163-179)Common_glTF_Exporter/Export/BinFile.cs (2)
BinFile(9-26)Create(14-25)
Common_glTF_Exporter/Materials/MaterialProperties.cs (2)
Common_glTF_Exporter/Materials/MaterialTextures.cs (1)
Autodesk(20-62)Common_glTF_Exporter/Utils/glTFExportUtils.cs (2)
BaseMaterial(20-23)BaseMaterial(25-37)
Common_glTF_Exporter/Export/BinFile.cs (1)
Common_glTF_Exporter/Export/GlbFile.cs (1)
Create(15-26)
Common_glTF_Exporter/Export/BufferConfig.cs (1)
Common_glTF_Exporter/Export/FileExport.cs (1)
Run(21-56)
Common_glTF_Exporter/Export/GltfJson.cs (1)
Common_glTF_Exporter/Utils/glTFExportUtils.cs (2)
BaseMaterial(20-23)BaseMaterial(25-37)
Common_glTF_Exporter/Utils/MaterialUtils.cs (2)
Common_glTF_Exporter/Materials/RevitMaterials.cs (2)
BaseMaterial(25-53)BaseMaterial(59-88)Common_glTF_Exporter/Utils/glTFExportUtils.cs (3)
BaseMaterial(20-23)BaseMaterial(25-37)GLTFExportUtils(14-148)
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
Common_glTF_Exporter/Utils/Util.cs (5)
List(219-241)Util(10-344)GetScalarMinMax(98-102)GetVec3MinMax(89-96)GetVec2MinMax(314-331)
Common_glTF_Exporter/Core/GlTFExportContext.cs (2)
Common_glTF_Exporter/Utils/glTFExportUtils.cs (3)
BaseMaterial(20-23)BaseMaterial(25-37)GLTFExportUtils(14-148)Common_glTF_Exporter/Core/IndexedDictionary.cs (3)
IndexedDictionary(12-180)IndexedDictionary(163-179)AddOrUpdateCurrentMaterial(92-111)
⏰ 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 (37)
Common_glTF_Exporter/Common_glTF_Exporter.projitems (1)
12-13: Files verified; naming pattern is intentional and clear—no issues foundBoth
Core\ProcessGeometry.csandMaterials\MaterialTexture.csexist and are correctly wired in the projitems. The naming distinction betweenMaterialTexture(instance class) andMaterialTextures(static utility class) is intentional and follows a standard pattern—not confusing. The projitems changes are valid and ready.Common_glTF_Exporter/Version/LatestVersion.cs (1)
31-36: LGTM — case-insensitive deserialization aligns with thePayloadmodel.The
PropertyNameCaseInsensitive = trueoption correctly handles JSON properties that may differ in casing from the C#Payloadclass properties.Common_glTF_Exporter/Core/IndexedDictionary.cs (1)
104-108: LGTM — type check updated for the new schema.The type check correctly migrated from
GLTFMaterialtoglTF.Manipulator.Schema.Material, and the logic to updatedoubleSidedon the existing material remains correct.Common_glTF_Exporter/EportUtils/ElementValidations.cs (1)
21-22: LGTM — parameter type updated to match the new schema.The signature change from
IndexedDictionary<GLTFNode>toIndexedDictionary<Node>correctly aligns with the broader migration toglTF.Manipulator.Schematypes.Common_glTF_Exporter/Core/ProcessGeometry.cs (1)
24-41: Emptyextrasobject attached when properties are disabled.When
preferences.propertiesis false, an emptyExtrasobject is still assigned to the node. Verify this is intentional per the glTF spec—settingextrastonullmight be preferred if no extra data exists.Common_glTF_Exporter/Core/GlTFExportContext.cs (4)
53-64: LGTM — type declarations correctly migrated to new schema.All collection types (
nodes,materials,textures,images,scenes,meshes,buffers,bufferViews,accessors) are correctly updated to useglTF.Manipulator.Schematypes.
214-218: LGTM — geometry processing correctly delegated.The extraction of mesh/primitive creation logic to
ProcessGeometry.ProcessNodeis a clean refactor that improves separation of concerns.
273-281: LGTM — UV handling correctly uses the newGltfUVtype.The explicit
GltfUVconstruction withUandVproperties, including the V-flip transformation, aligns with the new schema.
132-146: LGTM — buffer finalization logic is correct.The buffer byte length is properly retrieved from
globalBuffer.GetCurrentByteOffset()before adding to the buffers list.Revit_glTF_Exporter_2023/Revit_glTF_Exporter_2023.csproj (1)
108-110: LGTM - Runtime identifier specification.The addition of RuntimeIdentifiers is appropriate for specifying platform-specific build targets.
GltfInstaller/Program.cs (1)
36-37: LGTM - Installer updated to package the new dependency.The installer correctly packages
glTF.Manipulator.dllinstead ofNewtonsoft.Json.dllacross all Revit year variants (2019-2026), consistent with the project file dependency changes.Also applies to: 47-48, 58-59, 69-70, 80-81, 91-92, 102-103, 115-116
Revit_glTF_Exporter_2024/Revit_glTF_Exporter_2024.csproj (1)
69-71: The package version 2025.11.24.2122 is available on NuGet with no known security vulnerabilities.Verification confirms:
- ✓ Version 2025.11.24.2122 exists on NuGet and is the latest release
- ✓ Published 2025-11-24T21:25:51 UTC (legitimate recent timestamp)
- ✓ Package is listed and properly distributed
- ✓ No security advisories found for glTF.Manipulator
- ✓ Author is e-verse (matches repository context)
- ✓ MIT license with minimal dependencies (System.Text.Json)
The version is stable for use.
Common_glTF_Exporter/Export/GlbFile.cs (1)
15-19: LGTM!The signature change from
List<GLTFBinaryData>to a singleGLTFBinaryDataparameter aligns with the broader refactoring to streamline binary data handling. The implementation correctly passes the single buffer toGlbBinInfo.Get.Common_glTF_Exporter/Export/GlbBinInfo.cs (1)
9-41: LGTM!The refactored implementation correctly constructs a GLB binary chunk from a single
GLTFBinaryDatabuffer. The chunk structure follows the GLB specification with proper length prefix, chunk type, and binary data assembly using efficientBuffer.BlockCopyoperations.Common_glTF_Exporter/Export/BufferConfig.cs (1)
5-6: LGTM!The type migration from custom
GLTFBufferandGLTFBufferViewto the standardizedglTF.Manipulator.Schematypes is well-executed. The type alias forBufferappropriately avoids potential naming conflicts, and the logic remains unchanged.Also applies to: 14-15, 36-36
Revit_glTF_Exporter_2025/Revit_glTF_Exporter_2025.csproj (1)
78-78: The glTF.Manipulator package version2025.11.24.2122exists on NuGet and is publicly accessible.The package has been confirmed to exist on the official NuGet registry. The version follows the package's standard date-based versioning scheme and is the latest published version. However, automated security advisory verification could not be completed due to API timeouts. For comprehensive security validation, manually check the NuGet package page or use
dotnet list package --vulnerableafter restoring packages locally.Common_glTF_Exporter/Export/Draco.cs (3)
28-34: DeepClone implementation is correct but consider performance implications.The serialize-then-parse approach works but creates intermediate string allocations. For high-frequency calls, this could become a bottleneck. If
System.Text.Jsonadds nativeDeepClonesupport in future versions, consider migrating to it.
103-122: LGTM - Safe null handling in array patching.The method correctly checks for null arrays before iterating and safely casts items with null guards. The migration from
JObject/JArraytoJsonObject/JsonArrayis properly implemented.
169-246: Extension merging logic is correct and well-structured.The
MergeExtensionsUsedAndRequiredmethod properly handles nullable arrays, usesTryGetPropertyValuefor safe access, and maintains aHashSetto avoid duplicates. Good defensive programming.Common_glTF_Exporter/Export/BinFile.cs (2)
19-24: LGTM - Simplified binary write implementation.The migration from
List<GLTFBinaryData>to a singleGLTFBinaryDatawithToArray()is cleaner. UsingBufferedStreamis good practice for write performance.Note: The explicit
Flush()on line 23 is technically redundant sinceDispose()will flush, but it's harmless and makes intent explicit.
9-10: I'll help you verify the review comment about the BinFile class namespace issue. Let me start by examining the codebase to understand the namespace conventions in this project.
<function_calls>
#!/bin/bashCheck if other files in Export folder use namespaces
rg -l "^namespace" Common_glTF_Exporter/Export/
</function_calls>
<function_calls>
#!/bin/bashList all C# files in Export folder to understand project structure
fd --type f -e cs Common_glTF_Exporter/Export/
</function_calls>
<function_calls>
#!/bin/bashCheck the first few lines of BinFile.cs to see the actual namespace declaration
head -n 15 Common_glTF_Exporter/Export/BinFile.cs
</function_calls>Common_glTF_Exporter/Export/RevitGrids.cs (2)
22-22: Method signature migration looks correct.The signature update from
IndexedDictionary<GLTFNode>toIndexedDictionary<Node>aligns with the PR's objective of using theglTF.Manipulatorlibrary types.
44-61: Good use of CultureInfo.InvariantCulture for numeric serialization.Ensures consistent formatting regardless of the user's locale settings, preventing issues with decimal separators in different regions.
Common_glTF_Exporter/Export/FileExport.cs (3)
11-12: Good use of type aliases to avoid namespace conflicts.The aliases for
BufferandMaterialfromglTF.Manipulator.Schemahelp disambiguate fromAutodesk.Revit.DBtypes used elsewhere in the codebase.
21-32: LGTM - Method signature properly updated for new type system.The migration from custom GLTF types (
GLTFBufferView,GLTFBuffer,GLTFScene, etc.) to theglTF.Manipulatorlibrary types is consistent. The change fromList<GLTFBinaryData>to a singleGLTFBinaryDatasimplifies the API.
34-55: LGTM - Export flow correctly uses updated APIs.Both GLTF and GLB export paths properly call
BinFile.CreateandGlbFile.Createwith the newGLTFBinaryDataparameter. TheGltfJson.Getcalls pass the correct list types.Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
20-21: LGTM - Clean tuple return type for related color data.Returning
(Color baseColor, Color tintColor)is a clean approach for returning two related pieces of data. The nullable tuple(null, null)for error cases is handled appropriately.Also applies to: 61-62
64-89: LGTM - Well-structured texture creation helper.The
createTexturemethod cleanly encapsulates texture property calculation including scale, offset, and rotation transformations.Common_glTF_Exporter/Export/GltfJson.cs (1)
73-81: Migration to System.Text.Json looks correct.The serialization options are properly configured to ignore null values and maintain property naming. This aligns with the PR's goal of moving away from Newtonsoft.Json.
Common_glTF_Exporter/Utils/glTFExportUtils.cs (2)
14-37: Type migration to BaseMaterial looks correct.The class properly migrates from
GLTFMaterialtoBaseMaterialthroughout. TheDEF_UNIQUEL_IDconstant being made public is appropriate since it's now referenced from other files viaGLTFExportUtils.GetGLTFMaterial.
54-67: Material info assignment is correct.The method properly uses
material.uuidfor both the vertex key construction and theMaterialInfoassignment, maintaining consistency with the newBaseMaterialtype.Common_glTF_Exporter/Materials/RevitMaterials.cs (1)
25-53: Type migration and material processing flow look correct.The migration from
GLTFMaterialtoBaseMaterialis properly implemented. The method correctly:
- Creates a new
BaseMaterialwith the node's material ID as uuid- Checks for existing materials in the dictionary
- Falls back to the default GLTF material when Revit material is null
- Uses the new
Exportmethod signature with texture/image supportCommon_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (5)
13-63: LGTM!The face export logic correctly handles the index buffer creation with proper ELEMENT_ARRAY_BUFFER target and UNSIGNED_INT component type.
69-128: LGTM!The vertex export correctly converts double precision data to float32 for glTF compatibility and properly assigns the accessor to the mesh primitive's POSITION attribute.
213-214: Potential precision loss when castinglongtofloat.Casting
elementIdfromlongtofloaton line 213 can cause precision loss for element IDs exceeding ~16 million (float has ~7 significant decimal digits). While Revit element IDs typically stay within this range, verify this assumption holds for your use cases.
253-314: LGTM!The UV export correctly handles the VEC2 data format, properly calculating buffer sizes and accessor counts based on the number of UV coordinate pairs.
316-344: LGTM!The image buffer export correctly handles embedded images: getting the byte offset before appending, using
Targets.NONEfor the buffer view (appropriate for image data per glTF spec), and clearing theuriwhen embedding in the binary buffer.
| { | ||
| currentMaterial = MaterialUtils.GetGltfMeshMaterial(currentDocument, preferences, mesh, materials, true); | ||
| materials.AddOrUpdateCurrentMaterial(currentMaterial.UniqueId, currentMaterial, true); | ||
| materials.AddOrUpdateCurrentMaterial(currentMaterial.name, currentMaterial, true); |
There was a problem hiding this comment.
Inconsistent key used: name vs uuid.
Other calls to AddOrUpdateCurrentMaterial use uuid as the key (e.g., line 116 uses currentMaterial.uuid), but this call uses currentMaterial.name. This inconsistency may cause duplicate materials or lookup failures when the same material is referenced later by uuid.
- materials.AddOrUpdateCurrentMaterial(currentMaterial.name, currentMaterial, true);
+ materials.AddOrUpdateCurrentMaterial(currentMaterial.uuid, currentMaterial, true);📝 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.
| materials.AddOrUpdateCurrentMaterial(currentMaterial.name, currentMaterial, true); | |
| materials.AddOrUpdateCurrentMaterial(currentMaterial.uuid, currentMaterial, true); |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Core/GlTFExportContext.cs around line 385, the call uses
currentMaterial.name as the dictionary key while other callers use
currentMaterial.uuid; update this call to use currentMaterial.uuid to keep keys
consistent and avoid duplicate/lookup issues, and ensure currentMaterial and its
uuid are not null before calling AddOrUpdateCurrentMaterial.
| if (preferences.materials != MaterialsEnum.nonematerials) | ||
| { | ||
| primitive.material = materials.GetIndexFromUUID(kvp.Value.MaterialInfo.uuid); | ||
| } |
There was a problem hiding this comment.
Potential null reference or missing key exception.
If kvp.Value.MaterialInfo is null or its uuid is not present in materials, GetIndexFromUUID will throw. Consider adding a guard:
if (preferences.materials != MaterialsEnum.nonematerials)
{
- primitive.material = materials.GetIndexFromUUID(kvp.Value.MaterialInfo.uuid);
+ if (kvp.Value.MaterialInfo != null && materials.Contains(kvp.Value.MaterialInfo.uuid))
+ {
+ primitive.material = materials.GetIndexFromUUID(kvp.Value.MaterialInfo.uuid);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Core/ProcessGeometry.cs around lines 89 to 92, there's a
potential null-reference or missing-key when calling
materials.GetIndexFromUUID(kvp.Value.MaterialInfo.uuid); add guards to ensure
kvp.Value.MaterialInfo is not null and its uuid is not null/empty before calling
GetIndexFromUUID, and verify that materials contains that uuid (or that
GetIndexFromUUID returns a safe sentinel) — if any check fails, avoid calling
GetIndexFromUUID and either skip assigning primitive.material or assign a safe
default index and optionally log a warning with context (mesh/primitive id).
| public class MaterialTexture | ||
| { | ||
| public string EmbeddedTexturePath { get; set; } = null; | ||
| public double Fadevalue { get; set; } = 1; |
There was a problem hiding this comment.
Fix property naming convention.
The property Fadevalue violates C# naming conventions. It should be FadeValue (PascalCase with capital 'V').
Apply this diff:
- public double Fadevalue { get; set; } = 1;
+ public double FadeValue { get; set; } = 1;Note: Ensure all references to this property are updated throughout the codebase.
📝 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 double Fadevalue { get; set; } = 1; | |
| public double FadeValue { get; set; } = 1; |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/MaterialTexture.cs around line 6, rename the
property Fadevalue to FadeValue to follow C# PascalCase naming conventions;
update the property declaration and then search & replace all references across
the codebase (including usages, serialization keys, unit tests, XAML/JSON
mappings, and any reflection-based access) to use FadeValue, rebuild to catch
compile errors, and run tests to ensure nothing else breaks.
| public static Autodesk.Revit.DB.Material GetMeshMaterial(Document doc, Autodesk.Revit.DB.Mesh mesh) | ||
| { | ||
| ElementId materialId = mesh.MaterialElementId; | ||
|
|
||
| if (materialId != null) | ||
| { | ||
| return doc.GetElement(materialId) as Material; | ||
| return doc.GetElement(materialId) as Autodesk.Revit.DB.Material; | ||
| } | ||
| else | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect null check for ElementId.
ElementId is a struct (value type) in the Revit API, so comparing it to null may not work as expected. The correct check is against ElementId.InvalidElementId.
public static Autodesk.Revit.DB.Material GetMeshMaterial(Document doc, Autodesk.Revit.DB.Mesh mesh)
{
ElementId materialId = mesh.MaterialElementId;
- if (materialId != null)
+ if (materialId != ElementId.InvalidElementId)
{
return doc.GetElement(materialId) as Autodesk.Revit.DB.Material;
}
else
{
return null;
}
}📝 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 static Autodesk.Revit.DB.Material GetMeshMaterial(Document doc, Autodesk.Revit.DB.Mesh mesh) | |
| { | |
| ElementId materialId = mesh.MaterialElementId; | |
| if (materialId != null) | |
| { | |
| return doc.GetElement(materialId) as Material; | |
| return doc.GetElement(materialId) as Autodesk.Revit.DB.Material; | |
| } | |
| else | |
| { | |
| return null; | |
| } | |
| } | |
| public static Autodesk.Revit.DB.Material GetMeshMaterial(Document doc, Autodesk.Revit.DB.Mesh mesh) | |
| { | |
| ElementId materialId = mesh.MaterialElementId; | |
| if (materialId != ElementId.InvalidElementId) | |
| { | |
| return doc.GetElement(materialId) as Autodesk.Revit.DB.Material; | |
| } | |
| else | |
| { | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Utils/MaterialUtils.cs around lines 14 to 26, the code
checks mesh.MaterialElementId against null but ElementId is a value type;
replace the null check with a comparison to ElementId.InvalidElementId (e.g., if
(materialId != ElementId.InvalidElementId)) and only call
doc.GetElement(materialId) when the id is valid, returning null otherwise.
| gl_mat.baseColorFactor = new List<float>(4) { material.Color.Red / 255f, material.Color.Green / 255f, material.Color.Blue / 255f, opacity }; | ||
| gl_mat.metallicFactor = 0f; | ||
| gl_mat.roughnessFactor = 1f; |
There was a problem hiding this comment.
Color values not converted to linear color space.
The color values here are divided by 255 to normalize, but unlike MaterialProperties.SetMaterialColour, they are not converted from sRGB to linear color space. glTF expects linear color values in baseColorFactor. This inconsistency could lead to visually incorrect materials.
Consider using the same color conversion path:
-gl_mat.baseColorFactor = new List<float>(4) { material.Color.Red / 255f, material.Color.Green / 255f, material.Color.Blue / 255f, opacity };
+var baseColours = MaterialProperties.RgbToUnit(material.Color);
+gl_mat.baseColorFactor = MaterialProperties.GetLinearColour(baseColours, opacity);This ensures consistent sRGB-to-linear conversion across the codebase.
📝 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.
| gl_mat.baseColorFactor = new List<float>(4) { material.Color.Red / 255f, material.Color.Green / 255f, material.Color.Blue / 255f, opacity }; | |
| gl_mat.metallicFactor = 0f; | |
| gl_mat.roughnessFactor = 1f; | |
| var baseColours = MaterialProperties.RgbToUnit(material.Color); | |
| gl_mat.baseColorFactor = MaterialProperties.GetLinearColour(baseColours, opacity); | |
| gl_mat.metallicFactor = 0f; | |
| gl_mat.roughnessFactor = 1f; |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Utils/MaterialUtils.cs around lines 46 to 48, the
baseColorFactor is set by dividing RGB by 255 but not converted from sRGB to
linear, causing inconsistent/incorrect colors; update this assignment to pass
the material.Color through the same sRGB-to-linear conversion used by
MaterialProperties.SetMaterialColour (or call the shared helper) and then
normalize to 0–1 before creating the List<float>, preserving the existing
opacity value.
| int elemInView = elementsInView.Count; | ||
| ExportLog.Write($"{elemInView} elements will be exported"); | ||
| ProgressBarWindow progressBar = | ||
| ProgressBarWindow.Create(elementsInView.Count + 1, 0, "Converting elements...", this); | ||
| ProgressBarWindow.Create(elemInView + 1, 0, "Converting elements...", this); | ||
|
|
There was a problem hiding this comment.
Off-by-one error: progress bar won't reach 100%.
The progress bar is initialized with elemInView + 1 as the maximum value (line 97), but the final value is set to elemInView (line 115). This mismatch means the progress bar will never reach 100% completion, stopping at approximately 99%.
Apply this diff to fix the issue:
- ProgressBarWindow.ViewModel.ProgressBarValue = elemInView;
+ ProgressBarWindow.ViewModel.ProgressBarValue = elemInView + 1;Also applies to: 115-115
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Windows/MainWindow.xaml.cs around lines 94-98 (and also
line 115), the progress bar maximum is initialized as elemInView + 1 while the
final progress is set to elemInView, causing an off-by-one so the bar never
reaches 100%; fix this by making the progress max and final value
consistent—either change the ProgressBarWindow.Create call to use elemInView
(remove the +1) so the max equals the final value, or alternatively increment
the final progress to elemInView + 1; update the occurrence at line 115
accordingly so both initialization and final update match.
1 participant
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.