Flipped textures #116
Flipped textures #116
Conversation
WalkthroughThis update enhances the texture handling pipeline in the glTF exporter. It introduces new utilities for reading texture search paths from both default Autodesk locations and user-specific Revit.ini settings. Texture path resolution, UV coordinate handling, and texture offset application are refined, with new logic for tint color and offset/scale mapping in GLTF materials. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant TextureLocation
participant RevitIniReader
participant AssetProperties
participant MaterialTextures
User->>MainWindow: Launches exporter
MainWindow->>TextureLocation: GetPaths()
TextureLocation->>RevitIniReader: GetAdditionalRenderAppearancePaths()
RevitIniReader-->>TextureLocation: List of user-defined paths
TextureLocation-->>MainWindow: Aggregated texture paths
MainWindow->>AssetProperties: GetTexturePath(bitmapPath)
AssetProperties->>MainWindow: Uses TexturePaths to resolve file
MainWindow->>MaterialTextures: SetMaterialTextures(...)
MaterialTextures->>AssetProperties: GetOffset(...)
MaterialTextures->>GLTF: Apply offset/scale/tint to texture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation - BetaCodeRabbit's unit test generation is now available in Beta! Automatically generate comprehensive unit tests for your code changes, ensuring better test coverage and catching edge cases you might miss. Our AI analyzes your code structure and creates tests that follow best practices and your project's testing patterns. Learn more here, or just try it under ✨ Finishing Touches. 📜 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: 2
🧹 Nitpick comments (2)
Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
82-87: Consider code deduplication for SrgbToLinear method.The
SrgbToLinearmethod appears to be duplicated - there's a similar implementation inBitmapsUtils.cs(lines 128-133). Consider consolidating these into a shared utility to follow DRY principles.Extract to a common color utility class:
+public static class ColorUtils +{ + public static float SrgbToLinear(float srgb) + { + return srgb <= 0.04045f + ? srgb / 12.92f + : (float)Math.Pow((srgb + 0.055f) / 1.055f, 2.4); + } +}Common_glTF_Exporter/Utils/RevitIniReader.cs (1)
27-61: Consider adding exception handling for file operations.The file I/O operations could potentially throw exceptions (IOException, UnauthorizedAccessException, etc.). Consider wrapping the file operations in a try-catch block to ensure graceful handling of file system errors.
+try +{ if (!File.Exists(iniPath)) return null; foreach (var line in File.ReadAllLines(iniPath)) { // ... existing parsing logic ... } +} +catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException || ex is SecurityException) +{ + // Log error if logging is available, then return null + return null; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Common_glTF_Exporter/Common_glTF_Exporter.projitems(2 hunks)Common_glTF_Exporter/Core/GlTFExportContext.cs(1 hunks)Common_glTF_Exporter/Materials/AssetProperties.cs(4 hunks)Common_glTF_Exporter/Materials/BitmapsUtils.cs(1 hunks)Common_glTF_Exporter/Materials/MaterialProperties.cs(2 hunks)Common_glTF_Exporter/Materials/MaterialTextures.cs(3 hunks)Common_glTF_Exporter/Materials/TextureLocation.cs(1 hunks)Common_glTF_Exporter/Service/RevitCollectorService.cs(2 hunks)Common_glTF_Exporter/Utils/RevitIniReader.cs(1 hunks)Common_glTF_Exporter/Windows/MainWindow.xaml.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (1)
SrgbToLinear(129-134)
Common_glTF_Exporter/Materials/TextureLocation.cs (1)
Common_glTF_Exporter/Utils/RevitIniReader.cs (2)
List(13-62)RevitIniReader(11-63)
Common_glTF_Exporter/Materials/AssetProperties.cs (1)
Common_glTF_Exporter/Windows/MainWindow.xaml.cs (2)
MainWindow(27-171)MainWindow(32-52)
Common_glTF_Exporter/Materials/MaterialTextures.cs (2)
Common_glTF_Exporter/Materials/AssetProperties.cs (4)
AssetPropertiesUtils(10-182)GetOffset(122-141)GetRotationRadians(89-100)GetFade(171-181)Common_glTF_Exporter/Core/GLTFTextureInfo.cs (3)
GLTFTextureInfo(16-23)GLTFTextureExtensions(33-37)GLTFTextureTransform(25-31)
🔇 Additional comments (16)
Common_glTF_Exporter/Common_glTF_Exporter.projitems (2)
44-44: LGTM - New texture location utilities added.The addition of
Materials\TextureLocation.cssupports the enhanced texture path resolution functionality described in the PR objectives.
72-72: LGTM - Revit.ini reader utility added.The addition of
Utils\RevitIniReader.csenables reading texture search paths from Revit configuration files, enhancing the texture resolution capabilities.Common_glTF_Exporter/Core/GlTFExportContext.cs (1)
319-320: LGTF - UV coordinate flipping correctly implemented.The UV coordinate flipping by inverting the V component (
1.0 - uv.V) is the correct approach to address texture orientation issues. This ensures textures are properly oriented when exported to glTF format, which aligns with the PR objective of fixing flipped textures.Common_glTF_Exporter/Service/RevitCollectorService.cs (2)
1-1: LGTM - Required using directive added.The
using Autodesk.Revit.ApplicationServices;directive is correctly added to support the newGetApplication()method.
28-31: LGTM - Application accessor method properly implemented.The
GetApplication()method follows the same pattern as the existingGetDocument()method and correctly provides access to the Revit Application object, which supports the enhanced texture path resolution functionality.Common_glTF_Exporter/Windows/MainWindow.xaml.cs (3)
1-20: LGTM - Using directives properly organized.The reorganization and addition of using directives support the new texture handling functionality.
30-30: LGTM - Static texture paths property correctly added.The static
TexturePathsproperty provides global access to texture search paths, which aligns with the enhanced texture resolution functionality described in the PR.
49-49: Double-check TextureLocation.GetPaths implementationThe assignment in
MainWindow.xaml.csrelies onTextureLocation.GetPaths(). Please confirm inCommon_glTF_Exporter/Materials/TextureLocation.csthat:
- A public static
GetPaths()method is declared.- Its return type (e.g.
IEnumerable<string>orList<string>) matches whatTexturePathsexpects.- It correctly enumerates all texture locations and handles edge cases (empty folders, missing files).
- There are unit tests covering normal and boundary scenarios for this method.
Common_glTF_Exporter/Materials/TextureLocation.cs (1)
8-30: LGTM! Clean and well-structured texture path aggregation.The implementation correctly combines default Autodesk texture paths with user-configured paths from Revit.ini. The null-safety check for
externalPathsis appropriate.Common_glTF_Exporter/Materials/MaterialProperties.cs (1)
31-65: Tint color application logic is correct.The implementation correctly applies tint color multiplication in sRGB space before converting to linear space, which is the mathematically appropriate approach.
Common_glTF_Exporter/Materials/AssetProperties.cs (2)
45-63: Improved texture path resolution with proper fallback logic.The enhanced path resolution correctly handles both absolute and relative paths, searching through multiple base directories when needed.
122-141: GetOffset method implementation follows established patterns.The method correctly handles unit conversion and follows the same conditional compilation pattern used in
GetScale, ensuring consistency across different Revit API versions.Common_glTF_Exporter/Materials/MaterialTextures.cs (3)
42-42: Good refactoring: Moving tint color assignment earlier.Moving the tint color assignment to immediately after texture path retrieval is logical since it's independent of texture file existence.
59-60: Offset retrieval properly integrated.The offset values are correctly retrieved using the new
GetOffsetmethod, maintaining consistency with the existing scale retrieval pattern.
67-71: Verify the texture offset calculation math.The offset calculation involves coordinate space conversion between Revit and glTF. Please verify that the formulas are mathematically correct:
- X offset:
-(offsetX / scaleX)- Y offset:
offsetY / scaleY - gltfScale[1]The Y offset calculation subtracts the Y scale component, which suggests a specific coordinate system transformation.
Could you confirm this offset calculation handles the "flipped textures" issue mentioned in the PR title correctly? The coordinate system conversion logic should be validated against expected texture positioning.
Common_glTF_Exporter/Utils/RevitIniReader.cs (1)
13-62: Well-implemented INI file parsing with proper path normalization.The method correctly constructs the Revit.ini path, parses the configuration, and normalizes both absolute and relative paths. The logic for resolving relative paths against the user profile directory is appropriate.
|
|
||
| return texturePath; | ||
| // Otherwise, search each base path | ||
| foreach (string basePath in MainWindow.TexturePaths) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive null check for MainWindow.TexturePaths.
Consider adding a null check for MainWindow.TexturePaths to prevent potential NullReferenceException if the static property isn't properly initialized.
// Otherwise, search each base path
+if (MainWindow.TexturePaths != null)
+{
foreach (string basePath in MainWindow.TexturePaths)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/AssetProperties.cs at line 54, add a null
check before iterating over MainWindow.TexturePaths to prevent a
NullReferenceException. Modify the code to verify that MainWindow.TexturePaths
is not null before the foreach loop, and only proceed with the iteration if it
is initialized.
| using System.Drawing; | ||
| using System.Drawing.Imaging; | ||
| using System.IO; | ||
| using System.Linq; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the necessity of the System.Linq using directive.
The using System.Linq; directive has been added, but the current code in this file doesn't appear to use any LINQ operations. Ensure this directive is actually needed or remove it if it's not being used.
🏁 Script executed:
#!/bin/bash
# Description: Check if LINQ operations are used in BitmapsUtils.cs
# Expected: Find LINQ method calls or confirm the directive is unnecessary
# Search for common LINQ methods in the file
ast-grep --pattern 'Where($$$)' Common_glTF_Exporter/Materials/BitmapsUtils.cs
ast-grep --pattern 'Select($$$)' Common_glTF_Exporter/Materials/BitmapsUtils.cs
ast-grep --pattern 'Any($$$)' Common_glTF_Exporter/Materials/BitmapsUtils.cs
ast-grep --pattern 'First($$$)' Common_glTF_Exporter/Materials/BitmapsUtils.cs
rg -n '\.(Where|Select|Any|First|Last|Count|Sum|Min|Max|OrderBy|GroupBy)\(' Common_glTF_Exporter/Materials/BitmapsUtils.csLength of output: 627
Remove the unused System.Linq directive
The using System.Linq; on line 6 of Common_glTF_Exporter/Materials/BitmapsUtils.cs isn’t needed—no LINQ methods are used in this file. Please remove it:
- using System.Linq;📝 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.
| using System.Linq; |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/BitmapsUtils.cs at line 6, remove the unused
using directive 'using System.Linq;' since no LINQ methods are utilized in this
file.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores