Polish texture mapping #107
Conversation
WalkthroughThis update introduces a comprehensive refactor and expansion of material and texture handling in the glTF exporter. It adds new utility classes for materials, bitmaps, and UV mapping, reorganizes project structure, and enhances the CI workflows to handle certificates and versioning across multiple Revit exporter versions. The GLTFMaterial and export logic are extended for more robust and flexible material export. Changes
Sequence Diagram(s)sequenceDiagram
participant Exporter as RevitMaterials.Export
participant Revit as Revit API
participant AssetUtils as AssetPropertiesUtils
participant BitmapUtils as BitmapsUtils
participant MaterialProps as MaterialProperties
participant UVUtils as VertexUvs
Exporter->>Revit: Get Material by ElementId
Exporter->>MaterialProps: SetProperties(node, opacity, ...)
alt Preferences specify textures
Exporter->>Revit: Get AppearanceAsset
Exporter->>AssetUtils: GetDiffuseBitmap(asset)
AssetUtils-->>Exporter: BitmapAsset
Exporter->>AssetUtils: GetTexturePath(BitmapAsset)
Exporter->>AssetUtils: GetScale/Rotation(BitmapAsset)
Exporter->>BitmapUtils: BlendImageWithColor(imageBytes, fade, baseColor, format)
end
Exporter->>UVUtils: AddUvToVertex(...)
Exporter-->>Exporter: Return GLTFMaterial
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: 4
♻️ Duplicate comments (5)
.github/workflows/dev_pull_request.yml (5)
13-18: Propagate action-version upgrades here as well
actions/checkout@v3→v4andactions/setup-dotnet@v1→v3, mirroring the changes recommended for the dev workflow.Also applies to: 18-26
31-38: Add-yto WiX uninstall/installSame non-interactive concern as noted in
dev_actions.yml.
51-60: Apply typo fix & secret maskingReplicate the “certificate” typo fix and secret masking pattern.
67-106: Consolidate version-bump steps with a PowerShell loopMirrors the suggestion in the dev workflow; keeps both files in sync.
30-30: Remove trailing whitespace flagged by YAML-lintClean up the lines cited by the linter to avoid cosmetic CI failures.
Also applies to: 44-44, 66-66, 70-74
🧹 Nitpick comments (8)
.github/workflows/dev_actions.yml (3)
53-61: Typos in step names & masking of secrets
- “certfificate” is misspelled twice – minor, but it pollutes step history.
- Echoing the certificate key directly risks accidental log exposure (GitHub masks, but future tools might not). Prefer explicit masking:
- echo "CERTIFICATE_KEY=${{ secrets.EVERSE_CERTIFICATE_KEY }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf8 -Append + echo "::add-mask::${{ secrets.EVERSE_CERTIFICATE_KEY }}" + echo "CERTIFICATE_KEY=${{ secrets.EVERSE_CERTIFICATE_KEY }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf8 -Append
69-108: Repetitive PowerShell replacements – use a loop for maintainabilityThe eight nearly identical “Setting new version” steps can be collapsed into a loop, reducing maintenance overhead when new versions are added:
$paths = @( 'Revit_glTF_Exporter_2019/Properties/AssemblyInfo.cs', 'Revit_glTF_Exporter_2020/Properties/AssemblyInfo.cs', ... 'Common_glTF_Exporter/PackageContents.xml' ) foreach ($p in $paths) { (Get-Content $p) -replace '1\.0\.0', $env:FULL_VERSION | Out-File -Encoding ASCII $p }
7-8: Trim trailing whitespace to pass YAML-lintThe workflow currently fails
yamllintdue to trailing spaces on several lines (7, 32, 46, 68, 72, 76). Remove them to keep CI green.Also applies to: 32-33, 46-47, 68-76
Common_glTF_Exporter/Core/GLTFMaterial.cs (2)
5-5: Remove unused using directive.The
System.Drawingnamespace is imported but not used in this file. TheBaseColorproperty usesAutodesk.Revit.DB.Colorinstead.- using System.Drawing;
34-34: Follow C# naming conventions for property names.The property name should be
FadeValueinstead ofFadevalueto follow proper PascalCase naming conventions.- public double Fadevalue { get; set; } = 1; + public double FadeValue { get; set; } = 1;Common_glTF_Exporter/UVs/CylindricalUv.cs (1)
36-39: Consider documenting the orientation adjustmentThe orientation adjustment based on
OrientationMatchesParametricOrientationis correct but could benefit from a comment explaining why the V parameter is negated in this case.+ // Adjust V parameter based on surface orientation if (!surf.OrientationMatchesParametricOrientation) vParam = -vParam;Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
32-32: Consider explaining the roughness factor logicThe roughness factor assignment (
opacity != 1 ? 0.5f : 1f) could benefit from a comment explaining why transparent materials get reduced roughness.+ // Transparent materials typically appear smoother due to surface effects pbr.roughnessFactor = opacity != 1 ? 0.5f : 1f;Common_glTF_Exporter/Materials/BitmapsUtils.cs (1)
70-70: Fix typo in comment.The comment contains a typo that should be corrected.
- // sRGB → lineal + // sRGB → linear
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/dev_actions.yml(1 hunks).github/workflows/dev_pull_request.yml(1 hunks)Common_glTF_Exporter/Common_glTF_Exporter.projitems(2 hunks)Common_glTF_Exporter/Core/GLTFMaterial.cs(2 hunks)Common_glTF_Exporter/Core/GlTFExportContext.cs(5 hunks)Common_glTF_Exporter/Export/RevitMaterials.cs(0 hunks)Common_glTF_Exporter/Materials/AssetProperties.cs(1 hunks)Common_glTF_Exporter/Materials/BitmapsUtils.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialProperties.cs(1 hunks)Common_glTF_Exporter/Materials/RevitMaterials.cs(1 hunks)Common_glTF_Exporter/Model/MaterialCacheDTO.cs(1 hunks)Common_glTF_Exporter/UVs/CylindricalUv.cs(1 hunks)Common_glTF_Exporter/UVs/PlanarUv.cs(1 hunks)Common_glTF_Exporter/UVs/VertexUvs.cs(1 hunks)Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs(3 hunks)
💤 Files with no reviewable changes (1)
- Common_glTF_Exporter/Export/RevitMaterials.cs
🧰 Additional context used
🧬 Code Graph Analysis (3)
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (3)
ImageFormat(13-30)BitmapsUtils(11-118)BlendImageWithColor(32-96)
Common_glTF_Exporter/Materials/MaterialProperties.cs (4)
Common_glTF_Exporter/Core/glTFPBR.cs (1)
GLTFPBR(5-16)Common_glTF_Exporter/Core/GLTFMaterial.cs (1)
GLTFMaterial(14-41)Common_glTF_Exporter/Materials/RevitMaterials.cs (1)
GLTFMaterial(29-126)Common_glTF_Exporter/Materials/BitmapsUtils.cs (2)
SrgbToLinear(105-110)Pow(101-102)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (2)
Common_glTF_Exporter/Materials/AssetProperties.cs (1)
Autodesk(57-74)Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
SrgbToLinear(39-44)
🪛 actionlint (1.7.7)
.github/workflows/dev_actions.yml
16-16: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
21-21: the runner of "actions/setup-dotnet@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/setup-dotnet@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/dev_pull_request.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-19: the runner of "actions/setup-dotnet@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
24-24: the runner of "actions/setup-dotnet@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/dev_actions.yml
[error] 7-7: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 72-72: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
.github/workflows/dev_pull_request.yml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (28)
Common_glTF_Exporter/Core/GLTFMaterial.cs (1)
33-37: LGTM on the new material properties.The
JsonIgnoreattributes are appropriately used for internal properties, and the default value of 1 for fade value makes sense for full opacity.Common_glTF_Exporter/Model/MaterialCacheDTO.cs (1)
7-17: LGTM on the DTO design.Clean and simple DTO implementation with proper constructor initialization and following C# naming conventions.
Common_glTF_Exporter/UVs/PlanarUv.cs (1)
8-23: LGTM on the planar UV calculation logic.The mathematical approach using normalized direction vectors and dot products is correct for planar UV mapping. Clean implementation following good practices.
Common_glTF_Exporter/Common_glTF_Exporter.projitems (1)
40-42: LGTM on project structure improvements.Good organization of related functionality into dedicated directories (Materials, UVs) and proper integration of new utility classes.
Also applies to: 46-46, 51-51, 72-74
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (5)
1-9: LGTM: Using statements properly organized for new functionalityThe addition of
System.Drawing.ImagingandCommon_glTF_Exporter.Materialsnamespaces appropriately supports the new image processing and material utilities integration.
215-217: Good refactoring to use centralized MIME type detectionThe refactoring to use
BitmapsUtils.GetMimeTypecentralizes MIME type logic and returns both the MIME type string andImageFormatobject as a tuple, which is cleaner than the previous inline implementation.
219-234: Consistent usage of blended bytes throughout the methodAll references to the original
imageByteshave been correctly updated to useblendedBytes, ensuring the blended image is used consistently throughout the buffer creation process.
254-254: Proper usage of tuple item for MIME typeUsing
mimeType.Item1correctly accesses the MIME type string from the tuple returned byBitmapsUtils.GetMimeType.
217-217: ```shell
#!/bin/bashShow the BlendImageWithColor method implementation with context
rg -n -C 10 "public static byte\[\] BlendImageWithColor" Common_glTF_Exporter/Materials/BitmapsUtils.cs
</details> <details> <summary>Common_glTF_Exporter/UVs/CylindricalUv.cs (4)</summary> `10-14`: **Good defensive programming with surface type validation** The method properly validates that the face is actually cylindrical and throws an appropriate exception with a clear message if not. The null-coalescing with throw expression is clean and modern C#. --- `16-20`: **Correct extraction of cylindrical surface parameters** The extraction of origin, axis directions, and radius from the cylindrical surface is implemented correctly, with proper normalization of the direction vectors. --- `22-27`: **Proper handling of degenerate case** The check for zero-length radial vector (point on cylinder axis) is a good edge case handling, returning UV(0, vParam) which is mathematically correct for points on the axis. --- `29-34`: **Mathematically correct UV parameter calculation** The calculation using `Math.Atan2(sinT, cosT)` correctly computes the angular parameter, and the normalization to [0, 2π] range handles negative angles properly. </details> <details> <summary>Common_glTF_Exporter/Materials/MaterialProperties.cs (2)</summary> `12-37`: **Well-structured material property setup with proper color space conversion** The method correctly: - Converts RGB values from [0-255] to [0-1] range - Applies sRGB to linear conversion for accurate color representation - Sets appropriate PBR values including metallic factor and roughness based on opacity - Handles alpha mode selection based on transparency The logic is sound and follows glTF specification requirements. --- `39-44`: **sRGB to linear conversion implementation is correct** The implementation correctly follows the sRGB specification with the proper threshold (0.04045) and conversion formulas for both linear and gamma-corrected portions of the curve. This matches the implementation in `BitmapsUtils.SrgbToLinear`. </details> <details> <summary>Common_glTF_Exporter/Materials/AssetProperties.cs (5)</summary> `16-30`: **Robust diffuse bitmap asset retrieval** The method correctly iterates through multiple possible diffuse property names and safely handles null properties and missing connections. The early return pattern is clean and efficient. --- `32-55`: **Comprehensive texture path resolution with proper fallbacks** The method handles: - Null asset checks - Path extraction with pipe delimiter splitting (Revit-specific) - Path separator normalization - Relative path resolution against Autodesk shared materials directory This covers the common scenarios for Revit texture path handling. --- `57-74`: **Proper color extraction with range validation** The color extraction correctly: - Uses the appropriate diffuse property name - Handles null property cases - Converts double array values to byte range [0-255] - Returns InvalidColorValue as a sensible fallback --- `76-86`: **Correct rotation conversion from degrees to radians** The rotation extraction properly converts from degrees (Revit's format) to radians (glTF's expected format) using the standard conversion formula. --- `88-105`: **Version-aware unit conversion handling** The method correctly handles Revit version differences in unit conversion APIs: - Uses `DisplayUnitType` for Revit 2019/2020 - Uses `UnitTypeId` for newer versions - Provides sensible default value (1) when scale property is missing This ensures compatibility across different Revit versions. </details> <details> <summary>Common_glTF_Exporter/Materials/RevitMaterials.cs (6)</summary> `24-24`: **Good caching strategy to minimize API calls** The static dictionary cache for material names and unique IDs will improve performance by avoiding repeated Revit API calls for the same materials. --- `29-58`: **Well-structured material retrieval with proper caching** The material retrieval logic correctly: - Checks cache first to avoid API calls - Handles missing materials gracefully - Populates cache for future use - Maintains both name and unique ID information The cache hit path properly retrieves the material element for subsequent operations. --- `60-62`: **Good separation of concerns using utility classes** The delegation to `MaterialProperties.SetProperties` properly separates the PBR property setup logic into a dedicated utility class, improving maintainability. --- `64-84`: **Robust texture asset extraction with proper validation** The texture extraction logic properly: - Checks user preferences for texture export - Validates appearance asset existence - Uses utility methods for asset property extraction - Handles each step with appropriate null checks --- `86-108`: **Comprehensive texture processing with fade support** The texture processing correctly handles: - File existence validation - Scale and rotation extraction using utility methods - Fade property detection and application - Base color factor override for faded materials The fade handling logic properly sets white base color factors when fade is present, allowing the texture blending to control the final appearance. --- `110-122`: **Proper texture transform setup** The texture transform configuration correctly: - Sets index to -1 for future assignment during buffer export - Calculates inverse scale values (1/scaleX, 1/scaleY) for proper UV transformation - Applies rotation value in radians This follows glTF texture transform extension specifications. </details> <details> <summary>Common_glTF_Exporter/Materials/BitmapsUtils.cs (1)</summary> `13-30`: **LGTM!** The MIME type detection logic is clear and handles common image formats appropriately. </details> <details> <summary>Common_glTF_Exporter/Core/GlTFExportContext.cs (1)</summary> `1-378`: **Good refactoring to improve code organization!** The changes properly utilize the new UV utility methods and consistently use fully qualified type names to prevent ambiguity. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| - name: Checkout | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Update deprecated GitHub Actions to latest major versions
actions/checkout@v3 and actions/setup-dotnet@v1 run on the Node 16 runtime, which is scheduled for deprecation. Bump them to the latest major (v4 and v3, respectively) to avoid future workflow breaks.
- - name: Checkout
- uses: actions/checkout@v3
+ - name: Checkout
+ uses: actions/checkout@v4
- - name: Setup .NET Core 3.1
- uses: actions/setup-dotnet@v1
+ - name: Setup .NET Core 3.1
+ uses: actions/setup-dotnet@v3
- - name: Setup .NET Core 8.0
- uses: actions/setup-dotnet@v1
+ - name: Setup .NET Core 8.0
+ uses: actions/setup-dotnet@v3Also applies to: 20-23, 25-28
🧰 Tools
🪛 actionlint (1.7.7)
16-16: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/dev_actions.yml around lines 15 to 18 and also lines 20 to
23 and 25 to 28, update the GitHub Actions versions to their latest major
releases to avoid using deprecated Node 16 runtime. Change actions/checkout from
v3 to v4 and actions/setup-dotnet from v1 to v3 by modifying the version tags in
the uses fields accordingly.
| - name: Setup MSBuild Path | ||
| uses: microsoft/[email protected] | ||
|
|
||
| - name: Uninstall default WiX Toolset | ||
| run: | | ||
| choco uninstall wixtoolset | ||
| - name: Install specific version of WiX Toolset | ||
| run: | | ||
| choco install wixtoolset --version=3.11.2 | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing -y flag can make WiX installation non-deterministic
Chocolatey prompts for confirmation if the package was never installed on the runner. Add -y to both uninstall and install to ensure non-interactive execution:
- choco uninstall wixtoolset
+ choco uninstall wixtoolset -y
...
- choco install wixtoolset --version=3.11.2
+ choco install wixtoolset --version=3.11.2 -y📝 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.
| - name: Setup MSBuild Path | |
| uses: microsoft/[email protected] | |
| - name: Uninstall default WiX Toolset | |
| run: | | |
| choco uninstall wixtoolset | |
| - name: Install specific version of WiX Toolset | |
| run: | | |
| choco install wixtoolset --version=3.11.2 | |
| - name: Setup MSBuild Path | |
| uses: microsoft/[email protected] | |
| - name: Uninstall default WiX Toolset | |
| run: | | |
| choco uninstall wixtoolset -y | |
| - name: Install specific version of WiX Toolset | |
| run: | | |
| choco install wixtoolset --version=3.11.2 -y |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 32-32: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/dev_actions.yml around lines 30 to 40, the Chocolatey
commands for uninstalling and installing the WiX Toolset are missing the `-y`
flag, which can cause the process to prompt for confirmation and break
automation. Add the `-y` flag to both the `choco uninstall wixtoolset` and
`choco install wixtoolset --version=3.11.2` commands to ensure they run
non-interactively and deterministically.
| UV uv = null; | ||
|
|
||
| if (uv == null && currentFace is CylindricalFace cf) | ||
| { | ||
| uv = CylindricalUv.CalculateUv(cf, vertex); | ||
| } | ||
|
|
||
| if (uv == null && currentFace != null) | ||
| { | ||
| uv = currentFace.Project(vertex)?.UVPoint; | ||
| } | ||
|
|
||
| if (uv == null && currentFace is PlanarFace pf) | ||
| { | ||
| uv = PlanarUv.CalculateUv(pf, vertex); | ||
| } | ||
|
|
||
| if (uv == null) | ||
| { | ||
| uv = new UV(vertex.X, | ||
| vertex.Y); | ||
| } | ||
|
|
||
| geomItem.Uvs.Add(uv); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reorganize UV calculation logic for better efficiency.
The current order of UV calculation attempts is inefficient. For a PlanarFace, the code will first attempt cylindrical UV calculation (which will fail), then face projection, and only then try the appropriate planar UV calculation.
Consider reordering the logic to check face type first:
UV uv = null;
- if (uv == null && currentFace is CylindricalFace cf)
- {
- uv = CylindricalUv.CalculateUv(cf, vertex);
- }
-
- if (uv == null && currentFace != null)
- {
- uv = currentFace.Project(vertex)?.UVPoint;
- }
-
- if (uv == null && currentFace is PlanarFace pf)
- {
- uv = PlanarUv.CalculateUv(pf, vertex);
- }
+ if (currentFace is CylindricalFace cf)
+ {
+ uv = CylindricalUv.CalculateUv(cf, vertex);
+ }
+ else if (currentFace is PlanarFace pf)
+ {
+ uv = PlanarUv.CalculateUv(pf, vertex);
+ }
+ else if (currentFace != null)
+ {
+ uv = currentFace.Project(vertex)?.UVPoint;
+ }
if (uv == null)
{
uv = new UV(vertex.X, vertex.Y);
}🤖 Prompt for AI Agents
In Common_glTF_Exporter/UVs/VertexUvs.cs between lines 16 and 40, the UV
calculation logic tries cylindrical UV calculation before checking if the face
is planar, causing unnecessary attempts. Reorder the conditions to first check
the face type: if it's CylindricalFace, calculate cylindrical UV; else if it's
PlanarFace, calculate planar UV; otherwise, try projecting the face; finally, if
all fail, create a default UV from vertex coordinates. This will improve
efficiency by avoiding redundant checks.
| private static float SrgbToLinear(float c) | ||
| { | ||
| return (c <= 0.04045f) | ||
| ? c / 12.92f | ||
| : Pow((c + 0.055f) / 1.055f, 2.4); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid code duplication by reusing existing method.
The SrgbToLinear method is already implemented in MaterialProperties.cs. Consider reusing it to maintain DRY principles.
Remove this method and instead call MaterialProperties.SrgbToLinear in lines 44-46 and 71-73.
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/BitmapsUtils.cs around lines 105 to 110, the
SrgbToLinear method duplicates functionality already implemented in
MaterialProperties.cs. Remove this local SrgbToLinear method and replace its
calls at lines 44-46 and 71-73 with calls to MaterialProperties.SrgbToLinear to
avoid code duplication and adhere to DRY principles.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores