Skip to content

Conversation

@dsarno
Copy link
Owner

@dsarno dsarno commented Jan 1, 2026

Motivation

  • Ensure that when creating a new material the initial color is not redundantly applied if properties already specifies a color key.
  • Let the properties payload remain authoritative so MaterialOps.ApplyProperties does not get overwritten by the initial color set.
  • Reduce unnecessary work and avoid potential confusion when a custom colorProperty (e.g. "_TintColor") is provided in properties.

Description

  • Modified the condition in MCPForUnity/Editor/Tools/ManageMaterial.cs to also check for the presence of the provided colorProperty in properties before applying the initial color.
  • The new check requires that properties does not contain _BaseColor, _Color, or the colorProperty before parsing and setting the initial color.
  • All existing creation, asset persistence, and cleanup logic (including the try/finally destruction guard) remain unchanged.

Testing

  • No automated tests were executed for this change.
  • Manual inspection and compilation were used to validate the small conditional change.

Codex Task

@coderabbitai
Copy link

coderabbitai bot commented Jan 1, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

@dsarno dsarno merged commit e4dc7c6 into feature/gameobject-enhancements Jan 1, 2026
3 checks passed
@greptile-apps
Copy link

greptile-apps bot commented Jan 1, 2026

Greptile Summary

Enhanced the material creation logic to avoid redundant color assignment when properties already contains a custom colorProperty.

  • Modified the conditional check in CreateMaterial method to verify that properties does not contain the custom colorProperty before applying the initial color parameter
  • The check now ensures that properties takes precedence over the color parameter for _BaseColor, _Color, and any custom colorProperty
  • Prevents unnecessary color assignments and ensures MaterialOps.ApplyProperties remains authoritative
  • All existing creation, asset persistence, and cleanup logic remain unchanged

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is a small, targeted conditional enhancement that maintains backward compatibility. The logic correctly extends the existing check to include the custom colorProperty, preventing redundant work while preserving existing behavior. The try/finally cleanup pattern remains intact, and no breaking changes are introduced.
  • No files require special attention

Important Files Changed

Filename Overview
MCPForUnity/Editor/Tools/ManageMaterial.cs Added check to prevent redundant color assignment when properties already contains the custom colorProperty

Sequence Diagram

sequenceDiagram
    participant Client
    participant CreateMaterial
    participant Material
    participant MaterialOps
    
    Client->>CreateMaterial: create(materialPath, shader, color, colorProperty, properties)
    CreateMaterial->>Material: new Material(shader)
    
    alt colorToken != null
        alt properties == null OR (!contains _BaseColor AND !contains _Color AND !contains colorProperty)
            Note over CreateMaterial: Check if colorProperty is absent from properties
            CreateMaterial->>CreateMaterial: ParseColor(colorToken)
            alt colorProperty specified & material has it
                CreateMaterial->>Material: SetColor(colorProperty, color)
            else material has _BaseColor
                CreateMaterial->>Material: SetColor(_BaseColor, color)
            else material has _Color
                CreateMaterial->>Material: SetColor(_Color, color)
            end
        else properties contains color property
            Note over CreateMaterial: Skip initial color - let properties win
        end
    end
    
    CreateMaterial->>Material: CreateAsset(material, materialPath)
    
    alt properties != null
        CreateMaterial->>MaterialOps: ApplyProperties(material, properties)
        Note over MaterialOps: Applies all properties including<br/>custom color if specified
    end
    
    CreateMaterial->>Material: SetDirty() & SaveAssets()
    CreateMaterial-->>Client: success
Loading

@dsarno dsarno deleted the codex/fix-memory-leak-in-managematerial branch January 2, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants