jscad / OpenJSCAD.org

JSCAD is an open source set of modular, browser and command line tools for creating parametric 2D and 3D designs with JavaScript code. It provides a quick, precise and reproducible method for generating 3D models, and is especially useful for 3D printing applications.
https://openjscad.xyz/
MIT License
2.58k stars 505 forks source link

fix(modeling): boolean argument checks #1293

Closed platypii closed 9 months ago

platypii commented 10 months ago

A user on discord had a question, and I realized that his design revealed a weird behavior in union when invalid geometries are given. Simplified user code:

return union(vectorText({ input: 'I' }), vectorText({ input: ' ' }))

Rather than throwing an error in union, this actually returns the value 4. Weird.

It makes sense when you look at the code. vectorText returns an array of points like:

vectorText({ input: 'I' }) => [[[4, 21], [4, 0]]]

Which gets flattened by union, and then union doesn't know how to deal with non-geometries so it just returns the first element from the flattened list which is the number 4. Very weird.

I don't like throwing errors if we can avoid it. But in this case, it feels like the right thing to do is to tell the user "hey this is not a valid geometry" as soon as possible, to make debugging easier. So in this case I added throw new Error('union unsupported geometry type')

I added tests for this behavior. I also tried to make the error messages easier to understand and with better grammar, but open to suggestions here.

All Submissions:

z3dev commented 10 months ago

Trash in trash out. I'm surprised that the example didn't crash.

z3dev commented 10 months ago

Also, V3 vectorChar and vectorText return different results now. Are you sure this still happens with V3?

platypii commented 10 months ago

The specific example here would probably not happen with V3 vectorText because you changed it, but the behavior would remain kind of weird. If you have the above example in V3, since union doesn't support path2, I think it would just return the FIRST text but discard the second text.

I think this PR would still be better in that case so that users know that we don't support unioning paths (yet).

z3dev commented 9 months ago

@platypii these changes seem reasonable. but there's an issue raised in issue #820 which seems both reasonable and possible. So, I'd really like to see some changes for...

  1. how to handle empty lists in the API
  2. how to handle non-supported types in the API
  3. how to handle sparse lists in the API
platypii commented 9 months ago

@z3dev I am happy to make some suggestions for empty lists, sparse lists, and invalid geometries in general. I have some ideas.

But I would prefer to put up a separate PR after these changes are merged, if you agree with these changes. I can have another PR up with my suggestions for the rest of the cases in the next few days. I think some of the changes for sparse lists might warrant their own discussion, and I don't want this PR to get huge.