Skip to content

Remove weird Solver code.#15

Open
Robotino04 wants to merge 3 commits intoVedalAI:mainfrom
Robotino04:main
Open

Remove weird Solver code.#15
Robotino04 wants to merge 3 commits intoVedalAI:mainfrom
Robotino04:main

Conversation

@Robotino04
Copy link

@Robotino04 Robotino04 commented Dec 23, 2024

Also checks if the lamp is reachable and uses random colors otherwise.

Also checks if the lamp is reachable and uses random colors if not.
@remiliacn
Copy link

Your code does not look AI generated, I do not recommend approve this PR.

Copy link

@wolfschaf wolfschaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally something sane, but I would like to bikeshed a bit more:

  • Color change error
    The shifting does not always lerp flawlessly to the target value.
    I noticed it when it changed colors from yellow to blue, but I think on some other color combination it also sometimes does it.
    Don't know what causes it though, either the lerping over RGB or an error when calculating the hue shift. So I would just directly lerp over the hue shift and hope that fixes it.
  • Save hue shift instead of RGB
    I would also just directly calc the hue shift when fetching and storing it later for lerping. Then we don't have to calc hue2rgb in generateRandomSaturatedColor() and can just return the hue shift.

main.js Outdated
.catch(error => {
console.error("Failed to fetch lamp color:", error);
});
if (isRunningOnVedalsPC){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would clean this up:

if (isRunningOnVedalsPC){
    fetch("http://localhost:8000/lamp")
        .then(response => response.text().trim()) //toLowerCase() not needed anymore
        .then(hexColor => {targetColor = hexToRgb(hexColor);}) //or just calc the hue shift here
        .catch(error => {
            isRunningOnVedalsPC = false;
            console.error("Failed to fetch lamp color:", error);
        });
}

color.js Outdated
const ak = a[i] / Math.pow(A + k + 1, alpha);
values[i] = fix(values[i] - ak * g, i);
}
h /= 6;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why divide by 6?
Couldn't we just multiply by 60 and then remove the *360 below?

But I would just restructure this whole thing, since we only need to calc the hue value.

function calculateHueShift(r, g, b) {
    function rgbToHue(r, g, b) {
        r /= 255;
        g /= 255;
        b /= 255;

        const cMax  = Math.max(r,g,b);
        const cMin  = Math.min(r,g,b);
        const delta = cMax - cMin;
    
        if (!delta) return 0;
        
        let hue;
        switch (cMax) {
            case r: hue = (g - b) / delta + (g < b ? 6 : 0); break;
            case g: hue = (b - r) / delta + 2; break;
            case b: hue = (r - g) / delta + 4; break;
        }
        return hue * 60;
    }

    // Convert yellow RGB(255, 255, 0) and target color to hue
    const yellowHue = rgbToHue(255, 255, 0);
    const targetHue = rgbToHue(r, g, b);

    // Calculate hue shift
    let hueShift = targetHue - yellowHue;
    if (hueShift < 0) hueShift += 360;

    return hueShift;
}

color.js Outdated

if (max === min) {
h = s = 0;
function generateRandomSaturatedColor() {
Copy link

@wolfschaf wolfschaf Dec 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like mentioned, just return hue if we lerp over hue shift

@Robotino04
Copy link
Author

Your code does not look AI generated, I do not recommend approve this PR.

Then you didn't look hard enough. ChatGPT wrote all the color conversions.

@Robotino04
Copy link
Author

Robotino04 commented Dec 25, 2024

Finally something sane, but I would like to bikeshed a bit more:

* **Color change error**
  The shifting does not always lerp flawlessly to the target value.
  I noticed it when it changed colors from yellow to blue, but I think on some other color combination it also sometimes does it.
  Don't know what causes it though, either the lerping over RGB or an error when calculating the hue shift. So I would just directly lerp over the hue shift and hope that fixes it.

This probably happens because yellow and blue are complementary colors so RGB-lerping them will be gray for some time. And gray doesn't have a real hue so it's just random.
I didn't lerp over the hue shift because that would show all the colors in between as well. So yellow to blue would still show orange and violet before reaching blue. It would probably still look better so I'll try it anyways.

@wolfschaf
Copy link

This probably happens because yellow and blue are complementary colors so RGB-lerping them will be gray for some time. And gray doesn't have a real hue so it's just random. I didn't lerp over the hue shift because that would show all the colors in between as well. So yellow to blue would still show orange and violet before reaching blue. It would probably still look better so I'll try it anyways.

Ahh, yes, I see. It's because we don't change the saturation and the brightness, so after B>(RG) it jumps directly to the blue hue. In other words, the lerp is never linear and in this case it is instant.

Hmm, one solution would to reduce the sepia, so use the saturation value as sepia, I think?
Or just try and see how good the hue shift lerping looks.

Some improvements suggested by wolfschaf.
@Robotino04
Copy link
Author

Ahh, yes, I see. It's because we don't change the saturation and the brightness, so after B>(RG) it jumps directly to the blue hue. In other words, the lerp is never linear and in this case it is instant.

Hmm, one solution would to reduce the sepia, so use the saturation value as sepia, I think? Or just try and see how good the hue shift lerping looks.

Lerping the hue looks fine imo. The only case where it looks weird to show all colors in between is still complementary colors. But those would just be white for a while if we set the sepia using saturation, so idk which is better.

@Robotino04
Copy link
Author

Nevermind all this. I found a pure css solution so we can just transition: background-color now.

Copy link

@wolfschaf wolfschaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise, glad you found a simple solution 👍

@@ -48,4 +48,4 @@ <h1>Vedal forever enslaved by Neuro-sama</h1>
<script src="color.js"></script>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed

background-color: purple;
transition: background-color 1.5s;

background-image: url(lavalamp.png);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reduce this to:

#lavalamp {
    cursor: pointer;
    mix-blend-mode: luminosity;
}

#lamp-container {
    animation: pulse 2s ease-in-out infinite;
    transition: background-color 1.5s;

    mask-image: url(lavalamp.png);
    mask-size: 98%;  /*avoid artifacts*/
    mask-repeat: no-repeat;
    mask-position: center;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants