-
Notifications
You must be signed in to change notification settings - Fork 517
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
base: V3
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
|
||
const numberOfVertices = data[21] | ||
let ci = 22 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
||
|
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ export const fromCompactBinary = (data) => { | |
|
||
const created = create() | ||
|
||
created.transforms = mat4.clone(data.slice(1, 17)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
created.transforms = data.slice(1, 17) | ||
|
||
created.isClosed = !!data[17] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
Well... the logic in connectors is going to use vec3 functions so the axis and normal has best be vec3
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.
But the
vec3.clone
function assumes that it takes in avec3
and returns avec3
. So what exactly was this clone accomplishing? If the input is not a validvec3
, then how is cloning going to help? Ditto for many of the comments below.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.
Understood. Then I suggest using fromValues()
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.
Agreed. The cloning doesn't help.
But the underlying NUMERIC type might help, so I suggested using fromValues()