Skip to content

Fix glTF2 occlusion strength import/export and normal scale export#6602

Merged
kimkulling merged 2 commits intoassimp:masterfrom
Valakor:fix-gltf2-scale-strength
Apr 23, 2026
Merged

Fix glTF2 occlusion strength import/export and normal scale export#6602
kimkulling merged 2 commits intoassimp:masterfrom
Valakor:fix-gltf2-scale-strength

Conversation

@Valakor
Copy link
Copy Markdown
Contributor

@Valakor Valakor commented Apr 20, 2026

When trying to use assimp to import glTF2 files I ran into some issues with a handful of glTF-specific property keys:

  • Occlusion strength was being imported and exported with the custom property key "$tex.file.strength" rather than using the GLTF-specific key AI_MATKEY_GLTF_TEXTURE_STRENGTH defined as "$tex.strength" that already existed making it confusing and difficult to access this material property.
  • Normal scale was being imported as the existing GLTF-specific key AI_MATKEY_GLTF_TEXTURE_SCALE defined as "$tex.scale" but exported by looking for the custom property key "$tex.file.scale" meaning round-trip import -> export would not retain this property.

Summary by CodeRabbit

  • Refactor
    • Optimized glTF2 texture property handling in export and import operations. Texture scale and strength values are now accessed more efficiently, improving the overall robustness of glTF2 asset processing.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 2026

📝 Walkthrough

Walkthrough

The changes refactor glTF2 importer and exporter modules to replace string-based texture property key construction with macro-based direct property access. A deprecated GetMatTexProp helper method is removed, and all call sites are updated to use AI_MATKEY_GLTF_TEXTURE_SCALE and AI_MATKEY_GLTF_TEXTURE_STRENGTH macros.

Changes

Cohort / File(s) Summary
glTF2 Property Key Refactoring
code/AssetLib/glTF2/glTF2Exporter.h, code/AssetLib/glTF2/glTF2Exporter.cpp, code/AssetLib/glTF2/glTF2Importer.cpp
Removed deprecated GetMatTexProp float overload and replaced string-based property key construction with macro-based constants (AI_MATKEY_GLTF_TEXTURE_SCALE, AI_MATKEY_GLTF_TEXTURE_STRENGTH) across texture property access in both importer and exporter.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A cleaner path emerges bright,
Where strings dissolve to macros light,
No more keys built by hand each day,
Constants lead the glTF way! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main changes: fixing glTF2 occlusion strength import/export and normal scale export, which aligns with the PR's core objective of correcting mismatched material property keys.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
code/AssetLib/glTF2/glTF2Exporter.cpp (1)

553-557: Remove the unused GetMatTexProp declaration and definition.

The header still contains the declaration at glTF2Exporter.h:112, while the definition at glTF2Exporter.cpp:553–556 has no active call sites (only commented-out references at lines 648 and 659 remain). Clean up by removing:

  • The declaration from the header
  • The definition from the .cpp file
  • The commented-out calls at lines 648 and 659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/AssetLib/glTF2/glTF2Exporter.cpp` around lines 553 - 557, Remove the
unused GetMatTexProp API: delete its declaration from glTF2Exporter.h and remove
its definition (glTF2Exporter::GetMatTexProp) from glTF2Exporter.cpp, and also
remove the remaining commented-out references to GetMatTexProp where they appear
in the file so there are no stale mentions; ensure no other code references the
symbol before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@code/AssetLib/glTF2/glTF2Exporter.cpp`:
- Around line 553-557: Remove the unused GetMatTexProp API: delete its
declaration from glTF2Exporter.h and remove its definition
(glTF2Exporter::GetMatTexProp) from glTF2Exporter.cpp, and also remove the
remaining commented-out references to GetMatTexProp where they appear in the
file so there are no stale mentions; ensure no other code references the symbol
before committing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7a52b11c-8d00-447a-98fb-a35e4a7cd54b

📥 Commits

Reviewing files that changed from the base of the PR and between 17c12da and 7ef2e80.

📒 Files selected for processing (3)
  • code/AssetLib/glTF2/glTF2Exporter.cpp
  • code/AssetLib/glTF2/glTF2Exporter.h
  • code/AssetLib/glTF2/glTF2Importer.cpp
💤 Files with no reviewable changes (1)
  • code/AssetLib/glTF2/glTF2Exporter.h

@kimkulling kimkulling merged commit 25a1318 into assimp:master Apr 23, 2026
13 checks passed
@kimkulling
Copy link
Copy Markdown
Member

Merged, thanks a lot for your contribution.

@kimkulling kimkulling moved this from 👀 In review to ✅ Done in The Asset-Importer-Lib DevOps Board Apr 23, 2026
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants