Update compas_rhino artists #642

tomvanmele · 2020-10-22T15:21:41Z

adding draw_collection to box artist

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner.
Breaking change: bug fix or new feature that involve incompatible API changes.
Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

adding draw_collection to box artist

tomvanmele · 2020-10-22T15:22:56Z

i just updated BoxArtist for now. perhaps you can review this one first until we fully agree on the format for the shape artists, and then i roll this out for all others on this PR...

tomvanmele · 2020-10-22T15:26:12Z

just a few points for discussion:

should the faces of the boxes in the collection be grouped into joined meshes?
should the collection function also provide the option to show/hide other components than just the faces

gonzalocasas

Please add a line to CHANGELOG.
Otherwise it LGTM!

About your questions: boxes are always closed, right? so I'd tend to assume it would display it as one mesh rather than individual faces, but I realize the usage and assumptions on this point changes a lot between teams, so maybe add the join_faces option like the MeshArtist has.

Regarding collection functions, having functional parity between single entity and collection would be nice (ie. the same show_* params on both ideally)

tomvanmele · 2020-10-23T07:32:32Z

the reason behind the simplified functionality for the draw_collection function would be that the function is there for convenience and is meant to draw many objects fast, not necessarily withe all possible options, just the defaults. if more control is desired, the user can always use the draw function with artists for the individual boxes... what do you think @gonzalocasas ? otherwise, i am not sure there is much point to the collection variant.

update log

gonzalocasas · 2020-10-23T08:09:27Z

the reason behind the simplified functionality for the draw_collection function would be that the function is there for convenience and is meant to draw many objects fast, not necessarily withe all possible options, just the defaults. if more control is desired, the user can always use the draw function with artists for the individual boxes... what do you think @gonzalocasas ? otherwise, i am not sure there is much point to the collection variant.

Ah! Understood. I personally have never used these _collection variants, perhaps out of laziness or ignorance thou. If you want to try it as it is now, sounds good to me. At some point, we could gauge how much usage they get and if not much, we could get rid of them.

tomvanmele · 2020-10-23T11:42:23Z

src/compas_rhino/artists/capsuleartist.py

+            points = [{'pos': point, 'color': self.color} for point in vertices]
+            guids += compas_rhino.draw_points(points, layer=self.layer, clear=False, redraw=False)
+        if show_edges:
+            raise NotImplementedError


i have left this open for now. the capsule doesn't actually have any edges. they could be generated to be able to visualize the wireframe. in general, the question is what the most relevant information is. i would argue that even showing the vertices is questionable...

Leaving this as an exception permanently results in kind of a strange user experience though. I would tend toward letting the user decide if he wants to see edges and vertices, even if it doesn't make sense. I find pretty senselessness preferable to runtime errors. But if you do choose to leave it this way, please mention in the documentation that it's not implemented.

tomvanmele · 2020-10-23T11:46:14Z

src/compas/geometry/shapes/capsule.py

    @line.setter
    def line(self, line):
-        self._line = line
+        self._line = Line(*line)


while fixing this, i noticed that the capsule has a very different logic than for example the box. it doesn't have a local frame and is defined directly through a line defined by two points. perhaps that should become an alternative constructor. the default, like with the box, should accept a frame and the parameters that define the capsule in that frame. ideally this is the default for all shapes: a frame and parameters. alternative constructors allow to use direct geometrical inputs, but would then have to compute a frame from those internally...

@gonzalocasas @beverlylytle @Licini

the suggestion makes sense to me

will submit a proposal for this via a different PR

tomvanmele · 2020-10-23T11:48:12Z

src/compas_rhino/artists/boxartist.py

        return guids

+    @staticmethod
+    def draw_collection(collection,


the collection methods offer very little advantage over the individual draw methods. since static methods are somewhat frowned upon anyway, perhaps the collection methods are not really necessary...

i tend to agree. it seems that really the only added functionality of draw_collection is the grouping, and it seems like that function should be pulled out, perhaps as a static method to the BaseArtist or maybe somewhere else. it seems reasonable that one may want to group objects of mixed type, ie a box and a mesh or two.

tomvanmele · 2020-10-23T12:41:49Z

src/compas/geometry/shapes/cone.py

    @circle.setter
    def circle(self, circle):
-        self._circle = circle
+        self._circle = Circle(*circle)


while fixing this bug, i noticed that the cone does not have a bottom face. should it?

src/compas/geometry/__init__.py

Co-authored-by: beverlylytle <57254617+beverlylytle@users.noreply.github.com>

tomvanmele · 2020-10-26T08:46:27Z

@gonzalocasas this thing is final now, so please let me know if it can be merged...

adding options to show/hide vertices/edges/faces to box artist

5d1b9cb

adding draw_collection to box artist

tomvanmele requested review from Licini, beverlylytle and gonzalocasas October 22, 2020 15:23

gonzalocasas approved these changes Oct 22, 2020

View reviewed changes

update draw_collection

f67401c

update log

tomvanmele added 5 commits October 23, 2020 10:23

cleaning up

6588520

only top level example

f5bf779

removing join edges option

ffc3361

fix bug in capsule

731fa20

update capsule artist draw function

a9eab15

tomvanmele commented Oct 23, 2020

View reviewed changes

tomvanmele added 2 commits October 23, 2020 13:50

log

8f2d649

updated cone and cone artist

8e6880f

tomvanmele commented Oct 23, 2020

View reviewed changes

tomvanmele added 9 commits October 23, 2020 15:19

added bottom face to cone

301452d

upgrade cylinderartist

d1f39cd

log

f03131e

ngons and polyhedrons

f3ec145

support for edges

2349c66

lint

bb6e0f4

updated sphere artist

dcfdf62

torus update

2ac2e0c

upgrade face drawing to ngon

dcc3cb0

tomvanmele added 2 commits October 25, 2020 21:42

update primitive drawing

9bef1a0

update log

1fedf1f

beverlylytle reviewed Oct 26, 2020

View reviewed changes

src/compas/geometry/__init__.py Outdated Show resolved Hide resolved

Update src/compas/geometry/__init__.py

8acaf2b

Co-authored-by: beverlylytle <57254617+beverlylytle@users.noreply.github.com>

tomvanmele merged commit 7d3e626 into master Oct 27, 2020

tomvanmele deleted the compas-rhino-artists branch January 20, 2021 10:08

Update compas_rhino artists #642

Update compas_rhino artists #642

Uh oh!

Conversation

tomvanmele commented Oct 22, 2020

What type of change is this?

Checklist

Uh oh!

tomvanmele commented Oct 22, 2020

Uh oh!

tomvanmele commented Oct 22, 2020

Uh oh!

gonzalocasas left a comment

Choose a reason for hiding this comment

Uh oh!

tomvanmele commented Oct 23, 2020

Uh oh!

gonzalocasas commented Oct 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomvanmele commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants