[PATCH] D29363: [XRAY] A Color Choosing helper for XRay Graph

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Sorry to jump in with a peanut gallery comment, but just wanted to push back on some of the complexity we're adding here.



================
Comment at: tools/llvm-xray/xray-color-helper.cc:22-35
+// Static Polynomials for the ColorHelper.
+//
+// In order to calculate these polynomials,
+//   1. Convert the color scheme samples from sRGB to LAB color space.
+//   2. Interpolate between the descrete colors in LAB space using a cubic
+//      spline interpolation.
+//   3. Sample this interpolation at 100 points and convert to sRGB.
----------------
I really think this is more complexity than is reasonable to add to LLVM for this use case.

IMO, we don't need perfectly accurate color interpolation here. We need roughly accurate, *simple* interpolation so that it is useful even where it isn't perfect.

I would strongly suggest switching to what I understand is the very common scheme of RGB -> HSV, linear interpolate, HSV -> RGB process for interpolating colors. That seems likely to be good enough for all realistic usages LLVM will have, and much easier to understand and maintain going forward.


https://reviews.llvm.org/D29363