Skip to content

add primitives for vertical- and horizontal-symmetry#1862

Open
schanzer wants to merge 9 commits into
horizonfrom
image-symmetry
Open

add primitives for vertical- and horizontal-symmetry#1862
schanzer wants to merge 9 commits into
horizonfrom
image-symmetry

Conversation

@schanzer

Copy link
Copy Markdown
Contributor

add primitives for vertical- and horizontal-symmetry (needed in the library due to performance reasons). Claude Sonnet 4.6 and Gemini 3.1 Thinking did the heavy lifting.

…ibrary due to performance reasons). Claude Sonnet 4.6 and Gemini 3.1 Thinking did the heavy lifting.
@schanzer
schanzer requested a review from blerner March 27, 2026 12:56
@blerner

blerner commented Mar 27, 2026

Copy link
Copy Markdown
Member

Most of the delta here is gratuitous (and inconsistent) whitespace changes - could you revert those, so that we can focus on just the interesting part?

Naming-wise, I think the names are inconsistent -- everything else has an image- prefix, so these should be image-vertical-symmetry and image-horizontal-symmetry.

Also, the names are inconsistent with the other horizontal and vertical names: if img is the same picture as flip-horizontal(img), then its vertical-symmetry is 1. That's rather confusing....

…in the library due to performance reasons). Claude Sonnet 4.6 and Gemini 3.1 Thinking did the heavy lifting."

This reverts commit 9a467e4.
@schanzer

Copy link
Copy Markdown
Contributor Author

@blerner fixed the whitespace issue. Major disagreement with you about the naming, but that will be inefficient to do over text. Maybe we can chat?

Comment thread src/js/trove/image-lib.js
for (var x = 0; x < halfW; x++) {
var i1 = (y * width + x) * 4;
var i2 = (y * width + (width - 1 - x)) * 4;
for (var ch = 0; ch < 4; ch++) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the actual similarity formula being used in these two functions? I think, if I'm understanding correctly, that it's the root mean-squared error over every pixel and every color channel? Probably worth a clarifying comment somewhere...

Comment thread src/js/trove/image-lib.js Outdated
for (var y = 0; y < height; y++) {
for (var x = 0; x < halfW; x++) {
var i1 = (y * width + x) * 4;
var i2 = (y * width + (width - 1 - x)) * 4;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I hate magic formulas like these :( Though I do understand it, I kinda wish we had

const getPixel(data, x, y, channel) => data[((y * width) + x) * 4 + channel];

And then you'd have var d = getPixel(data, x, y, ch) - getPixel(data, width - 1 - x, y, ch), which to me reads a lot more cleanly. OTOH, you're writing this in raw JS because of performance, so you could do

const basePixelIndex(x, y) => ((y * width) + x) * 4;

and then have

var i1 = basePixelIndex(x, y);
var i2 = basePixelIndex(width - 1 - x, y);

which saves two whole multiplications per loop ;-)

@blerner

blerner commented Mar 28, 2026

Copy link
Copy Markdown
Member

Those changes look good; I'll double-check them with Joe the next time we meet, and we'll get this merged soon.

@jpolitz

jpolitz commented Mar 30, 2026

Copy link
Copy Markdown
Member

Good discussion good feature. @blerner and I had some good experiments just now.

I think we can use images-difference for this, which already exists, but has some bugs. If you want a library function right now:

include image-typed
import image-structs as IM
include from IM:
    * hiding (tan)
end

img = beside(square(20, mode-solid, color(255, 255, 0, 1)), square(20, mode-solid, color(0, 0, 255, 0)))

images-difference(flip-horizontal(img), img)

images-difference(flip-vertical(img), img)

fun-factor = num-sqrt((((255 * 255) * 3) + 1) / 4)

(~0 + images-difference(flip-horizontal(img), img).v) / fun-factor

(~0 + images-difference(flip-horizontal(img), img).v) / fun-factor

So you can do this with:

# returns 0ish if you flip horizontal and it is not equal, returns 1ish if you flip horizontal and it is close
fun symmetric-if-you-flip-horizontal(i1):
  diff = images-difference(flip-horizontal(i1), i1)
  1 - (~0 + (diff.v / fun-factor))
end

# returns 0ish if you flip vertical and it is not equal, returns 1ish if you flip vertical and it is close
fun symmetric-if-you-flip-vertical(i1):
  diff = images-difference(flip-vertical(i1), i1)
  1 - (~0 + (diff.v / fun-factor))
end

The extra roughnum and the fun-factor are because of (a) a bug where a roughnum is not wrapped and (b) the library mixing 0-255 and 0-1 as magnitudes for color and alpha channels, making it not be clearly 255 as a denominator.

We should fix images-difference! It is only used in tests. But it is a good place to start here.

We can add images-difference-by and have more algorithms if we want later.

jpolitz added a commit that referenced this pull request Mar 30, 2026
…explicitly

ask for 0-255 ranges for rgb and a

Relevant for #1862
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