OpenJSCAD.org icon indicating copy to clipboard operation
OpenJSCAD.org copied to clipboard

feat(modeling): allow zero size in primitives

Open platypii opened this issue 2 years ago • 7 comments

This PR allows primitives to "scale to zero" in size. Specifically, for rectangles and cuboids allow size equal to zero. And for ellipses and cylinders allow radius equal to zero.

Previously these primitives would throw an exception if the size was zero. This leads to a bad user experience in cases where you have a parametric design that has a parameter that might scale to zero and disappear. Right now, you need awkward special cases in your code to handle that (a user raised this issue on discord recently).

Another example where this is helpful: Using a slider parameter, today if you slide it to zero you get an exception and the design does not update on the screen. After this PR, you can scale these to zero, and it works fine:

const { circle, cube, cylinder, square } = require('@jscad/modeling').primitives

const getParameterDefinitions = () => ([
  { name: "size", type: "slider", initial: 1, min: 0, max: 10 }
])

const main = ({ size }) => {
  return [
    circle({ radius: size, center: [0, 0, 0] }),
    cylinder({ radius: size, center: [5, 0, 0] }),
    cube({ size, center: [10, 0, 0] }),
    square({ size, center: [15, 0, 0] })
  ]
}

module.exports = { main, getParameterDefinitions }

All Submissions:

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [x] Does your submission pass tests?

platypii avatar May 30 '22 19:05 platypii

Not just those questions!

Why would a design want zero (0) sizes on rectangles, etc? What's the use case?

Also, sliders only work on integers, so one(1) is the smallest viable size. Again, why start at zero(0) for sliders? What's the use case?

z3dev avatar Jun 01 '22 12:06 z3dev

This leads to a bad user experience in cases where you have a parametric design that has a parameter that might scale to zero and disappear. Right now, you need awkward special cases in your code to handle that (a user raised this issue on discord recently).

:100: agree with this

In my own app, I had to litter my code with many if conditions because some circle's radius would become <= 0 depending on a user-specified parameter.

Having these checks in jscad would simplify application code, while not significantly impacting performance.

t1u1 avatar Jun 01 '22 17:06 t1u1

Also, sliders only work on integers, so one(1) is the smallest viable size.

There is no reason you can't set the slider minimum to 0, I gave an example in the original post.

What's the use case?

One of the reasons for parametric designs is to be able to experiment, and try out different options. Or maybe be able to generate designs with optional features.

Currently it is awkward to make a design where an element can "scale to zero and disappear". You would either need special handling in the code (if (size > 0) objects.push(...)). Or you would need a seperate parameter to enable or disable a design feature (eg- a check box). These are awkward workarounds, and users on discord have complained about them. And I agree.

There are choices of how JSCAD could handle zero sizes: throw an exception, or quietly produce an empty geometry. I feel that we should throw errors for things like negative radius, because it is unclear what that even means. But for radius zero, I think it is MUCH more user friendly to just produce an empty geometry.

do we allow scale=0 ?

Yes, today we allow scale 0

should transform or colorize or other functions have some special handling of those zero size primitives

I don't see why they need special handling? They work fine today on empty geometry. It was always possible to have empty geoemetries (eg- intersection of nonoverlapping cubes)

What's the use case?

Here's a parametric crown with re-sizable point balls. This code would be more elegant with this PR change:

const jscad = require("@jscad/modeling")
const { subtract } = jscad.booleans
const { cylinder, sphere } = jscad.primitives
const { rotateX, rotateZ, translate } = jscad.transforms

const getParameterDefinitions = () => ([
  {name: "size", type: "slider", min: 0, initial: 1, max: 10}
])

const main = ({ size }) => {
  const segments = 100
  const cutout = translate([0, 0, 6], rotateX(Math.PI / 2, cylinder({ height: 20, radius: 6, segments })))
  const ball = sphere({ center: [10, 0, 2.5], radius: size / 10 })
  return [
    subtract(
      cylinder({ height: 10, radius: 10, segments }),
      cylinder({ height: 10, radius: 9.8, segments }),
      cutout,
      rotateZ(Math.PI * 2 / 3, cutout),
      rotateZ(Math.PI * 4 / 3, cutout),
    ),
    ball,
    rotateZ(Math.PI * 1 / 3, ball),
    rotateZ(Math.PI * 2 / 3, ball),
    rotateZ(Math.PI * 3 / 3, ball),
    rotateZ(Math.PI * 4 / 3, ball),
    rotateZ(Math.PI * 5 / 3, ball),
  ]
}

module.exports = { main, getParameterDefinitions }

crown

platypii avatar Jun 01 '22 17:06 platypii

I feel that we should throw errors for things like negative radius, because it is unclear what that even means.

Ah ok. I just now realized that even after this PR, negative radius still needs to be handled in app code. That's fine; if this PR gets merged, I will be able to write something like radius = maximum(0, computedRadius). Earlier, I had to write radius = maximum(epsilon, computedRadius) or add if conditions to eliminate zero radius circles, and both options were not very elegant.

t1u1 avatar Jun 01 '22 17:06 t1u1

I think we are actually taking about the same thing. Designs need to validate parameters. Of course, the 'defaults' are useful to control the UI but users can still input almost anything.

There have been a few requests in the past to add validations. One was a request for general validation, adding validation per parameter, or a general validation function.

https://github.com/jscad/OpenJSCAD.org/issues/49

Another was about validating the relationships between parameters, i.e. parameter A should be larger than parameter B, etc.

https://github.com/jscad/OpenJSCAD.org/issues/85

Also, there was a nice PR for creating dependencies between parameters as well, but against V1. Something like this would add some nice flexibility to the parameter definitions.

https://github.com/jscad/OpenJSCAD.org/pull/42

z3dev avatar Jul 24 '22 00:07 z3dev

Just continuing, it would be best if the validation of parameters was performed within the UI, rather then passing bad parameters into the main design logic. So, maybe another predefined function needs to be called, which can perform the validations and throw an appropriate error.

z3dev avatar Aug 27 '22 02:08 z3dev

Or just thow a ValidationError from main with info on which fields failed and message for each

hrgdavor avatar Aug 27 '22 08:08 hrgdavor

@platypii are you still looking to make these changes? V2 or V3?

z3dev avatar Apr 17 '23 00:04 z3dev

@platypii are you still looking to make these changes? V2 or V3?

I still strongly believe this is a good change. It makes the designs more robust, for cases where size zero makes logical sense, it just makes it easier for users to "play" with designs more freely. This makes parameters much more useful.

I just rebased it onto V3 branch. How do feel about it as a V3 change @z3dev?

platypii avatar Apr 22 '23 06:04 platypii

@platypii thanks.

I reviewed the changes again, and came to the same conclusion that these are really improving the usability of the API.

Making these changes to V2 is fine. I'll review seriously. ;)

z3dev avatar Apr 22 '23 12:04 z3dev

@z3dev I re-rebased it onto V2 master branch again. Please review :-)

platypii avatar Apr 23 '23 20:04 platypii

@z3dev good call. I added similar logic and tests for zero size for rounded primitives. Star has a confusing relationship of inner and outer radius, so I left it alone.

platypii avatar May 06 '23 22:05 platypii