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

Allow hull() to process listOfPoints3 in addition to path2, geom2 and geom3 #1343

Closed
wants to merge 1 commit into from
Closed

Conversation

Hermann-SW
Copy link
Contributor

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?

Note: please do NOT include build files (those generate by the build-xxx commands) with your PR,

Thank you for your help in advance, much appreciated !

@Hermann-SW
Copy link
Contributor Author

Hermann-SW commented May 21, 2024

New testcase is for "n=5" and "≤ n" in this JSCAD app:
https://stamm-wilbrandt.de/lattice_sphere_cmp.html
In that app "hullFromPoints3(listofpoints)" does the job of this pull request.

image

@z3dev
Copy link
Member

z3dev commented May 22, 2024

@Hermann-SW Thanks again.

There are many ways to make a cake. I'm thinking that we could offer users more recipes if we reorganize the hull functions slightly. Something like this...

hull() // takes lists of path2, geom2, geom3
  calls hullGeom3() // with list of geom3
     calls hullPoints3() // with unique set of 3D vertices

Today, only hull() is available in the API, but I'm thinking that all of these can exposed in the API.

And we do the same for 2D hulls as well.

@platypii Is this what you were thinking?

@platypii
Copy link
Contributor

My thought was to leave hull as is. It's okay if hull accepts a list of points to me. One thing I don't like about this api change though, is that nowhere else do we treat "list of points" as a geometry anywhere else, so it makes hull inconsistent with the rest of the API.

Personally, my preferred design for this API would actually be something like geom3.createConvex(points) or something similar to that. It actually is the same operation as hull on points, but I think makes it much more intuitive for users who think "I want to define a 3D shape based on its outer points" whereas now if you wanted to make an arbitrary 3D shape from points, you need to think in "polygons".

That being said, this change looks fine, and I appreciate the test @Hermann-SW! I am happy to have this change in whatever form people prefer.

@Hermann-SW
Copy link
Contributor Author

Hermann-SW commented May 24, 2024

My thought was to leave hull as is. It's okay if hull accepts a list of points to me. One thing I don't like about this api change though, is that nowhere else do we treat "list of points" as a geometry anywhere else, so it makes hull inconsistent with the rest of the API.

OK.

Personally, my preferred design for this API would actually be something like geom3.createConvex(points) or something similar to that. It actually is the same operation as hull on points, but I think makes it much more intuitive for users who think "I want to define a 3D shape based on its outer points" ...

I need help then, as I tried to implement that function and dev server stops working.
It was easy to just modify hull.js, here new files need to be created and correctly integrated ...
Without the index* file changes, geom3.createConvex() is undefined.
With the changes working hull demo below stops working with:

TypeError: geom3.isA is not a function

    toUniquePoints/<@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:37495:22

    toUniquePoints@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:37492:14

    hullGeom3@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:35950:32

    hull@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:35833:13

    main@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d line 2253 > Function:16:11

    instanciateDesign@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:1780:49

    rebuildSolids@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:1855:41

    rebuildGeometryWorker/self.onmessage@blob:http://192.168.10.34:8081/b266d022-a331-429c-9d18-52fc42328a3d:1897:24
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   index.d.ts
	modified:   index.js

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	createConvex.d.js
	createConvex.js

no changes added to commit (use "git add" and/or "git commit -a")
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ 
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ git diff
diff --git a/packages/modeling/src/geometries/geom3/index.d.ts b/packages/modeling/src/geometries/geom3/index.d.ts
index 436f6e01..da51a7b4 100644
--- a/packages/modeling/src/geometries/geom3/index.d.ts
+++ b/packages/modeling/src/geometries/geom3/index.d.ts
@@ -1,6 +1,7 @@
 export { default as clone } from './clone'
 export { default as create } from './create'
 export { default as fromPoints } from './fromPoints'
+export { default as createConvex } from './createConvex'
 export { default as fromCompactBinary } from './fromCompactBinary'
 export { default as invert } from './invert'
 export { default as isA } from './isA'
diff --git a/packages/modeling/src/geometries/geom3/index.js b/packages/modeling/src/geometries/geom3/index.js
index 2591de08..3ca83da5 100644
--- a/packages/modeling/src/geometries/geom3/index.js
+++ b/packages/modeling/src/geometries/geom3/index.js
@@ -23,6 +23,7 @@
 module.exports = {
   clone: require('./clone'),
   create: require('./create'),
+  createConvex: require('./createConvex'),
   fromPoints: require('./fromPoints'),
   fromCompactBinary: require('./fromCompactBinary'),
   invert: require('./invert'),
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ 
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ cat createConvex.d.js 
import Geom3 from './type'
import Vec3 from '../../maths/vec3/type'

export default createsConvex

declare function createConvex(points: Array<Array<Vec3>>): Geom3
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ 
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ cat createConvex.js 
const create = require('./create')

const fromPoints = require('./fromPoints')

const hullGeom3 = require('../../operations/hulls/hullGeom3')

/**
 * Construct a new convex 3D geometry from a list of points.
 * @param {Array} listofpoints - list of points to construct convex 3D geometry
 * @returns {geom3} a new geometry
 * @alias module:modeling/geometries/geom3.createConvex
 */
const createConvex = (listofpoints) => {
  if (Array.isArray(listofpoints) && listofpoints.every((p) =>
       (Array.isArray(p) && p.length===3 && p.every((c) =>
         (typeof(c)==="number"))))) {
           return hullGeom3([[ create(),
             fromPoints(listofpoints.map((p) => [p,p,p])) ]])
  }
}

module.exports = createConvex
hermann@j4105:~/OpenJSCAD.org/packages/modeling/src/geometries/geom3$ 

Test script for "npm run dev":

const jscad = require('@jscad/modeling')
const { sphere } = jscad.primitives
const { hull } = jscad.hulls
const { geom3 } = jscad.geometries

function main() {
  let out = []
  for(x=-2;x<=2;++x)
    for(y=-2;y<=2;++y)
     for(z=-2;z<=2;++z)
        if (x*x+y*y+z*z <= 5)
          out.push([x,y,z])
   return hull(out)       
//   return geom3.createConvex(out)       
}

module.exports = { main }

@platypii
Copy link
Contributor

I would do something like this. Also @z3dev what do you think about the name geom3.fromPointsConvex?

const quickhull = require('../../operations/hulls/quickhull')
const create = require('./create')
const poly3 = require('../poly3')

/**
 * Construct a new convex 3D geometry from a list of points.
 * @param {Array} listofpoints - list of points to construct convex 3D geometry
 * @returns {geom3} a new geometry
 * @alias module:modeling/geometries/geom3.createConvex
 */
const fromPointsConvex = (listofpoints) => {
  if (!Array.isArray(listofpoints)) {
    throw new Error('the given points must be an array')
  }

  const faces = quickhull(listofpoints, { skipTriangulation: true })

  const polygons = faces.map((face) => {
    const vertices = face.map((index) => listofpoints[index])
    return poly3.create(vertices)
  })

  return create(polygons)
}

module.exports = createConvex

@Hermann-SW Hermann-SW closed this by deleting the head repository May 25, 2024
@Hermann-SW
Copy link
Contributor Author

I created new #1344 based on @platypii last comment.

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