Conversation
WalkthroughThis update removes custom UV mapping logic and associated source files, centralizing texture property handling in a new Changes
Sequence Diagram(s)sequenceDiagram
participant RevitMaterials
participant MaterialTextures
participant AssetPropertiesUtils
participant GLTFMaterial
RevitMaterials->>MaterialTextures: SetMaterialTextures(material, gl_mat, doc, opacity)
MaterialTextures->>AssetPropertiesUtils: GetTexturePath(asset)
MaterialTextures->>AssetPropertiesUtils: GetScale(asset)
MaterialTextures->>AssetPropertiesUtils: GetRotationRadians(asset)
MaterialTextures->>AssetPropertiesUtils: GetTint(asset)
MaterialTextures->>AssetPropertiesUtils: GetFade(asset)
MaterialTextures->>GLTFMaterial: Set texture properties (embed, scale, rotation, tint, fade, etc.)
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 9
🧹 Nitpick comments (5)
Revit_glTF_Exporter_2021/Revit_glTF_Exporter_2021.csproj (1)
95-100: Preferxcopy /Y /I /Q(orrobocopy) over barecopyfor wildcard directory copies.
copytreats the destination as a filename; with*sources it can prompt or fail.
xcopy(orrobocopy) handles directories, preserves attributes, and suppresses prompts with flags:-copy "$(ProjectDir)$(OutputPath)\Images\*" "$(APPDATA)\Autodesk\ApplicationPlugins\leia.bundle\Contents\2021\Images\" +xcopy "$(ProjectDir)$(OutputPath)\Images\*" "$(APPDATA)\Autodesk\ApplicationPlugins\leia.bundle\Contents\2021\Images\" /Y /I /QReplicate the switch for the remaining
copycommands to make the post-build step idempotent and CI-friendly.Common_glTF_Exporter/Materials/RevitMaterials.cs (1)
10-13: Remove unused imports.The added imports
System.IO.Ports,System.Windows.Controls, andSystem.Windows.Media.Media3Ddon't appear to be used in this file.-using System.IO.Ports; -using System.Windows.Controls; -using System.Windows.Media.Media3D;Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
10-12: Remove unused imports to improve code clarity.The following imports appear to be unused in this file:
System.IO.Ports(line 10)System.Windows.Controls(line 11)System.Windows.Media.Media3D(line 12)-using System.IO.Ports; -using System.Windows.Controls; -using System.Windows.Media.Media3D;
20-21: Consider improving method parameter formatting for better readability.The method signature spans multiple lines inconsistently. Consider aligning the parameters properly.
- public static GLTFMaterial SetMaterialTextures(Material material, GLTFMaterial gl_mat, - Document doc, float opacity) + public static GLTFMaterial SetMaterialTextures(Material material, GLTFMaterial gl_mat, + Document doc, float opacity)Revit_glTF_Exporter_2024/Revit_glTF_Exporter_2024.csproj (1)
87-100: Post-build script duplication across versions – move to a shared targetNearly identical post-build blocks now exist in every yearly project file (2019-2024). Centralising them in an MSBuild
.targets/shared props file (or invoking a single PowerShell/batch script) will eliminate copy-paste drift and ease future path updates (e.g. next Revit version).Small upfront refactor; big long-term maintenance win.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Common_glTF_Exporter/Common_glTF_Exporter.projitems(1 hunks)Common_glTF_Exporter/Core/GLTFMaterial.cs(1 hunks)Common_glTF_Exporter/Core/GlTFExportContext.cs(2 hunks)Common_glTF_Exporter/Materials/AssetProperties.cs(6 hunks)Common_glTF_Exporter/Materials/BitmapsUtils.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialTextures.cs(1 hunks)Common_glTF_Exporter/Materials/RevitMaterials.cs(2 hunks)Common_glTF_Exporter/UVs/CylindricalUv.cs(0 hunks)Common_glTF_Exporter/UVs/PlanarUv.cs(0 hunks)Common_glTF_Exporter/UVs/VertexUvs.cs(0 hunks)Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs(1 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)
💤 Files with no reviewable changes (3)
- Common_glTF_Exporter/UVs/PlanarUv.cs
- Common_glTF_Exporter/UVs/CylindricalUv.cs
- Common_glTF_Exporter/UVs/VertexUvs.cs
🧰 Additional context used
🧬 Code Graph Analysis (4)
Common_glTF_Exporter/Core/GLTFMaterial.cs (1)
Common_glTF_Exporter/Materials/AssetProperties.cs (2)
Autodesk(61-78)Autodesk(113-139)
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (2)
BitmapsUtils(11-138)BlendImageWithColor(32-116)
Common_glTF_Exporter/Materials/MaterialTextures.cs (4)
Common_glTF_Exporter/Materials/AssetProperties.cs (8)
Autodesk(61-78)Autodesk(113-139)Asset(20-34)AssetPropertiesUtils(9-152)GetTexturePath(36-59)GetScale(93-111)GetRotationRadians(80-91)GetFade(141-151)Common_glTF_Exporter/Core/GLTFMaterial.cs (1)
GLTFMaterial(14-44)Common_glTF_Exporter/Materials/RevitMaterials.cs (1)
GLTFMaterial(30-69)Common_glTF_Exporter/Core/GLTFTextureInfo.cs (3)
GLTFTextureInfo(16-23)GLTFTextureExtensions(33-37)GLTFTextureTransform(25-31)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (2)
Common_glTF_Exporter/Materials/AssetProperties.cs (2)
Autodesk(61-78)Autodesk(113-139)Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
SrgbToLinear(39-44)
⏰ 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 (23)
Revit_glTF_Exporter_2023/Revit_glTF_Exporter_2023.csproj (1)
97-99: Validate per-user deployment strategySwitching from
$(ALLUSERSPROFILE)to$(APPDATA)moves the bundle from a machine-wide to a per-user location.
Revit will load add-ins from either path, so leaving an old copy under ProgramData (“All Users”) can result in duplicate plug-ins or stale resources being preferred over the fresh ones.Consider adding an explicit clean-up step (or at least documenting the manual action) that removes the legacy
…\ProgramData\Autodesk\ApplicationPlugins\leia.bundlefolder on developer / CI machines.
This prevents “ghost” copies that may still reference the missing-texture layout you’re fixing here.Revit_glTF_Exporter_2021/Revit_glTF_Exporter_2021.csproj (1)
93-101: Verify Revit-add-in discovery when switching fromALLUSERSPROFILEtoAPPDATA.Revit’s documented lookup paths for
ApplicationPluginsdefault to%ProgramData%(≙ALLUSERSPROFILE) rather than%AppData%.
Moving the bundle to a per-user roaming folder may prevent Revit from loading the add-in unless a custom search path is configured.Request a quick smoke-test on a clean machine (no prior installer) to confirm that the
.bundledropped under:%AppData%\Autodesk\ApplicationPlugins\is still discovered by Revit 2021.
If that is not the case, consider copying to both locations or falling back to the original path in Release builds.Revit_glTF_Exporter_2019/Revit_glTF_Exporter_2019.csproj (1)
98-108: Verify per-user install location is acceptable for all deployment scenariosChanging from
$(ALLUSERSPROFILE)(%ProgramData%) to$(APPDATA)switches the bundle from a machine-wide to a per-user install.Revit does support
%AppData%\Autodesk\ApplicationPlugins, but this breaks:• company-wide deployments via MSI/GPO
• multiple-user machines (each user will compile/copy separately)Please confirm this is intentional for 2019 exporter; otherwise consider making the target configurable (e.g. an MSBuild property) or keep both locations.
Revit_glTF_Exporter_2020/Revit_glTF_Exporter_2020.csproj (1)
142-148: Good move to user-scope deployment, but double-check non-Debug builds.The post-build script only runs when
$(ConfigurationName) == Debug. Release and custom Revit-specific configurations now deploy to the old All Users location (or nowhere).
Confirm this is intentional; if not, replicate the updated paths (or refactor into a shared target) for parity across build types.Common_glTF_Exporter/Core/GLTFMaterial.cs (1)
36-37: LGTM! Clean property addition.The new
TintColourproperty follows the established pattern with the[JsonIgnore]attribute and matches the type convention used forBaseColor. This integrates well with the tinting functionality introduced in the broader refactor.Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
217-218: LGTM! Correctly integrates tint color support.The addition of
material.TintColourparameter properly integrates the new tinting functionality into the image blending pipeline. The parameter order matches the updatedBitmapsUtils.BlendImageWithColormethod signature.Common_glTF_Exporter/Core/GlTFExportContext.cs (2)
301-301: Direct UV retrieval simplifies the logic.Good simplification by directly retrieving UVs from the polymesh instead of using the removed UV calculation classes.
317-321: UV mapping logic validated – no changes required
- No references to deprecated UV classes (e.g., VertexUvs, CylindricalUv, PlanarUv) remain.
- In GLTFBinaryDataUtils.cs, UVs are collected and exported from geomData.Uvs only when present.
- In glTFExportUtils.cs, UV export is gated by both
baseColorTexture != nullandgeomData.Uvs.Count > 0, matching the new addition condition.The updated conditional (
preferences.materials == MaterialsEnum.textures && currentMaterial?.EmbeddedTexturePath != null) aligns with the existing export checks and preserves the previous UV mapping behavior.Common_glTF_Exporter/Common_glTF_Exporter.projitems (1)
43-43: LGTM! Project file correctly reflects the refactoring.The addition of
MaterialTextures.csaligns with the centralized texture property handling introduced in this refactor, replacing the removed UV calculation classes.Common_glTF_Exporter/Materials/RevitMaterials.cs (1)
65-65: Excellent refactoring to centralize texture handling.The replacement of inline texture extraction logic with
MaterialTextures.SetMaterialTexturesimproves code organization and maintainability by centralizing texture property management.Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
77-78: LGTM: Texture scaling logic is mathematically correct.The inverse scaling calculation
1f / scaleXand1f / scaleYcorrectly converts from material space to UV space coordinates.
70-81: Resolve Texture Index Placeholder UsageThe
index = -1acts as a sentinel indicating “unassigned” forbaseColorTexture. InGLTFBinaryDataUtils.cs, the exporter checks forindex == -1and then embeds the texture and updatesbaseColorTexture.indexwith the actual texture index. No change is needed here.Common_glTF_Exporter/Materials/BitmapsUtils.cs (5)
39-41: LGTM: Improved early exit logic with tint color consideration.The early exit optimization correctly checks for both flat color blending and tint color to avoid unnecessary processing when no color modifications are needed.
44-55: LGTM: Proper handling of flat color blending parameters.The logic correctly handles the case where flat color blending is disabled by setting appropriate blend factors and converting flat colors to linear space only when needed.
57-64: LGTM: Correct tint color initialization and conversion.The tint factors are properly initialized to neutral values (1.0) and converted to linear space only when a tint color is provided. This ensures multiplicative tinting works correctly.
85-97: LGTM: Mathematically correct color blending and tinting implementation.The pixel processing correctly:
- Converts sRGB to linear color space
- Applies flat color blending using fade factors
- Applies multiplicative tinting
- Converts back to sRGB for output
The order of operations is correct for realistic color blending.
125-137: LGTM: Standard sRGB/linear color space conversion functions.The
SrgbToLinearandLinearToSrgbfunctions implement the standard sRGB specification correctly, matching the implementation inMaterialProperties.cs.Common_glTF_Exporter/Materials/AssetProperties.cs (6)
11-17: LGTM: Improved type safety with strongly-typed constants.Replacing hardcoded strings with strongly-typed constants from the Autodesk.Revit.DB.Visual namespace improves maintainability and reduces the risk of typos.
40-40: LGTM: Consistent use of strongly-typed constants.Updated to use
UnifiedBitmap.UnifiedbitmapBitmapconstant instead of hardcoded string, improving type safety.
66-66: LGTM: Consistent constant usage for diffuse color property.Updated to use
Generic.GenericDiffuseconstant, maintaining consistency with the DIFFUSE_NAMES array.
82-83: LGTM: Consistent constant usage for texture rotation.Updated to use
UnifiedBitmap.TextureWAngleconstant for better type safety.
113-139: LGTM: Well-implemented tint color extraction with proper validation.The
GetTintmethod correctly:
- Checks if tinting is enabled via the toggle property
- Extracts RGBA values from the tint color property
- Converts from normalized doubles to byte values
- Returns null when tinting is disabled
The logic handles edge cases appropriately.
141-151: LGTM: Simple and correct fade value extraction.The
GetFademethod properly extracts the fade value from the asset property and provides a sensible default value of 1.0 when the property is not found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Common_glTF_Exporter/Materials/MaterialTextures.cs (1)
50-74: Consider setting baseColorFactor to incorporate BaseColor values.The method sets various texture properties including
BaseColor(line 60), but doesn't set thebaseColorFactoron the PBR material. Based on the past review comment and the pattern inMaterialProperties.cs, you might want to consider setting the baseColorFactor here to incorporate the actual BaseColor values rather than relying on the hardcoded white values set elsewhere.However, since this is now handled by the separate
SetMaterialColourmethod inMaterialProperties.cs, this concern may be addressed through that integration point.
🧹 Nitpick comments (1)
Common_glTF_Exporter/Materials/MaterialTextures.cs (1)
1-15: Remove unused imports to improve code clarity.Several imports appear to be unused and should be removed:
System.IO.Ports(line 10)System.Windows.Controls(line 11)System.Windows.Media.Media3D(line 12)-using System.IO.Ports; -using System.Windows.Controls; -using System.Windows.Media.Media3D;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Common_glTF_Exporter/Core/GlTFExportContext.cs(3 hunks)Common_glTF_Exporter/Materials/BitmapsUtils.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialProperties.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialTextures.cs(1 hunks)Common_glTF_Exporter/Materials/RevitMaterials.cs(2 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)
🚧 Files skipped from review as they are similar to previous changes (9)
- Revit_glTF_Exporter_2024/Revit_glTF_Exporter_2024.csproj
- Revit_glTF_Exporter_2019/Revit_glTF_Exporter_2019.csproj
- Common_glTF_Exporter/Core/GlTFExportContext.cs
- Revit_glTF_Exporter_2021/Revit_glTF_Exporter_2021.csproj
- Revit_glTF_Exporter_2023/Revit_glTF_Exporter_2023.csproj
- Common_glTF_Exporter/Materials/BitmapsUtils.cs
- Revit_glTF_Exporter_2020/Revit_glTF_Exporter_2020.csproj
- Revit_glTF_Exporter_2022/Revit_glTF_Exporter_2022.csproj
- Common_glTF_Exporter/Materials/RevitMaterials.cs
⏰ 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 (3)
Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
22-56: LGTM - Well-structured conditional color handling.The new
SetMaterialColourmethod correctly implements conditional baseColorFactor setting:
- When no embedded texture exists, it properly converts the node's sRGB color to linear space
- When an embedded texture is present, it sets baseColorFactor to white to allow the texture to show through
- The sRGB to linear conversion is mathematically correct for glTF compliance
This approach aligns with glTF best practices where baseColorFactor acts as a multiplier for the base color texture.
Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
20-48: LGTM - Robust texture extraction with proper validation.The
SetMaterialTexturesmethod implements good defensive programming:
- Validates
AppearanceAssetIdis not invalid- Checks if
AppearanceAssetElementexists- Verifies texture path exists on disk before processing
- Properly delegates texture setup to private method
The logic flow is clear and handles edge cases appropriately.
62-73: Please confirm the intended inversion of texture scale values.The code currently inverts the real-world scales:
- File
Common_glTF_Exporter/Materials/MaterialTextures.cs(lines 62–73)scale = new float[] { 1f / scaleX, 1f / scaleY },Ensure that using
1f/scaleXand1f/scaleYproduces the correct UV scaling direction in your target runtime. If this inversion is deliberate (e.g., converting world units to UV space), no change is needed; otherwise adjust the scale factors to match your desired behavior.
1 participant
Summary by CodeRabbit
New Features
Refactor