microsoft / MixedReality-GraphicsTools-Unity

Graphics tools and components for developing Mixed Reality applications in Unity.
MIT License
178 stars 41 forks source link

Removing a mesh from a ClippingPrimitive resets all the properties from the material #202

Open Maesla opened 8 months ago

Maesla commented 8 months ago

Describe the bug

Removing a mesh from a ClippingPrimitive resets all the properties from the material. It clears the properties that the clipping system has set, but the issue is that it also clears all the properties that other systems have set to the material using a MaterialPropertyBlock

To reproduce

  1. Set a color to a material using a MaterialPropertyBlock
  2. Add the render to a clipping primitive
  3. Remove the render from the clipping primitive
  4. Check that the color set in step 1 is lost

You can use this code to reproduce it

using Microsoft.MixedReality.GraphicsTools;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using UnityEngine;

public class RemoveFromClippingPrimitiveBugTest: MonoBehaviour
{
    public bool randomColor = false;
    public Color color = Color.yellow;
    public Material material;

    public List<MeshRenderer> renderers;

    private MaterialPropertyBlock propBlock;

    public ClippingPrimitive clippingPrimitive;

    public float waitTime = 1f;

    public void OnEnable()
    {
        renderers = this.GetComponentsInChildren<MeshRenderer>().ToList();

        foreach (var renderer in renderers)
        {
            renderer.sharedMaterial = material;
        }

        propBlock = new MaterialPropertyBlock();
    }

    public IEnumerator Start()
    {
        clippingPrimitive.gameObject.SetActive(false);
        yield return new WaitForSeconds(waitTime);
        SetColor(); //Use property block to set color
        yield return new WaitForSeconds(waitTime);
        AddToClippingPrimitive(); //Add all renderers to clipping primitive
        yield return new WaitForSeconds(waitTime);
        RemoveFromClippingPrimitive(); //Remove all renderers to clipping primitive. See how color is lost
    }

    private void SetColor()
    {
        foreach (var renderer in renderers)
        {
            Color c = randomColor ? Random.ColorHSV() : color;
            renderer.GetPropertyBlock(propBlock);
            propBlock.SetColor("_Color", c);
            renderer.SetPropertyBlock(propBlock);
        }
    }

    private void AddToClippingPrimitive()
    {
        clippingPrimitive.gameObject.SetActive(true);

        foreach (var renderer in renderers)
        {
            clippingPrimitive.AddRenderer(renderer);
        }
    }

    private void RemoveFromClippingPrimitive()
    {
        clippingPrimitive.gameObject.SetActive(false);

        foreach (var renderer in renderers)
        {
            clippingPrimitive.RemoveRenderer(renderer);
        }
    }

    public void Update()
    {
        if(Input.GetKeyDown(KeyCode.Space))
        {
            SetColor();
        }

        if (Input.GetKeyDown(KeyCode.C))
        {
            ClearPropertyBlock();
        }

    }

    private void ClearPropertyBlock()
    {
        foreach (var renderer in renderers)
        {
            propBlock.Clear();
            renderer.SetPropertyBlock(propBlock);
        }
    }
}

This is happing because ClippingPrimitive.RemoveRenderer calls to ClippingPrimitive.ResetRenderer that has this call

materialPropertyBlock.Clear();
_renderer.SetPropertyBlock(materialPropertyBlock);

https://github.com/microsoft/MixedReality-GraphicsTools-Unity/blob/9a2f1242ce09fb1c28dba898f51e436611959ac8/com.microsoft.mrtk.graphicstools.unity/Runtime/Clipping/ClippingPrimitive.cs#L187

Expected behavior

The color (or any other property that is used outside the clipping system) remains. This was the behavior in MRTK 2.x

Screenshots

https://github.com/microsoft/MixedReality-GraphicsTools-Unity/assets/25863696/7ce4dc7b-8c4a-4ea7-9183-59003df15d02

Your setup (please complete the following information)

Target platform (please complete the following information)

Additional context

The used shader is Graphics Tool/Standard

Cameron-Micka commented 8 months ago

Thank you for the very detailed bug report @Maesla, it's appreciated, and we will follow up!