Skip to content

Conversation

@vnoves
Copy link
Member

@vnoves vnoves commented Oct 8, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability by safely handling missing asset properties to prevent potential crashes during export.
    • Ensured deterministic behavior by only selecting a linked asset when there is exactly one connection, avoiding ambiguous or incorrect exports when multiple links exist.
    • Increased reliability of material/asset linkage in glTF exports, reducing unexpected outcomes in edge cases.

@coderabbitai
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

Replaced a permissive connected-properties check with explicit null handling and a strict equality check for exactly one connection. Adds an early null return when the asset property is missing and only returns a connected asset when there is exactly one connected property; otherwise returns null. Minor formatting changes.

Changes

Cohort / File(s) Summary
Asset property connection check tightening
Common_glTF_Exporter/Materials/AssetProperties.cs
Adjusted control flow: explicit null check for the asset property, require NumberOfConnectedProperties == 1 to return the connected asset; otherwise return null. Minor spacing/formatting updates.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant AP as AssetProperties
  Note over AP: Get connected asset from property

  Caller->>AP: GetConnectedAsset(prop)
  alt prop is null
    AP-->>Caller: null
  else prop.NumberOfConnectedProperties == 1
    AP->>AP: Retrieve connected asset
    AP-->>Caller: ConnectedAsset
  else
    AP-->>Caller: null
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at stricter trails,
One path, not many, the logic prevails.
If null winds blow, I pause my run—
Only hop through when there’s exactly one.
In codey clover, I prune the sprawl,
A careful bunny, returning null or all. 🐇✨

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ 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 “Fix Connected Asset” directly reflects the main change of correcting the connected asset handling logic to require exactly one connection and explicit null checking, making it concise, clear, and focused on the core fix.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch FixDoubleConnectedAsset

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d9c2e6 and dd28daa.

📒 Files selected for processing (1)
  • Common_glTF_Exporter/Materials/AssetProperties.cs (1 hunks)
⏰ 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 (2)
Common_glTF_Exporter/Materials/AssetProperties.cs (2)

34-35: LGTM! Explicit null check improves clarity.

The explicit null check with early return makes the code's intent clearer and follows defensive programming practices.


37-42: Downstream null handling is safe – GetDiffuseBitmap now returns null for zero or multiple connections, GetTexturePath returns null, and MaterialTextures only calls SetTextureProperties when texturePath is non‐empty, so no NRE or broken behavior.


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.

@vnoves vnoves merged commit b80841a into develop Oct 8, 2025
2 checks passed
@vnoves vnoves deleted the FixDoubleConnectedAsset branch October 28, 2025 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants