Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/COMPAS FAB GH components #249

Merged
merged 77 commits into from
Feb 23, 2021
Merged

Feature/COMPAS FAB GH components #249

merged 77 commits into from
Feb 23, 2021

Conversation

romanarust
Copy link
Member

@romanarust romanarust commented Nov 25, 2020

What type of change is this?

Draft for COMPAS FAB GH components made with the Python GHPY compiler.
Based on the Tutorial by Giulio Piacentino, which provides a transparent way of dealing with future versions of this components.

For review, you have to run src\compas_fab\ghpython\components\compile.py from Rhino's PythonScriptEditor.
The library is then copied into 'USER\AppData\Roaming\Grasshopper\Libraries'

If you open Grasshopper, you'll see something like the following (icons need to be updated of course):
Capture

The robot_playground.ghx is updated with these components.

  • 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.rst 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_fab.robots.Configuration.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good!!
I added a couple of comments here and there, besides that, it seems the linter is complaining about some stuff, and I would add a short bit of documentation somewhere indicating the compilation process. Ideally also add a def in tasks.py so that it can be triggered like all other dev tasks.

import System

assembly_name = "COMPAS FAB"
assembly_version = "0.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe best here would be to use the same string we use for the overall compas_fab version. The way to do that is to 1) use the current version here, and 2) add this file and search/replace condition to the .bumpversion.cfg file on the root of the repo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed bumpversion and added a few lines in 'CONTRIBUTING.rst'. But would need some help on adding a task and the naming of that task.
I created a file named install.py in src\compas_fab\ghpython\components\ that should be executed.

src/compas_fab/ghpython/components/compile.py Outdated Show resolved Hide resolved
os.path.join(appdata, 'Grasshopper', 'Libraries') # TODO: must move into compas_ghpython

src = os.path.join(os.path.dirname(__file__), filename)
dst = os.path.join(r'C:\Users\rustr\AppData\Roaming\Grasshopper\Libraries', filename)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have functions in compas_rhino.__init__ that can help you retrieve this folder dynamically.

@@ -0,0 +1,7 @@
import System

o = "iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAIlSURBVEhL7VM7aFRREN0mYqWIYLHkztzPW5UVQVw/+CNoYZNGC1EsRfxWFoIKQgqxUBDRThS0kbSJJqKCjSgIKdIFf6AWUQSttLCKZ+47q9n1hY1dihwYHnPmP3debRELH2vr9ZXJ+bGkfqa36Jvo4iaG9gYCds8Kvh5Uj0BOJpHTbSlUTxkP+wX4TZtvVD3HFHMjpeTMOTj/jNS8wEIzhYQDpKqBLkaCk19U/wtB/CMU+Vl4v5fUX6xxri4iKyw5ujmfnFuHYrei+EuQy+CuJAlHY4zLGfIPkHjApkji75MqgXW8LA2yEd/vQWQQ+72RuQqxogztAIqvos8EKSRX/y4Hqe43nQkuZmMXms3mEtiulf7+Nuk/sEtirtHsjI7vGmHroY8VfAFummolsK59THSCVAbO+o7xyfuDNZzZmezU1W0IYQuDr5KqBNY4jEIPqJo+yHxnMwHj81ytPxWZmIXo/XGz4R3e2upCf1iPaVv4bm54v9V0NLIaK9kZRfbA9yH97zFF3vWEkfYwpDqAhNth/2Y+VWLXVl5cqXevy+79Jg2HSFXC3gdd7sDE26x767ohEhuqG2y6qg1kFCJNPMoUdveJ1LxRFMUyxH3FvR8jVQ37mfKIIq9I9YR1XE7uf6jqUtJzI7iwq73H5GQSXzyYPoE8bQtW9BjTjsP+gQ29timYojdarVYfEg3hH3iPRJ9zok75CPkCmcSlHGbYIhY0arXfMz6/PVGPpY0AAAAASUVORK5CYII="
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a build step, it would probably make sense to have these icon files as files and create the base64 encoded string at compile-time.


ros_client = st.get(key, None)
is_connected = ros_client.is_connected if ros_client else False
return (ros_client, is_connected)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it more complicated to return this tuple instead of just ros_client, considering is_connected is also available on ros_client.is_connected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning the bool as well is practical from a user perspective: you can simply add a textfield (panel) to that outlet, see below:

image

src/compas_fab/ghpython/components/ros_robot.py Outdated Show resolved Hide resolved
@romanarust
Copy link
Member Author

Alternatively, compas core could gain the ability to know how to install GH components instead, and then we would only need to provide the component locations.

I think that would be the way to go. I will open a draft PR on compas to get that started in parallel.

As a third alternative, we could add for now an entry-point like compas_fab.ghpython.install and do this completely in here. If we go that route, it would be a temp solution until we move it to core.

Yes, this as temp solution that sounds right. Could you @gonzalocasas add this?

@gonzalocasas
Copy link
Member

Yes, this as temp solution that sounds right. Could you @gonzalocasas add this?

yep, will do

@gonzalocasas gonzalocasas marked this pull request as ready for review February 22, 2021 13:14
Base automatically changed from master to main February 23, 2021 13:56
@gonzalocasas gonzalocasas merged commit 86bfdcb into main Feb 23, 2021
@gonzalocasas gonzalocasas deleted the feature/ghuser_objects branch February 23, 2021 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants