SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

The method #transform_entities does not work on vertices that are used in curves #866

Open indie3d opened 1 year ago

indie3d commented 1 year ago

It seems that the method #transform_entities does not work on vertices that are used in curves. if you have a circle made out of a curve, it transforms the vertex in the wrong direction. Seems it is using another axis that is different than the group axis.

To test the code below select a face where the edge is a curve.

model = Sketchup.active_model
entities = model.active_entities
selection = model.selection
vertices = selection.grep(Sketchup::Face).map(&:vertices).flatten.uniq

vec = Geom::Vector3d.new(0,0,1.m)
tr = Geom::Transformation.new(vec)
entities.transform_entities(tr, vertices)

To fix this in the next code example I explode the curves. However, someone might want to preserve the curve and not destroy it.

I would like to know if this is a BUG or if it is normal?

model = Sketchup.active_model
entities = model.active_entities
selection = model.selection
vertices = selection.grep(Sketchup::Face).map(&:vertices).flatten.uniq

# Explode curves if any
vertices.each do |vertex|
  vertex.edges[0].explode_curve
end

vec = Geom::Vector3d.new(0,0,1.m)
tr = Geom::Transformation.new(vec)
entities.transform_entities(tr, vertices)

More info on the following forum post: https://forums.sketchup.com/t/can-not-transform-vertex

thomthom commented 1 year ago

A Curve or ArcCurve?

ArcCurves understandably doesn't work well if you try to move its vertices. But I would expect non-ArcCurves to work.

Do you have a sample file to go along with this? (Or a snippet that creates sample geometry to go with this?)

DanRathbun commented 1 year ago

ArcCurves understandably doesn't work well if you try to move its vertices.

It should work if:

(a) it is a translational transform by vector moving each vertex in the same direction by the same amount,

(b) it is a rotational transform moving each vertex (with respect to each other) about the same axis by the same angle,

(c) it is a uniform scaling transform (no shear) that preserves the arc-ness of the curve,

... or ...

(d) it is a non-uniform scaling transform (with shear) that converts the ArcCurve to a Curve object.

DanRathbun commented 1 year ago

Do you have a sample file to go along with this?

The originator who found this issue posted a sample model with a group containing a cylindrical solid in the linked public forum post.

Fredosixx commented 1 year ago

I think this is by design. Curves move as a whole, unless exploded.

You have however the method Curve#move_vertices to move explicitly the vertices of a curve without breaking its Curve property. That's what I use for deforming splines.

DanRathbun commented 1 year ago

No doubt it was by design. But two things:

(1) Basically the Entities#transform_entities() method could have "more smarts" with respect to curves as described above, so that all the transforming is done in one operation.

(2) Curve#move_vertices makes the coder do most of the "smart work". It takes a result point array rather than a transformation so the coder must do something like:

points = curve.vertices.map(&:position).each {|pt| pt.transform!(tr) }
curve.move_vertices(points)

This implies that the vertices to transform must be separated by those belonging to curves and those not. This then means that the manipulation would be done in two or more operations, one using Entities#transform_entities(), the others using Curve#move_vertices. This might result in unwanted triangulation of faces because of the multiple operations.

DanRathbun commented 1 year ago

You have however the method Curve#move_vertices to move explicitly the vertices of a curve without breaking its Curve property.

g.skp.zip

I just tested this in the issue model and it does work.

I also do not see any unwanted face triangulation. Maybe because there are no non-curve edges transformed in another sub operation?

# Select the tube group, then ...
model = Sketchup.active_model
grp = model.selection[0]
vec = Geom::Vector3d.new(0, 0, 1.m)
tr  = Geom::Transformation.new(vec)
edges  = grp.entities.grep(Sketchup::Edge)
curves = edges.map(&:curve).compact.uniq
model.start_operation("Transform Entities",true)
  curves.each do |curve|
    vertices = curve.vertices
    points = vertices.map(&:position)
    destinations = points.map {|pt| pt.transform(tr) }
    curve.move_vertices(destinations)
  end
model.commit_operation

NOTE: The originator of the public forum topic has chosen the above snippet as the solution to the original coding challenge.

DanRathbun commented 1 year ago

@thomthom The docs should have a @see Curve.move_vertices YARD tag for Entities#transform_entities(). Perhaps a NOTE with some of the knowledge discussed in this thread (especially the silent fail returning true) ?

DanRathbun commented 1 year ago

This issue should at least get a "documentation" label.

thomthom commented 1 year ago

The originator who found this issue posted a sample model with a group containing a cylindrical solid in the linked public forum post.

It really helps us to get the relevant information in the issue description itself. Makes the scrubbing and tracking process a lot easier. The description text of the GitHub issue is copied into the internal ticket we log.

DanRathbun commented 1 year ago

With regard to posting the test model file ...

@thomthom It really helps us to get the relevant information in the issue description itself.

I reposted the "g.skp" test model from the original forum post in this issue thread at post 7, ... ... but as I am not the originator so I cannot "parrot" it into the 1st post.

@indie3d (from your initial post): More info on the following forum post: https://forums.sketchup.com/t/can-not-transform-vertex

This link fails as it was a duplicate which has been deleted from the forum.

The following topic is still valid: https://forums.sketchup.com/t/how-to-transform-curve-vertices/218784