speckleworks / SpeckleCore

Check a brand new Speckle at: https://github.com/specklesystems
https://speckle.systems
MIT License
38 stars 17 forks source link

SpeckleCircle needs X-axis and Y-axis properties for aligning planes #81

Closed secretlyagoblin closed 6 years ago

secretlyagoblin commented 6 years ago

Expected Behaviour

Start point on a circle domain should be based on the plane the curve has been defined in

Actual Behaviour

When circles are created on twisted frames, before translation evalutation correctly picks up curve start, afterwards, the start point defaults to standard coordinates.

create circle

alvpickmans commented 6 years ago

@secretlyagoblin as you suggest the circle schema (and arc by extension) seems to require some other property for better definition. I run into something similar while implementing arcs on the dynamo converter

Rather than 2 vectors I thought more of having the start point as property as such would be slightly lighter and easier to re-construct on the target platform(?)

@didimitrie

secretlyagoblin commented 6 years ago

all the same to me! point + normal might be more useful for reducing error as they're further apart. Worth noting that Ellipse and Arc are derived from a xy axis.

didimitrie commented 6 years ago

More than happy to discuss these things! I've been running around in circles trying to fix these issues - pun intended.

SpeckleArcs do have a plane property, which basically includes X, Y and Z (normal) dirs. I know there have been problems (https://github.com/speckleworks/SpeckleRhino/issues/170, https://github.com/speckleworks/SpeckleCore/issues/68).

@alvpickmans: how are we doing for arcs from rhino/gh in dynamo at the moment? I remember I fixed this issue theoretically.

@secretlyagoblin: would the plane be ok for circles? I guess this would give you the control you need.

@all: the big fuckup previously with arcs and rhino was the domain and how it's specified. Adding this to arcs resolved https://github.com/speckleworks/SpeckleRhino/issues/170, but not really sure what's our status on this one.

alvpickmans commented 6 years ago

afaik although both Rhino and Dynamo have circle constructor based on Plane and Radius, Dynamo only uses the plane's origin as its centre building the curve from the positive vector from the origin parallel to XY plane (rigth side if counterclockwise), while Rhino from the circle's plane X axis (starting points in blue on pics below). The same applies to arcs, so in Rhino the start point is on the plane's X axis and start angle 0 while in Dynamo can be any.

Quickly checking three.js api , it considers the starting point same as Dynamo. It would be good to know how other possible clients handle this (Unity, Blender...), and define SpeckleCircle/Arc schemas according to what would be easier to implement.

RhinoCircle

DynamoCircle

didimitrie commented 6 years ago

then ping @secretlyagoblin and @cwmorse regarding unity circles and @tsvilans for blender circles.

It might also be the case that this level of control is not needed in Blender/Unity/Threejs, and we need to nail it down for now Dynamo - Gh/Rh.

@alvpickmans, talk more irl tomorrow, but we can have start points added too to circles and arcs.

secretlyagoblin commented 6 years ago

@didimitrie yep the plane should be fine, thanks! I'm using verbnurbs and it considers a circle to be a special case arc, which is probably a good way of thinking about it.

@alvpickmans that almost feels like bug on the part of Dynamo! I can't imagine that being preferred behaviour.

alvpickmans commented 6 years ago

@secretlyagoblin I agree that makes sense to treat circles as "closed arcs". I wouldn't say it is a bug, probably just a different interpretation of the geometry, just like nurbcurves are defined differently (great old post here). In any case, there is no harm in asking Dynamo/Autodesk :)

didimitrie commented 6 years ago

I got in this when i reduced lines to a polyline with only two point coords in its array. Had to bring lines back because of public outcry.

So would keep arcs as arcs on the speckle side for now...

didimitrie commented 6 years ago

image

okay this is sorted now! I've remade a quick def to spin up some circles. will be closed soon...

secretlyagoblin commented 6 years ago

chefs kiss perfect!

alvpickmans commented 6 years ago

Made it work in Dynamo as well! It was indeed a bug on its geometry library, but it is resolved until they release the fix with version 2.1. This can be closed now. image