-
-
Notifications
You must be signed in to change notification settings - Fork 23
Color lib swap thi.ng #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
17eff72
a566452
b31e975
d498572
404d465
4b58ef3
4822f10
ce5fddf
bce37e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,8 +10,11 @@ | |||||||||||||
| [afterglow.rhythm :as rhythm] | ||||||||||||||
| [afterglow.show-context :refer [*show*]] | ||||||||||||||
| [afterglow.util :as util] | ||||||||||||||
| [afterglow.transform :as tf] | ||||||||||||||
| [clojure.math.numeric-tower :as math] | ||||||||||||||
| [com.evocomputing.colors :as colors] | ||||||||||||||
|
||||||||||||||
| [thi.ng.color.core :as clr] | ||||||||||||||
| [thi.ng.math.core :as cmath] | ||||||||||||||
| [taoensso.timbre :as timbre] | ||||||||||||||
| [taoensso.tufte :as tufte]) | ||||||||||||||
| (:import (afterglow.effects Assigner Effect))) | ||||||||||||||
|
|
@@ -23,10 +26,10 @@ | |||||||||||||
| the highest green component, and the highest blue component." | ||||||||||||||
| [previous current] | ||||||||||||||
| (if (some? previous) | ||||||||||||||
| (let [red (max (colors/red previous) (colors/red current)) | ||||||||||||||
| green (max (colors/green previous) (colors/green current)) | ||||||||||||||
| blue (max (colors/blue previous) (colors/blue current))] | ||||||||||||||
| (colors/create-color :r red :g green :b blue)) | ||||||||||||||
| (->> (map clr/as-rgba [previous current]) | ||||||||||||||
| (map deref) | ||||||||||||||
| (apply map max) | ||||||||||||||
| (apply clr/rgba)) ;get alpha support for free (when we dont need it)! but what about white | ||||||||||||||
|
||||||||||||||
| current)) | ||||||||||||||
|
|
||||||||||||||
| (defn build-htp-color-assigner | ||||||||||||||
|
|
@@ -40,13 +43,13 @@ | |||||||||||||
| assignment held the highest." | ||||||||||||||
| [head param show snapshot] | ||||||||||||||
| (let [resolved (params/resolve-unless-frame-dynamic param show snapshot head)] | ||||||||||||||
| (fx/build-head-assigner :color head | ||||||||||||||
| (fn [show snapshot target previous-assignment] | ||||||||||||||
| (if (some? previous-assignment) | ||||||||||||||
| (let [current (params/resolve-param resolved show snapshot head) | ||||||||||||||
| previous (params/resolve-param previous-assignment show snapshot head)] | ||||||||||||||
| (htp-merge previous current)) | ||||||||||||||
| resolved))))) | ||||||||||||||
| (fx/build-head-assigner | ||||||||||||||
| :color head | ||||||||||||||
| (fn [show snapshot target previous-assignment] | ||||||||||||||
| (if (some? previous-assignment) | ||||||||||||||
| (apply htp-merge (map #(params/resolve-param % show snapshot head) | ||||||||||||||
| [previous-assignment resolved]))) | ||||||||||||||
| resolved)))) | ||||||||||||||
|
Comment on lines
+51
to
+52
|
||||||||||||||
| [previous-assignment resolved]))) | |
| resolved)))) | |
| [previous-assignment resolved])) | |
| resolved))))) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type comparison should use instance? or the defined constant color-type from params.clj. Raw class name won't work correctly.
| (params/validate-param-type color thi.ng.color.core.HSLA) | |
| (params/validate-param-type color params/color-type) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "wont support multiple shows like this right?" indicates uncertainty about the implementation. This concern should be addressed or clarified before merging.
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The swap! call on line 102 is misaligned - it appears to be at the wrong indentation level and should be inside the let block's scope. This looks like it's outside the main resolution logic where it should be.
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function tf/degrees is called but the namespace tf (afterglow.transform) is imported on line 13. The original code used (:hue ch) directly as degrees, but now it's wrapped in tf/degrees and then negated. Verify that this conversion is necessary and correct for the thi.ng library's hue rotation function.
| (let [as-if-red (clr/rotate-hue resolved (- (tf/degrees (:hue ch))))] | |
| (let [as-if-red (clr/rotate-hue resolved (- (:hue ch)))] |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple when-let statements starting at lines 104 and 111 that should be consistently indented. The inconsistent indentation makes the code harder to read and maintain.
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other locations, this creates a color by converting through the old library. Define directly with the thi.ng library for consistency and performance.
| (clr/as-hsla (apply clr/rgba (map #(/ % 255) (:rgba (colors/create-color :black)))))) | |
| (clr/hsla 0 0 0 1)) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment ";does nothing on this lib it resets hue but maybe someday" suggests that clr/adjust-luminance doesn't work as expected. If the function doesn't behave correctly, this needs to be fixed or an alternative approach used. Having known broken functionality is a critical issue.
| (if color (clr/adjust-luminance color -1.0) default-color)) ;does nothing on this lib it resets hue but maybe someday | |
| (if color | |
| (let [hsla (clr/as-hsla color)] | |
| ;; Preserve hue, saturation, and alpha, but force lightness to 0.0 (fully dark). | |
| (clr/hsla (clr/hue hsla) (clr/saturation hsla) 0.0 (clr/alpha hsla))) | |
| default-color)) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment ";;nvm comment above" suggests the previous understanding about parameter order was incorrect. While the code may work, this comment should be clarified or removed for better code maintainability.
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "shouldnt target/head be to-assignment??" indicates uncertainty about the correctness of using from-assignment's target. This should be verified - if the target can differ between assignments during a fade, using the wrong one could cause issues.
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function clr/luminance is used here but the original code used colors/lightness. These may not be equivalent - luminance is typically a perceptual measure while lightness is from HSL. Verify that using luminance here produces the correct behavior for the saturation sweep effect.
| (clr/hsla (clr/hue color) saturation (clr/luminance color) (clr/alpha color))))) | |
| (clr/hsla (clr/hue color) saturation (clr/lightness color) (clr/alpha color))))) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -609,7 +609,7 @@ | |||||
| cue based on the value of a dynamic parameter. If the parameter | ||||||
| evaluates to `nil`, the non-dynamic cue color is returned." | ||||||
| [param] | ||||||
| (params/validate-param-type param :com.evocomputing.colors/color) | ||||||
| (params/validate-param-type param thi.ng.color.core.HSLA) | ||||||
|
||||||
| (params/validate-param-type param thi.ng.color.core.HSLA) | |
| (params/validate-param-type param thi.ng.color.core/HSLA) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |||||
| [afterglow.util :as util] | ||||||
| [clojure.math.numeric-tower :as math] | ||||||
| [com.evocomputing.colors :as colors] | ||||||
|
||||||
| [com.evocomputing.colors :as colors] |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This converts colors through the old library (colors/create-color) and then converts to the new format. This is inefficient and maintains a dependency on the old library. Consider creating colors directly with the thi.ng library, e.g., (clr/as-hsla (clr/css :black)) or defining the color directly as (clr/hsla 0 0 0).
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "should pass back a bound color??" indicates uncertainty or incomplete implementation. This should be clarified or the TODO resolved before merging.
| (keyword? color) ;should pass back a bound color?? | |
| (keyword? color) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment ";comparing speed" suggests this is experimental or temporary code. If keeping both color type checks is intentional for backwards compatibility during migration, this should be documented properly. Otherwise, the old color type check should be removed once migration is complete.
| (= (type color) :com.evocomputing.colors/color) ;comparing speed | |
| (= (type color) :com.evocomputing.colors/color) ; Support legacy com.evocomputing.colors/color values for backwards compatibility. |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to line 402, this creates a color by converting through the old library unnecessarily. Define the default color directly using the thi.ng library.
| (clr/as-hsla (apply clr/rgba (map #(/ % 255) (:rgba (colors/create-color :black)))))) | |
| (clr/hsla 0 0 0 1)) |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |||||||||
| "Utility functions that are likely to be widely useful" | ||||||||||
| {:author "James Elliott"} | ||||||||||
| (:require [clojure.math.numeric-tower :as math] | ||||||||||
| [com.evocomputing.colors :as colors])) | ||||||||||
| [thi.ng.color.core :as clr])) | ||||||||||
|
|
||||||||||
| (defn ubyte | ||||||||||
| "Convert small integer to its signed byte equivalent. Necessary for convenient handling of DMX values | ||||||||||
|
|
@@ -99,10 +99,10 @@ | |||||||||
| [color] | ||||||||||
| (if (and color | ||||||||||
| ;; Calculate the perceived brightness of the color. | ||||||||||
| (let [[r g b] (map #(/ % 255) [(colors/red color) (colors/green color) (colors/blue color)])] | ||||||||||
| (let [[r g b] @(clr/as-rgba color)] | ||||||||||
| (> (Math/sqrt (+ (* 0.299 r r) (* 0.587 g g) (* 0.114 b b))) 0.6))) | ||||||||||
| "#000" | ||||||||||
| "#fff")) | ||||||||||
| "#222" | ||||||||||
| "#edc")) | ||||||||||
|
Comment on lines
+104
to
+105
|
||||||||||
| "#222" | |
| "#edc")) | |
| "#000" | |
| "#fff")) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,9 @@ | |||||
| (clj-time core format coerce) | ||||||
| [clojure.string :as string] | ||||||
| [com.evocomputing.colors :as colors] | ||||||
| [overtone.at-at :refer [now]] | ||||||
| [thi.ng.color.core :as clr] | ||||||
| [org.httpkit.server :refer [with-channel on-receive on-close]] | ||||||
| [overtone.at-at :refer [now after]] | ||||||
|
||||||
| [overtone.at-at :refer [now after]] | |
| [overtone.at-at :refer [now]] |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function dereferences the result of a pipeline that converts color to int24 then to CSS. The @ dereference suggests these functions return atoms or delays. Verify that this chain works correctly - if clr/as-css returns a string directly, the @ is unnecessary and will cause an error. Check the thi.ng.color.core API documentation.
| (defn rgb-hexstr [color] @(-> color clr/as-int24 clr/as-css)) | |
| (defn rgb-hexstr [color] (-> color clr/as-int24 clr/as-css)) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type comparison (= (type v) thi.ng.color.core.HSLA) should use the fully qualified class name as a symbol or class object. Currently, this will not work as intended because thi.ng.color.core.HSLA is not being treated as a class reference. Consider using (instance? thi.ng.color.core.HSLA v) instead, or define a constant like color-type as done in params.clj line 97.
| (assoc r k (if (= (type v) thi.ng.color.core.HSLA) | |
| (assoc r k (if (instance? thi.ng.color.core.HSLA v) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name used in the list should match the new library. This constructs a list with 'clr/hsla but the function is not directly invocable from the quoted list. This appears to be constructing code for evaluation, but should use the correct namespace-qualified symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable
id-keyis created but appears to be for storing the resolved value in the movement atom. However, this functionality may not have existed in the original code for functions. Verify that this is intentional and that the id-key construction is correct.