From 47fadee3c39cf754a8d2824bcfe054c76805979b Mon Sep 17 00:00:00 2001 From: Alexandru Popovici Date: Mon, 2 May 2022 17:40:22 +0300 Subject: [PATCH 1/2] Alex/#723#711 (#733) * Handled #711. New arc converter for viewer. Original issues fixed, still needs more testing * #711. Added some documenting comments * Working on parsing the displayValue when no direct conversion exists. Seems to working now. Justs need some additional info * Worked on #723 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- packages/viewer-sandbox/src/main.ts | 2 +- .../viewer/src/modules/converter/Converter.js | 124 ++++++++++++++++-- 2 files changed, 117 insertions(+), 9 deletions(-) diff --git a/packages/viewer-sandbox/src/main.ts b/packages/viewer-sandbox/src/main.ts index 5a98b38ad..ea4afbeb0 100644 --- a/packages/viewer-sandbox/src/main.ts +++ b/packages/viewer-sandbox/src/main.ts @@ -32,7 +32,7 @@ pane.addInput(PARAMS, 'color') // Load demo object viewer.loadObject( - 'https://latest.speckle.dev/streams/010b3af4c3/objects/a401baf38fe5809d0eb9d3c902a36e8f' + 'https://speckle.xyz/streams/99abc74dd4/objects/ab503a2025e706717bff467ef8f96488' ) viewer.on<{ progress: number; id: string; url: string }>('load-progress', (a) => { diff --git a/packages/viewer/src/modules/converter/Converter.js b/packages/viewer/src/modules/converter/Converter.js index 66a89ab2f..56840e19c 100644 --- a/packages/viewer/src/modules/converter/Converter.js +++ b/packages/viewer/src/modules/converter/Converter.js @@ -5,6 +5,9 @@ import * as BufferGeometryUtils from 'three/examples/jsm/utils/BufferGeometryUti import ObjectWrapper from './ObjectWrapper' import { getConversionFactor } from './Units' import MeshTriangulationHelper from './MeshTriangulationHelper' +import { Matrix4 } from 'three' +import { Vector3 } from 'three' +import { Line3 } from 'three' /** * Utility class providing some top level conversion methods. @@ -90,7 +93,7 @@ export default class Coverter { } } - const target = obj.data || obj + const target = obj //obj.data || obj // Check if the object has a display value of sorts let displayValue = @@ -173,6 +176,19 @@ export default class Coverter { this.activePromises -= childrenConversionPromisses.length } + directConverterExists(obj) { + return this[`${this.getSpeckleType(obj)}ToBufferGeometry`] !== undefined + } + + getDisplayValue(obj) { + return ( + obj['displayMesh'] || + obj['@displayMesh'] || + obj['displayValue'] || + obj['@displayValue'] + ) + } + /** * Directly converts an object and invokes the callback with the the conversion result. * If you don't know what you're doing, use traverseAndConvert() instead. @@ -186,7 +202,19 @@ export default class Coverter { const type = this.getSpeckleType(obj) if (this[`${type}ToBufferGeometry`]) { return await this[`${type}ToBufferGeometry`](obj.data || obj, scale) - } else return null + } + /** + * Regarding #723. This would be more generic and possibly handle other + * types with missing direct convertor, however I don't feel it is the + * 'convert' fuction's place to handle this... + */ + // else { + // let element; + // if((element = this.getDisplayValue(obj)) !== undefined) { + // return await this.convert(element, scale); + // } + // } + return null } catch (e) { console.warn(`(Direct convert) Failed to convert object with id: ${obj.id}`) throw e @@ -519,8 +547,12 @@ export default class Coverter { const buffers = [] for (let i = 0; i < obj.segments.length; i++) { - const element = obj.segments[i] - const conv = await this.convert(element, scale) + let element = obj.segments[i] + let conv + if (this.directConverterExists(element)) conv = await this.convert(element, scale) + else if ((element = this.getDisplayValue(element)) !== undefined) + conv = await this.convert(element, scale) + buffers.push(conv?.bufferGeometry) } const geometry = BufferGeometryUtils.mergeBufferGeometries(buffers) @@ -553,21 +585,94 @@ export default class Coverter { } async ArcToBufferGeometry(obj, scale = true) { + /** + * Old implementation + */ + // const radius = obj.radius + // const curve = new THREE.EllipseCurve( + // 0, + // 0, // ax, aY + // radius, + // radius, // xRadius, yRadius + // obj.startAngle, + // obj.endAngle, // aStartAngle, aEndAngle + // false, // aClockwise + // 0 // aRotation + // ) + // const points = curve.getPoints(50); + // const t = this.PlaneToMatrix4(obj.plane, scale); + // const geometry = new THREE.BufferGeometry() + // .setFromPoints(points) + // .applyMatrix4(t) + // return new ObjectWrapper(geometry, obj, 'line') + + /** + * New implementation, a bit verbose, but it's more clear this way. + */ + const origin = new Vector3( + obj.plane.origin.x, + obj.plane.origin.y, + obj.plane.origin.z + ) + const startPoint = new Vector3(obj.startPoint.x, obj.startPoint.y, obj.startPoint.z) + const endPoint = new Vector3(obj.endPoint.x, obj.endPoint.y, obj.endPoint.z) + const midPoint = new Vector3(obj.midPoint.x, obj.midPoint.y, obj.midPoint.z) + + const chord = new Line3(startPoint, endPoint) + // This the projection of the origin on the chord + const chordCenter = chord.getCenter(new Vector3()) + // Direction from the origin to the mid point + const d0 = new Vector3().subVectors(midPoint, origin) + d0.normalize() + // Direction from the origin to it;s projection on the chord + const d1 = new Vector3().subVectors(chordCenter, origin) + d1.normalize() + // If the two above directions point in opposite directions, we need to reverse the arc's winding order + const _clockwise = d0.dot(d1) < 0 + + // Here we compute arc's orthonormal basis vectors using the origin and the two end points. + const v0 = new Vector3().subVectors(startPoint, origin) + v0.normalize() + const v1 = new Vector3().subVectors(endPoint, origin) + v1.normalize() + const v2 = new Vector3().crossVectors(v0, v1) + v2.normalize() + const v3 = new Vector3().crossVectors(v2, v0) + v3.normalize() + + // This is just the angle between the start and end points. Should be same as obj.angleRadians(or something) + const angle = Math.acos(v0.dot(v1)) const radius = obj.radius + // We draw the arc in a local un-rotated coordinate system. We rotate it later on via transformation const curve = new THREE.EllipseCurve( 0, 0, // ax, aY radius, radius, // xRadius, yRadius - obj.startAngle, - obj.endAngle, // aStartAngle, aEndAngle - false, // aClockwise + 0, + angle, // aStartAngle, aEndAngle + _clockwise, // aClockwise 0 // aRotation ) + // This just samples points along the arc curve const points = curve.getPoints(50) + + const matrix = new Matrix4() + // Scale first, in order for the composition to work correctly + const conversionFactor = scale ? getConversionFactor(obj.plane.units) : 1 + if (scale) { + matrix.scale( + new THREE.Vector3(conversionFactor, conversionFactor, conversionFactor) + ) + } + // We determine the orientation of the plane using the three basis vectors computed above + matrix.makeBasis(v0, v3, v2) + // We translate it to the circle's origin + matrix.setPosition(origin) + const geometry = new THREE.BufferGeometry() .setFromPoints(points) - .applyMatrix4(this.PlaneToMatrix4(obj.plane, scale)) + .applyMatrix4(matrix) return new ObjectWrapper(geometry, obj, 'line') } @@ -620,6 +725,9 @@ export default class Coverter { this.PointToVector3(plane.normal).normalize() ) m.setPosition(this.PointToVector3(plane.origin)) + /** + * I think scaling should be done first. + */ if (scale) { m.scale(new THREE.Vector3(conversionFactor, conversionFactor, conversionFactor)) } From 753b38abb4e17060476cc20e37a74ef476542cf5 Mon Sep 17 00:00:00 2001 From: Alexandru Popovici Date: Mon, 2 May 2022 17:50:42 +0300 Subject: [PATCH 2/2] Alex/#723#711 patch (#737) * Handled #711. New arc converter for viewer. Original issues fixed, still needs more testing * #711. Added some documenting comments * Working on parsing the displayValue when no direct conversion exists. Seems to working now. Justs need some additional info * Worked on #723 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Prioritized 'displayValue' over 'displayMesh' properties when fetching displayValue data as per Claire's suggestion. Removed the builint lineWidth assignment since it will only add confusion and it's also incorrect Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- packages/viewer/src/modules/SceneObjectManager.js | 7 +++++-- packages/viewer/src/modules/converter/Converter.js | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/viewer/src/modules/SceneObjectManager.js b/packages/viewer/src/modules/SceneObjectManager.js index cfd048503..d8cf90d39 100644 --- a/packages/viewer/src/modules/SceneObjectManager.js +++ b/packages/viewer/src/modules/SceneObjectManager.js @@ -210,8 +210,11 @@ export default class SceneObjectManager { let material = this.lineMaterial if (wrapper.meta.displayStyle) { material = this.lineMaterial.clone() - material.linewidth = - wrapper.meta.displayStyle.lineweight > 0 ? wrapper.meta.displayStyle : 1 + // This will only add confusion since it *might* work on some platforms. + // However, it's in pixels and the displayStyle will express the thickness in world units + // This will be replaced by the upcoming change to line rendering which supports variable + // thickness in both world space and pixels + // material.linewidth = wrapper.meta.displayStyle.lineweight > 0 ? wrapper.meta.displayStyle : 1 material.color = new THREE.Color(this._argbToRGB(wrapper.meta.displayStyle.color)) // material.color.convertSRGBToLinear(); diff --git a/packages/viewer/src/modules/converter/Converter.js b/packages/viewer/src/modules/converter/Converter.js index 56840e19c..3450df320 100644 --- a/packages/viewer/src/modules/converter/Converter.js +++ b/packages/viewer/src/modules/converter/Converter.js @@ -182,10 +182,10 @@ export default class Coverter { getDisplayValue(obj) { return ( - obj['displayMesh'] || - obj['@displayMesh'] || obj['displayValue'] || - obj['@displayValue'] + obj['@displayValue'] || + obj['displayMesh'] || + obj['@displayMesh'] ) }