Skip to content
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

refactor(modeling)!: no geometry clone #1201

Open
wants to merge 1 commit into
base: V3
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions packages/modeling/src/colors/colorize.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,19 @@ import * as path2 from '../geometries/path2/index.js'
import * as poly3 from '../geometries/poly3/index.js'

const colorGeom2 = (color, object) => {
const newGeom2 = geom2.clone(object)
newGeom2.color = color
return newGeom2
return Object.assign({}, object, { color })
}

const colorGeom3 = (color, object) => {
const newGeom3 = geom3.clone(object)
newGeom3.color = color
return newGeom3
return Object.assign({}, object, { color })
}

const colorPath2 = (color, object) => {
const newPath2 = path2.clone(object)
newPath2.color = color
return newPath2
return Object.assign({}, object, { color })
}

const colorPoly3 = (color, object) => {
const newPoly = poly3.clone(object)
newPoly.color = color
return newPoly
return Object.assign({}, object, { color })
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/modeling/src/connectors/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ import * as vec3 from '../maths/vec3/index.js'
* @example
* let myConnector = create()
*/
export const create = () => ({ point: vec3.create(), axis: vec3.clone([0, 0, 1]), normal: vec3.clone([1, 0, 0]) })
Copy link
Member

Choose a reason for hiding this comment

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

Well... the logic in connectors is going to use vec3 functions so the axis and normal has best be vec3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the vec3.clone function assumes that it takes in a vec3 and returns a vec3. So what exactly was this clone accomplishing? If the input is not a valid vec3, then how is cloning going to help? Ditto for many of the comments below.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. Then I suggest using fromValues()

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The cloning doesn't help.
But the underlying NUMERIC type might help, so I suggested using fromValues()

export const create = () => ({ point: vec3.create(), axis: [0, 0, 1], normal: [1, 0, 0] })
3 changes: 0 additions & 3 deletions packages/modeling/src/geometries/geom2/clone.d.ts

This file was deleted.

7 changes: 0 additions & 7 deletions packages/modeling/src/geometries/geom2/clone.js

This file was deleted.

26 changes: 0 additions & 26 deletions packages/modeling/src/geometries/geom2/clone.test.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const fromCompactBinary = (data) => {

const created = create()

created.transforms = mat4.clone(data.slice(1, 17))
created.transforms = data.slice(1, 17)
Copy link
Member

Choose a reason for hiding this comment

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

ditto


for (let i = 21; i < data.length;) {
const length = data[i++] // number of points for this polygon
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/geom2/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export { clone } from './clone'
export { create } from './create'
export { fromSides } from './fromSides'
export { fromCompactBinary } from './fromCompactBinary'
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/geom2/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* import { geometries } from '@jscad/modeling'
* let myShape = geometries.geom2.create([ [[-1,-1], [1,-1], [1,1], [-1,1]] ])
*/
export { clone } from './clone.js'
export { create } from './create.js'
export { fromSides } from './fromSides.js'
export { fromCompactBinary } from './fromCompactBinary.js'
Expand Down
3 changes: 1 addition & 2 deletions packages/modeling/src/geometries/geom2/reverse.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { clone } from './clone.js'

/**
* Reverses the given geometry so that the outline points are flipped in the opposite order.
Expand All @@ -11,7 +10,7 @@ import { clone } from './clone.js'
* let newGeometry = reverse(geometry)
*/
export const reverse = (geometry) => {
const reversed = clone(geometry)
const reversed = Object.assign({}, geometry)
reversed.outlines = reversed.outlines.map((outline) => outline.slice().reverse())
return reversed
}
3 changes: 0 additions & 3 deletions packages/modeling/src/geometries/geom3/clone.d.ts

This file was deleted.

7 changes: 0 additions & 7 deletions packages/modeling/src/geometries/geom3/clone.js

This file was deleted.

31 changes: 0 additions & 31 deletions packages/modeling/src/geometries/geom3/clone.test.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const fromCompactBinary = (data) => {

const created = create()

created.transforms = mat4.clone(data.slice(1, 17))
created.transforms = data.slice(1, 17)
Copy link
Member

Choose a reason for hiding this comment

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

ditto


const numberOfVertices = data[21]
let ci = 22
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/geom3/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export { clone } from './clone'
export { create } from './create'
export { fromPoints } from './fromPoints'
export { fromCompactBinary } from './fromCompactBinary'
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/geom3/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* [[-1,-1,1], [1,-1,1], [1,1,1], [-1,1,1]]
* ])
*/
export { clone } from './clone.js'
export { create } from './create.js'
export { fromPoints } from './fromPoints.js'
export { fromCompactBinary } from './fromCompactBinary.js'
Expand Down
5 changes: 2 additions & 3 deletions packages/modeling/src/geometries/path2/appendArc.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,10 @@ export const appendArc = (options, geometry) => {

// validate the given options
if (!Array.isArray(endpoint)) throw new Error('endpoint must be an array of X and Y values')
if (endpoint.length < 2) throw new Error('endpoint must contain X and Y values')
Copy link
Member

Choose a reason for hiding this comment

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

Why fix in this PR?

endpoint = vec2.clone(endpoint)
if (endpoint.length !== 2) throw new Error('endpoint must contain X and Y values')
z3dev marked this conversation as resolved.
Show resolved Hide resolved

if (!Array.isArray(radius)) throw new Error('radius must be an array of X and Y values')
if (radius.length < 2) throw new Error('radius must contain X and Y values')
if (radius.length !== 2) throw new Error('radius must contain X and Y values')

if (segments < 4) throw new Error('segments must be four or more')

Expand Down
3 changes: 0 additions & 3 deletions packages/modeling/src/geometries/path2/clone.d.ts

This file was deleted.

7 changes: 0 additions & 7 deletions packages/modeling/src/geometries/path2/clone.js

This file was deleted.

4 changes: 1 addition & 3 deletions packages/modeling/src/geometries/path2/close.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import { EPS } from '../../maths/constants.js'

import * as vec2 from '../../maths/vec2/index.js'

import { clone } from './clone.js'

/**
* Close the given geometry.
* @param {path2} geometry - the path to close
Expand All @@ -13,7 +11,7 @@ import { clone } from './clone.js'
export const close = (geometry) => {
if (geometry.isClosed) return geometry

const cloned = clone(geometry)
const cloned = Object.assign({}, geometry)
cloned.isClosed = true

if (cloned.points.length > 1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const fromCompactBinary = (data) => {

const created = create()

created.transforms = mat4.clone(data.slice(1, 17))
Copy link
Member

Choose a reason for hiding this comment

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

ditto

created.transforms = data.slice(1, 17)

created.isClosed = !!data[17]

Expand Down
3 changes: 1 addition & 2 deletions packages/modeling/src/geometries/path2/fromPoints.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ export const fromPoints = (options, points) => {
const defaults = { closed: false }
let { closed } = Object.assign({}, defaults, options)

let created = create()
created.points = points.map((point) => vec2.clone(point))
Copy link
Member

Choose a reason for hiding this comment

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

seems reasonable

however, we need to mention somewhere that points/vectors supplied to from* functions will be retained by the new geometry, and should not be changed. i added #1195 to track this.

let created = create(points)

// check if first and last points are equal
if (created.points.length > 1) {
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/path2/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
export { appendArc, AppendArcOptions } from './appendArc'
export { appendBezier, AppendBezierOptions } from './appendBezier'
export { appendPoints } from './appendPoints'
export { clone } from './clone'
export { close } from './close'
export { concat } from './concat'
export { create } from './create'
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/path2/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
export { appendArc } from './appendArc.js'
export { appendBezier } from './appendBezier.js'
export { appendPoints } from './appendPoints.js'
export { clone } from './clone.js'
export { close } from './close.js'
export { concat } from './concat.js'
export { create } from './create.js'
Expand Down
3 changes: 1 addition & 2 deletions packages/modeling/src/geometries/path2/reverse.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { clone } from './clone.js'

/**
* Reverses the path so that the points are in the opposite order.
Expand All @@ -12,7 +11,7 @@ import { clone } from './clone.js'
*/
export const reverse = (geometry) => {
// NOTE: this only updates the order of the points
const cloned = clone(geometry)
const cloned = Object.assign({}, geometry)
cloned.points = geometry.points.slice().reverse()
return cloned
}
3 changes: 0 additions & 3 deletions packages/modeling/src/geometries/poly2/clone.d.ts

This file was deleted.

8 changes: 0 additions & 8 deletions packages/modeling/src/geometries/poly2/clone.js

This file was deleted.

17 changes: 0 additions & 17 deletions packages/modeling/src/geometries/poly2/clone.test.js

This file was deleted.

1 change: 0 additions & 1 deletion packages/modeling/src/geometries/poly2/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export { arePointsInside } from './arePointsInside'
export { clone } from './clone'
export { create } from './create'
export { isA } from './isA'
export { isConvex } from './isConvex'
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/poly2/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* const p1 = geometries.poly2.create([[0,0], [4,0], [4,3]])
*/
export { arePointsInside } from './arePointsInside.js'
export { clone } from './clone.js'
export { create } from './create.js'
export { isA } from './isA.js'
export { isConvex } from './isConvex.js'
Expand Down
4 changes: 0 additions & 4 deletions packages/modeling/src/geometries/poly3/clone.d.ts

This file was deleted.

26 changes: 0 additions & 26 deletions packages/modeling/src/geometries/poly3/clone.js

This file was deleted.

36 changes: 0 additions & 36 deletions packages/modeling/src/geometries/poly3/clone.test.js

This file was deleted.

1 change: 0 additions & 1 deletion packages/modeling/src/geometries/poly3/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export { clone } from './clone'
export { create } from './create'
export { fromVerticesAndPlane } from './fromVerticesAndPlane'
export { invert } from './invert'
Expand Down
1 change: 0 additions & 1 deletion packages/modeling/src/geometries/poly3/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* import { geometries } from '@jscad/modeling'
* const polygon = geometries.poly3.create([[0,0,0], [4,0,0], [4,3,12]])
*/
export { clone } from './clone.js'
export { create } from './create.js'
export { fromVerticesAndPlane } from './fromVerticesAndPlane.js'
export { invert } from './invert.js'
Expand Down
Loading