Add HSL color picker UI component for intuitive color selection#25
Add HSL color picker UI component for intuitive color selection#25LuticaCANARD wants to merge 6 commits intomainfrom
Conversation
- 색상환(Hue ring) + 채도/명도 삼각형(SL triangle)으로 구성된 HSLColorPicker 정적 유틸리티 클래스를 추가하여 마우스 드래그로 색상을 직관적으로 선택할 수 있게 함. - ColorCorrection 컨텐츠의 Lift/Gamma/Gain, Shadow/Highlight Tone에 HSL 픽커 버튼을 추가해 선택된 속성을 픽커로 조정 가능. - SplitToning 모드의 잘못된 _ShadowColor/_HighlightColor 속성을 실제 셰이더 속성인 _ShadowsTone/_HighlightsTone 으로 수정. - ImageFilter (HSV) 컨텐츠에 HSL Target Color 섹션을 추가하여 픽커로 선택한 색상을 Convert 또는 Offset으로 적용 가능. - 영문/한국어/일본어 언어 파일에 신규 키 추가. https://claude.ai/code/session_01DVUmzYCzGpteav2NZmWAsm
네임스페이스 명확성을 위해 FilterMode.Bilinear 사용 부분을 UnityEngine.FilterMode.Bilinear로 변경하였습니다. 코드 동작에는 영향이 없으며, 가독성과 유지보수성을 높이기 위한 수정입니다.
색상환 픽커에서 선택한 Hue의 위치를 따라 SL 그라데이션 삼각형의 순색(pure hue) 꼭짓점이 정렬되도록 회전 처리. - 삼각형 텍스처를 GUIUtility.RotateAroundPivot로 hue * 360° 만큼 중심 기준 회전하여 그리도록 수정. - 입력 처리 및 인디케이터 위치 계산용 정점도 hue에 따라 회전된 좌표를 반환하도록 GetTriangleVertices 시그니처에 hue 인자 추가. - 결과적으로 링의 Hue 마커와 삼각형의 순색 꼭짓점이 항상 같은 반경 직선 위에 위치하여 RGB값이 시각적으로 일치. https://claude.ai/code/session_01DVUmzYCzGpteav2NZmWAsm
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom HSL color picker UI component and integrates it into the Color Correction and Image Filter modules, accompanied by updated localization for English, Japanese, and Korean. The review feedback highlights several critical architectural and performance concerns, specifically the use of static fields in the editor utility which may cause memory leaks and multi-window interference. Additionally, the feedback identifies a potential breaking change in shader property names for Split Toning, performance bottlenecks in the triangle texture generation, and opportunities to improve the user experience by allowing independent hue adjustments and refactoring duplicated logic.
| private static Texture2D _hueRingTex; | ||
| private static Texture2D _triangleTex; | ||
| private static float _cachedTriangleHue = -1f; | ||
|
|
||
| private enum DragMode { None, Ring, Triangle } | ||
| private static int _activeControlID = -1; | ||
| private static DragMode _dragMode = DragMode.None; |
There was a problem hiding this comment.
Using static fields for Texture2D and interaction state (_activeControlID, _dragMode) in an Editor utility class can lead to several issues:
- Memory Leaks: Static textures with
HideFlags.HideAndDontSaveare not automatically cleaned up by Unity and will persist in native memory across domain reloads (recompilation), as the static reference is lost but the object remains. - Multi-window Interference: If multiple
TextureCocktailwindows are open with different colors, they will share the same_cachedTriangleHueand textures. This will cause the triangle texture to be regenerated every frame as the windows fight over the cached hue, leading to significant performance degradation. - Interaction Bugs:
GUIUtility.GetControlIDgenerates IDs that are local to the window. If two windows have controls with the same ID, the static_activeControlIDwill cause both windows to respond to drag events simultaneously.
Consider moving these fields to the instance of the window or content class using the picker, or implement a more robust caching mechanism that is instance-aware.
| h = AngleToHue(local); | ||
| // If the current color is desaturated, the hue change would be | ||
| // invisible. Promote to a visible saturated mid-lightness color. | ||
| if (s < 0.05f) { s = 1f; l = 0.5f; } |
There was a problem hiding this comment.
Forcing saturation to 1.0 and lightness to 0.5 when the hue is adjusted on the ring can be disruptive to the user experience. If a user is trying to fine-tune the hue of a desaturated or dark color, this logic will cause the color to jump to a fully saturated version unexpectedly. It is generally better to allow the hue to change independently of saturation and lightness, even if the visual change is subtle.
References
- UI components should avoid unexpected side effects on unrelated properties unless explicitly requested by the user.
| var pixels = new Color32[size * size]; | ||
| for (int y = 0; y < size; y++) | ||
| { | ||
| for (int x = 0; x < size; x++) | ||
| { | ||
| Vector2 p = new Vector2(x + 0.5f, y + 0.5f); | ||
| float a, b, c; | ||
| Barycentric(p, vTop, vLeft, vRight, out a, out b, out c); | ||
| if (a < -0.005f || b < -0.005f || c < -0.005f) | ||
| { | ||
| pixels[y * size + x] = new Color32(0, 0, 0, 0); | ||
| continue; | ||
| } | ||
| a = Mathf.Clamp01(a); | ||
| b = Mathf.Clamp01(b); | ||
| c = Mathf.Clamp01(c); | ||
| float r = a * pure.r + c; | ||
| float g = a * pure.g + c; | ||
| float bl = a * pure.b + c; | ||
| pixels[y * size + x] = new Color(r, g, bl, 1f); | ||
| } | ||
| } |
There was a problem hiding this comment.
The EnsureTriangleTexture method performs expensive barycentric coordinate calculations for every pixel in a 192x192 loop every time the hue changes. Since the triangle's shape and the pixels' positions relative to it are constant, the barycentric coordinates a, b, c for each pixel can be precomputed once. Only the final color interpolation (a * pure + c) needs to happen when the hue changes. This would significantly improve performance during mouse dragging.
| private void ApplyTintToHsvOffset() | ||
| { | ||
| var material = GetMaterial(); | ||
| if (material == null || !material.HasProperty("_hsvOffsetVector")) return; | ||
|
|
||
| float h, s, v; | ||
| Color.RGBToHSV(_tintColor, out h, out s, out v); | ||
| // Treat hue 0 as the neutral identity, so offset is signed in [-0.5, 0.5]. | ||
| float hueOffset = h > 0.5f ? h - 1f : h; | ||
| material.SetVector("_hsvOffsetVector", new Vector4(hueOffset, s - 0.5f, v - 0.5f, 0f)); | ||
| baseWindow.CompileShader(); | ||
| } | ||
|
|
||
| private void ApplyTintToHsvConvert() | ||
| { | ||
| var material = GetMaterial(); | ||
| if (material == null || !material.HasProperty("_hsvConvertVector")) return; | ||
|
|
||
| float h, s, v; | ||
| Color.RGBToHSV(_tintColor, out h, out s, out v); | ||
| float hueOffset = h > 0.5f ? h - 1f : h; | ||
| material.SetVector("_hsvConvertVector", new Vector4(hueOffset, s - 0.5f, v - 0.5f, 0f)); | ||
| baseWindow.CompileShader(); | ||
| } |
There was a problem hiding this comment.
The logic for calculating the HSV vector from the tint color is duplicated in ApplyTintToHsvOffset and ApplyTintToHsvConvert. Consider refactoring this into a helper method to improve maintainability.
private void ApplyTintToHsvOffset()
{
ApplyTintToVector("_hsvOffsetVector");
}
private void ApplyTintToHsvConvert()
{
ApplyTintToVector("_hsvConvertVector");
}
private void ApplyTintToVector(string propertyName)
{
var material = GetMaterial();
if (material == null || !material.HasProperty(propertyName)) return;
float h, s, v;
Color.RGBToHSV(_tintColor, out h, out s, out v);
// Treat hue 0 as the neutral identity, so offset is signed in [-0.5, 0.5].
float hueOffset = h > 0.5f ? h - 1f : h;
material.SetVector(propertyName, new Vector4(hueOffset, s - 0.5f, v - 0.5f, 0f));
baseWindow.CompileShader();
}- 도메인 리로드 시 캐시된 Texture2D 누수를 막기 위해 AssemblyReloadEvents.beforeAssemblyReload 훅으로 정리. - 링 드래그 시 무조건 Saturation/Lightness를 강제하던 로직 제거. 이제 Hue만 독립적으로 변하므로 어두운/회색 색상의 미세 조정이 의도치 않게 채도가 튀는 문제 해소. - 삼각형 텍스처 픽셀별 무게중심좌표(barycentric)는 한 번만 계산하여 캐싱하고, hue 변경 시에는 선형 블렌드만 수행하도록 EnsureTriangleTexture 분리/최적화. - 중복 _activeControlID 상태 제거. GUIUtility.hotControl만으로 드래그 소유권을 판단해 멀티 윈도우 간섭 가능성 축소. - ImageFilter의 Tint→HSV 적용 로직 중복을 ApplyTintToVector(string) 헬퍼로 통합. https://claude.ai/code/session_01DVUmzYCzGpteav2NZmWAsm
There was a problem hiding this comment.
Pull request overview
Adds a new editor-side HSL color picker (hue ring + saturation/lightness triangle) and integrates it into existing color workflows (ColorCorrection + ImageFilter), alongside localization updates.
Changes:
- Introduces
HSLColorPickereditor UI component with procedural textures and HSL↔RGB utilities. - Integrates the picker into
ColorCorrection(Lift/Gamma/Gain + Split Toning colors) and adds a “target color” picker section toImageFilterto apply selected color to HSV vectors. - Adds new localization strings (EN/JA/KR) and removes a few unused fields.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Packages/luticalab.texturecocktail/Editor/UI/HSLColorPicker.cs.meta | Adds Unity meta for the new editor script. |
| Packages/luticalab.texturecocktail/Editor/UI/HSLColorPicker.cs | Implements the HSL picker UI and color conversion helpers. |
| Packages/luticalab.texturecocktail/Editor/UI.meta | Adds Unity folder meta for the new Editor/UI folder. |
| Packages/luticalab.texturecocktail/Editor/TextureCocktail.cs | Removes an unused _shaderChanged field. |
| Packages/luticalab.texturecocktail/Editor/Content/ImageFilter.cs | Adds HSL target color section and “apply to HSV” helpers. |
| Packages/luticalab.texturecocktail/Editor/Content/FastImageConverter.cs | Removes unused foldout state fields. |
| Packages/luticalab.texturecocktail/Editor/Content/ColorCorrection.cs | Replaces several color fields with HSL-picker-enabled rows and adds an active picker panel. |
| Packages/luticalab.core/Languages/English.json | Adds new UI strings for HSL picker + apply buttons. |
| Packages/luticalab.core/Languages/Japanese.json | Adds new UI strings for HSL picker + apply buttons. |
| Packages/luticalab.core/Languages/Korean.json | Adds new UI strings for HSL picker + apply buttons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_dragMode == DragMode.Ring) | ||
| { | ||
| Vector2 local = e.mousePosition - center; | ||
| h = AngleToHue(local); | ||
| color = HSLToRGB(h, s, l); | ||
| GUI.changed = true; |
| Color pure = HSLToRGB(hue, 1f, 0.5f); | ||
| return new Color( | ||
| a * pure.r + c, | ||
| a * pure.g + c, | ||
| a * pure.b + c, | ||
| 1f); |
| h = Mathf.Repeat(h, 1f); | ||
| s = Mathf.Clamp01(s); | ||
| l = Mathf.Clamp01(l); | ||
| if (s < 1e-6f) | ||
| { | ||
| return new Color(l, l, l, 1f); | ||
| } | ||
| float q = l < 0.5f ? l * (1f + s) : l + s - l * s; | ||
| float p = 2f * l - q; | ||
| float r = HueToChannel(p, q, h + 1f / 3f); | ||
| float g = HueToChannel(p, q, h); | ||
| float b = HueToChannel(p, q, h - 1f / 3f); | ||
| return new Color(r, g, b, 1f); |
| bool isActive = _activeColorProperty == propertyName; | ||
| GUI.enabled = !isActive || !_showHSLPicker; | ||
| if (GUILayout.Button( |
- 회색(S=0) 색상에서도 링 드래그/회전이 자연스럽도록 컨트롤 ID별 hue 캐시(_hueByControl) 도입. 색의 채도가 0일 때도 마지막 사용자 hue를 유지하여 인디케이터/삼각형 방향이 보존됨. - HSLToRGB(h,s,l,a) 알파 보존 오버로드 추가하고, Draw 진입 시점 원본 알파를 캡처하여 ColorFromTrianglePoint·HSLToRGB 호출에 전달. 트라이앵글/링 상호작용 후 alpha 채널이 1로 강제되던 문제 해결. - DrawNumericInputs의 슬라이더 결과에도 입력 색의 알파를 반영. - BarycentricFromColor에서 사용하지 않던 hue 매개변수 제거. - ColorCorrection.DrawHSLColorRow가 GUI.enabled의 기존 값을 저장/복원하도록 수정. 부모 스코프의 비활성 상태가 의도치 않게 해제되던 회귀 차단. https://claude.ai/code/session_01DVUmzYCzGpteav2NZmWAsm
Summary
Introduces a new HSL (Hue, Saturation, Lightness) color picker UI component that provides an intuitive visual interface for color selection in the editor. The picker features a hue ring and an inscribed saturation/lightness triangle, replacing standard color fields in color grading workflows.
Key Changes
New HSLColorPicker Component (
HSLColorPicker.cs)ColorCorrection Integration
DrawHSLColorRow()method to display color fields with HSL picker buttonsImageFilter Enhancement
ApplyTintToHsvOffset()andApplyTintToHsvConvert()methods to apply picked colors as HSV adjustmentsLocalization Updates
Implementation Details
https://claude.ai/code/session_01DVUmzYCzGpteav2NZmWAsm