SketchUp / api-issue-tracker

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

Add better examples for `entities.intersect_with` #741

Open thomthom opened 2 years ago

thomthom commented 2 years ago

This method is not easy to understand.

Yet another demonstration of that: https://forums.sketchup.com/t/how-can-i-understand-entities-intersect-with-method/182335

We should add more complete code examples. It might be challenging to do a fully functional example in the Ruby API docs without a lot of setup code to generate the geometry. But we can do better than the current one-liner.

We can add better example/tutorial to the example/tutorial repo and link to it from the docs. (https://github.com/SketchUp/sketchup-ruby-api-tutorials)

DanRathbun commented 2 years ago

One of the main reasons why the doc is confusing is the naming of the arguments.

For example, transform1 is the transformation for the receiver entities and transform2 is the transformation for entities1

It would be better if either the argument names matched ...

... or were more descriptive. ie:

~

CAUL2000 commented 2 years ago

The documentation is very frustrating. There are three entitites collections and two transformations so the two transformations must embed the connections between all three. I think an example like this would be quite good in the docs:

  def intersect(cutter_group, mesh_group, result_group)
    # transformation from mesh to cutter
    tr0 = cutter_group.transformation.inverse * mesh_group.transformation
    # transformation from result to cutter
    tr1 = cutter_group.transformation.inverse * result_group.transformation
    mesh_group.entities.intersect_with(false, tr0, result_group.entities, tr1, true, cutter_group.entities.to_a)
  end
DanRathbun commented 2 years ago

Here's another example:

  # @example  In a model whose top level entities has 2 intersecting groups:
  #   model  = Sketchup.active_model
  #   groups = model.entities.grep(Sketchup::Group)
  #   group  = groups.first
  #   group2 = groups[1]
  #   recurse = false
  #   hidden  = true
  #   ents = group.entities
  #   result_group = model.entities.add_group
  #   ents.intersect_with(
  #     recurse, group.transformation, result_group.entities,
  #     result_group.transformation, hidden, group2
  #   )
CAUL2000 commented 2 years ago

Interesting example since it works, is more compact then my proposed solution and clearly violates the docs which states that the last parameter is:

entities2 (Sketchup::Entity, Array<Sketchup::Entity>) — A single entity, or an array of entities.

It seems that if you send in a container instead of an entities collection Sketchup picks the transformation from the container. It makes perfect sense but there is no mention of it in the documentation.

CAUL2000 commented 2 years ago

Now the documentation actually makes sense - it's just that it is exactly wrong. With entities2 being an entities collection the comment for the transformations should clearly state that they are transformation INTO entities2. The current wording is only valid for the case in @DanRathbun's example where the last parameter is a container.

CAUL2000 commented 2 years ago

After a night's sleep I realized that a group is an entity so the documentation isn't exactly wrong, it's just exactly confusing. It should clearly separate the more general case where entities2 is an entities collection and the given transformations has to embed the path into this collection and the narrower case where entities2 is a group in which case Sketchup automatically prepends group.transformation.inverse to the other transformations.

DanRathbun commented 2 years ago

After a night's sleep I realized that a group is an entity ...

Well I debated stating the obvious. Yes, everything that resides in the model dB's collections are instances of subclasses of Entity. Those members of Entities collections are subclasses of Drawingelement* a direct subclass of Entity.

* Except Vertex instances which don't use the rendering and visibility control methods that the other Drawingelement subclasses need.

... so the documentation isn't exactly wrong, it's just exactly confusing.

I think this is subjective. It depends upon the readers level of experience with Ruby, OOP (in general) and YARD generated documentation in particular.

So it could range from "a little bit confusing" to "somewhat confusing" to "pulling your hair out". ;) (yea, I'm teasin')

It should clearly separate the more general case where entities2 is an entities collection ...

Seriously, though, ... I never read the documentation this way. Ie, it didn't seem (to me) to imply that entities2 could be an an Entities collection object just because entites1 must be and the parameter names have the same prefix.

BUT ... rereading the method description, it does clearly state ... "to intersect an entities, component instance, or group object" ... with the receiver "entities object". However, the parameter list for the last argument (entities2) omits listing an Entities collection object as allowable.

I just tested using this as the last statement ...

    ents.intersect_with(
      recurse, group.transformation, result_group.entities,
      result_group.transformation, hidden, group2.entities
    )

... and got this exception:

Error: #<TypeError: wrong argument type (expected Sketchup::Entity)>
<main>:1:in `intersect_with'

So, the last argument cannot be an Entities collection object ! And in your test @CAUL2000, you did not either. You used a plain ol' Array of Entity objects.

What the doc does not imply is that the last argument can also be a single primitive like an edge.

So the description should read:

The term "entity" is this context and many other places in the documentation actually means Drawingelement instance objects.


We who've been using (and helping to improve) the SketchUp API documentation for going on 15 years (in it's several forms) just take for granted that the lower case word "entities" in the text can refer to either an Array of Entity objects or an actual Entities collection object depending upon context.

If the context is not clear enough then those new to the concept or in this case this method, can be easily confused if the explanation is too terse. Sometimes a guru (writing) technical descriptions doesn't realize that the "common knowledge" that they take for granted isn't yet understood by novices who will be reading those words.

Anyway .. the #intersect_with method must be vying for the "Most Confusing Method" award in SketchUp API history. There have been many long forum topics about this method. (A forum search produces over 50 hits in just the Developer categories. And I haven't checked SketchUcation.)

~

CAUL2000 commented 2 years ago

I've always interpreted the last argument as an entities collection and the thrown error as a bug, but when looking closely that's a bug on my part. Nevertheless - for the method to be general and work with three contexts arbitrarily placed in the model hierarchy the last argument has to be an array of raw entities and the transformations have to embed the relation between the three contexts explicitly. For this case (which I until now thought was the only one....) the docs are simply annoyingly unhelpful since they do not state that the context of the intersection is entities2 (one would think that it should be in the context of this) and that the two transformations should both go TO entities2 (the logical thing would be for the result to be embedded as a transformation from the cutting context to the result context).

DanRathbun commented 2 years ago

Could be why I noticed a difference in the placement of the result_group between having the last argument as group2 and group2.entities.to_a.

I think the former put the intersecting edges exactly where they intersected, and the latter had the result_group at the model origin.

... the docs are simply annoyingly unhelpful ...

Agree. That one-line description doesn't do much for understanding the quirks or power of this method.