Conversation
WalkthroughA new CleanGamma method is added to BitmapsUtils to re-encode images to clean sRGB format by removing gamma and ICC profile metadata. ExportImageBuffer is modified to use this new method when processing embedded textures, applying gamma correction before blending operations. Changes
Sequence Diagram(s)sequenceDiagram
participant EB as ExportImageBuffer
participant BU as BitmapsUtils
participant IM as Image Processing
EB->>BU: GetMimeType(filePath)
BU-->>EB: mimeType
rect rgb(200, 220, 240)
Note over EB,IM: NEW: Gamma Cleaning Path
EB->>BU: CleanGamma(filePath, format)
BU->>IM: Load source image
BU->>IM: Create 24bpp RGB target
BU->>IM: Render source → target
BU->>IM: Save to memory stream
IM-->>BU: cleaned imageBytes
BU-->>EB: imageBytes (sRGB, no gamma)
end
EB->>EB: BlendImageWithColor (on cleaned bytes)
EB->>EB: Setup buffer/view
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (1)
39-53: Consider adding error handling for I/O operations.File and image operations can fail for various reasons (file not found, corrupted image, insufficient permissions). Consider wrapping the operations in a try-catch block or allowing exceptions to propagate with meaningful context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Common_glTF_Exporter/Materials/BitmapsUtils.cs(1 hunks)Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
Common_glTF_Exporter/Materials/BitmapsUtils.cs (3)
ImageFormat(14-31)BitmapsUtils(12-164)CleanGamma(39-53)
🔇 Additional comments (1)
Common_glTF_Exporter/Utils/GLTFBinaryDataUtils.cs (1)
214-218: Verify that unconditional gamma cleaning is appropriate for all embedded textures.The change now applies gamma cleaning to all embedded textures. This adds processing overhead (decode → create new bitmap → draw → re-encode) compared to directly reading file bytes. While this ensures glTF compliance by removing custom gamma/ICC profiles, consider whether:
- All textures need this processing, or only those with non-standard gamma/ICC profiles
- The performance impact is acceptable for your use case (especially with many/large textures)
- This aligns with the intended behavior when textures are already in sRGB
The logic correctly integrates the new
CleanGammamethod and the approach of cleaning gamma before blending makes sense for glTF compliance.
| /// by re-encoding it to a clean sRGB version in memory. | ||
| /// </summary> | ||
| /// <param name="path">Path to the source image</param> | ||
| /// <returns>Byte array of the cleaned image (PNG)</returns> |
There was a problem hiding this comment.
Update documentation to reflect parameterized format.
The documentation states "Byte array of the cleaned image (PNG)" but the method returns the format specified by the imageFormat parameter, which could be JPEG, BMP, or GIF.
Apply this diff:
- /// <returns>Byte array of the cleaned image (PNG)</returns>
+ /// <returns>Byte array of the cleaned image in the specified format</returns>📝 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.
| /// <returns>Byte array of the cleaned image (PNG)</returns> | |
| /// <returns>Byte array of the cleaned image in the specified format</returns> |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/BitmapsUtils.cs around line 38, the XML
summary/returns documentation incorrectly states the method always returns a PNG
byte array; update the XML comments to reflect that the returned byte array
format is determined by the imageFormat parameter (add/adjust a <param
name="imageFormat"> describing accepted formats and modify the <returns> text to
say it returns a byte array in the specified image format such as PNG, JPEG,
BMP, or GIF). Ensure the doc mentions the imageFormat parameter and that the
return value corresponds to that format.
| public static byte[] CleanGamma(string path, ImageFormat imageFormat) | ||
| { | ||
| using (var original = new Bitmap(path)) | ||
| using (var ms = new MemoryStream()) | ||
| { | ||
| // Convert to standard sRGB (System.Drawing assumes sRGB by default) | ||
| using (var converted = new Bitmap(original.Width, original.Height, PixelFormat.Format24bppRgb)) | ||
| using (var g = Graphics.FromImage(converted)) | ||
| { | ||
| g.DrawImage(original, 0, 0, original.Width, original.Height); | ||
| converted.Save(ms, imageFormat); | ||
| } | ||
| return ms.ToArray(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Alpha channel is lost when using Format24bppRgb.
The method uses PixelFormat.Format24bppRgb which strips the alpha channel from images with transparency. This will break any textures that require transparency (e.g., cutout materials, decals). glTF fully supports texture transparency, so this data should be preserved.
Apply this diff to preserve alpha:
- using (var converted = new Bitmap(original.Width, original.Height, PixelFormat.Format24bppRgb))
+ using (var converted = new Bitmap(original.Width, original.Height, PixelFormat.Format32bppArgb))📝 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 byte[] CleanGamma(string path, ImageFormat imageFormat) | |
| { | |
| using (var original = new Bitmap(path)) | |
| using (var ms = new MemoryStream()) | |
| { | |
| // Convert to standard sRGB (System.Drawing assumes sRGB by default) | |
| using (var converted = new Bitmap(original.Width, original.Height, PixelFormat.Format24bppRgb)) | |
| using (var g = Graphics.FromImage(converted)) | |
| { | |
| g.DrawImage(original, 0, 0, original.Width, original.Height); | |
| converted.Save(ms, imageFormat); | |
| } | |
| return ms.ToArray(); | |
| } | |
| } | |
| public static byte[] CleanGamma(string path, ImageFormat imageFormat) | |
| { | |
| using (var original = new Bitmap(path)) | |
| using (var ms = new MemoryStream()) | |
| { | |
| // Convert to standard sRGB (System.Drawing assumes sRGB by default) | |
| using (var converted = new Bitmap(original.Width, original.Height, PixelFormat.Format32bppArgb)) | |
| using (var g = Graphics.FromImage(converted)) | |
| { | |
| g.DrawImage(original, 0, 0, original.Width, original.Height); | |
| converted.Save(ms, imageFormat); | |
| } | |
| return ms.ToArray(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Materials/BitmapsUtils.cs around lines 39 to 53, the
current conversion uses PixelFormat.Format24bppRgb which discards the alpha
channel; change the conversion to a 32-bit format that preserves alpha (e.g.,
PixelFormat.Format32bppArgb) and draw the source into that bitmap while ensuring
the Graphics compositing mode preserves alpha (SourceOver/SourceCopy as
appropriate); then save to the MemoryStream with an image format/encoder that
supports alpha (e.g., PNG) or fall back to the provided imageFormat only if it
supports transparency, so transparent pixels are retained for glTF textures.
1 participant
Summary by CodeRabbit