Skip to content

fix(modeling): keep geometry fields when reversing a geom2 - #1485

Open
jbroll wants to merge 1 commit into
jscad:masterfrom
jbroll:pr/upstream-fix-geom2-reverse-fields
Open

jbroll wants to merge 1 commit into
jscad:masterfrom
jbroll:pr/upstream-fix-geom2-reverse-fields

Conversation

@jbroll

@jbroll jbroll commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

geom2.reverse built a new geometry from the reversed sides and dropped every other field. Mirroring a geom2 (a transform with a negative determinant) goes through reverse, so a colored geom2 lost its color and any user fields when mirrored. This copies the input's fields and replaces only sides and transforms.

Tests

New tests in reverse.test.js:

  • reverse keeps color and a user field, returns new sides, and resets transforms to identity
  • mirroring through geom2.transform with a negative X scale keeps color and a user field

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests? packages/modeling: 595 passed; standardx clean on the changed files.

No build files are included.

🤖 Generated with Claude Code

geom2.reverse built a new geometry from the reversed sides and dropped every
other field, so mirroring a colored geom2 (transform with a negative
determinant goes through reverse) lost its color and any user fields. Copy
the input's fields and replace only sides and transforms.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@z3dev z3dev left a comment

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.

@jbroll Nice. Thanks. Let me know what you think about the requests.

Sadly, I think that geom3.invert() has the same issue.

t.true(compareVectors(another.transforms, mat4.create()))
})

test('reverse: mirroring a geom2 keeps its fields', (t) => {

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.

Can you move this to transform.test.js? It's probably better in that suite.

const newsides = oldsides.map((side) => [side[1], side[0]])
newsides.reverse() // is this required?
return create(newsides)
return Object.assign({}, geometry, create(newsides))

@z3dev z3dev Sep 27, 2026 •

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.

We could save a function call and multiple object creation by using...

return Object.assign({}, geometry, { sides: newsides })

@z3dev
z3dev requested a review from hrgdavor September 27, 2026 08:27
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.

2 participants