Revolves about the z axis are calling the engine with the incorrect arguments.

[mlfarrell]

See this snippet derived from an engine issue that paul posted in engine repo

startSketchOn("YZ")
  |> startProfileAt([1, 0], %)
  |> bezierCurve({
    control1: [1, 0],
    control2: [9, 0],
    to: [10, 10],
  }, %)
  |> line([1, 0], %)
  |> line([0, -10.25], %)
  |> line([-11, 0], %)
  |> close(%)
  |> revolve({ axis: "Z", }, %)

the is_axis_2d: true flag (revolve.rs: 281) will cause the engine to interpret the axis in the coordinate system of the 2D plane itself. That has its conveniences, but it seems like you guys are trying to interpret it as the 3D axis. using local-z here will get you an extrude thats about y, which causes this example to break.

What you should see is:

but instead, you get a revolution that makes no sense.

@jessfraz i'm tagging you on this since you last worked on the revolve.rs code.

[jessfraz]

oh seems like we should just set that to false, @nadr0 was looking at this as well

[jessfraz]

same issue https://github.com/KittyCAD/modeling-app/issues/2540

[jessfraz]

@nadr0 you just wanna swap that bool to false? EDIT: nevermind see comments below

[mlfarrell]

i think so. but i'm worried about other stuff breaking. i'm looking at https://github.com/KittyCAD/engine/issues/2584 now which I thought this would fix, but it didn't. will update on that soon.

[mlfarrell]

we just have to be extra clear about what axis we're using and if its the 2D one or not

[jessfraz]

we should make that user settable too if its not already

[mlfarrell]

yea like even see jordan's own wording here

It should be able to perform a revolve around the sketch plane's Y axis to make a nozzle/cone looking shape.

so there will be a bunch of confusion if we change it

[jessfraz]

so we keep it to true but let the user override, makes sense?

[mlfarrell]

totally. i think that's the move. and then help explain it when newcomers get confused. i.e. thats what I should go back to paul with. "you didn't really want z here, unless axis_is_2d: false"

[Irev-Dev]

I kinda think axis_is_2d: true was always the right call (if I understand it correctly), and giving user's the ability to override it is giving users a foot gun with no upside. Because the revolve axis has to be on the same plane as the sketch they are trying to revolve, defining things in 3d just gives them an extra DOF that makes it easy for them to get an engine error, and no extra ability to define the axis where they want it.

I.e. this API for defining a custom axis

const part001 = revolve({
  axis: {
    custom: {
      axis: [0.0, 1.0, 0.0],
      origin: [0.0, 0.0, 0.0]
    }
  }
}, sketch001)

I think should be

const part001 = revolve({
  axis: {
    custom: {
      axis: [0.0, 1.0],
      origin: [0.0, 0.0]
    }
  }
}, sketch001)

That is, defining a 2d origin on the sketch plane of sketch001 and a 2d unit vector on that plane too.

[mlfarrell]

good points, Kurt :+1:

[jessfraz]

okay so are you saying dont let users override because that certainly makes it easer kurt haha

[jessfraz]

thats fair when it comes to the custom plane

[Irev-Dev]

Yeah I think the api should be

// custom plane (2d origin and axis)
const part001 = revolve({
  axis: {
    custom: {
      axis: [0.0, 1.0],
      origin: [0.0, 0.0]
    }
  }
}, sketch001)

// X or Y, (because it's referring to the 2d plane)
revolve({ axis: "X", }, %)
revolve({ axis: "Y", }, %)
// error for revolve({ axis: "Z", }, %) but with a good message

// using tags
const part001 = revolve({
  axis: getOppositeEdge(revolveAxis)
}, sketch001)

One thing worth mentioning is that revolve({ axis: "X", }, %) the "X" can be a source of confusion for sure, maybe if we change it to "planeX" and "planeY" that would be better?

[jessfraz]

what if we name axis to plane instead I just think it’s weird we use X YZ for planes in startSketchOn and this seems inconsistent to not have the same strings

[Irev-Dev]

Yeah that works for me, maybe plane or maybe planeAxis?

[nadr0]

Yeah my PR https://github.com/KittyCAD/modeling-app/pull/3907 is supposed to address this.

I thought about changing it but when I had a discussion in slack about it no one felt strong about changing the keyword. The documentation will be updated properly in my PR, someone can at least check the docs and realize Z is not allowed for axis.

If we do go with a rename we will need to make sure any other command follows the same pattern.

[nadr0]

the is_axis_2d: true flag (revolve.rs: 281) will cause the engine to interpret the axis in the coordinate system of the 2D plane itself.

what if we name axis to plane instead I just think it’s weird we use X YZ for planes in startSketchOn and this seems inconsistent to not have the same strings

const DefaultPlaneStrs = ['XY' , 'XZ' , 'YZ' , '-XY' , '-XZ' , '-YZ'];

I don't think it is inconsistent, when using startSketchOn the coordinate system has 2 unknown dimensions. If you say X you need to pick another dimension for the plane aka XZ. When in revolve the system has already picked your local plane of XY for you. When you do a revolve you are picking on of the axis within the local plane.

[Irev-Dev]

I agree with you Kevin, I think axis within the local plane. is the correct mental model, it just it's possible that some users will come into it thinking "global XYZ", but I think if we're able to have a good error for axis: "Z" that should be enough to catch that bad mental model and correct it.

[nadr0]

Yup, I agree that having better error handling and feedback to the user will allow them to easily correct the mistake.

[jessfraz]

i think we are in a better place now after teh mirror2d enhancements for this but let me if not, closing