SOHNE / Colorblindness

Post-processing effects manager for Unity3D (URP & HDRP) in order to simulate different color palettes for color blind
MIT License
33 stars 7 forks source link

Code suggestion #2

Open Stader opened 2 years ago

Stader commented 2 years ago

To increase customization, I'd like to suggest a change to the currentType:

public int currentType
        {
            get => _currentType;

            set
            {
                if (value > maxType) _currentType = 0;
                else if(value < 0)_currentType = maxType;
                else _currentType = value;
            }
        }

So we can go back and forth in the array.

I also incremented a change in InitChange and created its variation to return.

public void InitChangeAdd()
        {
            if (volumes == null) return;

            currentType++;

            PlayerPrefs.SetInt("Accessibility.ColorblindType", currentType);

            StartCoroutine(ApplyFilter());

#if UNITY_EDITOR
            // TODO: Use a public event system to announce the change of the activated filter
            Debug.Log($"Color changed to <b>{(ColorblindTypes)currentType} {currentType}</b>/{maxType}");
#endif
        }

        public void InitChangeRemove()
        {
            if (volumes == null) return;

            currentType--;

            PlayerPrefs.SetInt("Accessibility.ColorblindType", currentType);

            StartCoroutine(ApplyFilter());

#if UNITY_EDITOR
            // TODO: Use a public event system to announce the change of the activated filter
            Debug.Log($"Color changed to <b>{(ColorblindTypes)currentType} {currentType}</b>/{maxType}");
#endif
        }

There is a way to simplify this in just one method, but for a while I ended up not doing it.

Thank you for developing this solution for color blindness!

zschzen commented 2 years ago

I'm really happy with your suggestion, and it's good to know that a project done quickly for a college project caught your eye!

I will now do the steps to implement these fixes and other changes. As I had done in a hurry, a lot of things had to be revised.

For example, I believe that it might be feasible to use a single volume with priority attached and manipulate only it, without having to go through the entire list of the scene looking for any and all volume to manipulate (?!).

Stader commented 2 years ago

It's a good idea, it would perform better using a single volume. I encourage you to continue this project, at least it is one of the few compatible with the new Unity versions with the URP and HDRP pipelines. You could even put it in the asset store.