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

RobotModel SceneObject with Python3 in Rhino8 #1356

Merged
merged 5 commits into from
May 29, 2024

Conversation

gidodielemans
Copy link
Contributor

@gidodielemans gidodielemans commented May 14, 2024

Rhino Python3 compatibility

What type of change is this?

Robot scene objects were not working in the Python3 version of Rhino8
Python3 does not have access to the System module

  • Bug fix in a backwards-compatible manner.

Checklist

  • 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)

Python3 compatibility
@jf---
Copy link
Contributor

jf--- commented May 14, 2024

hi @gidodielemans,

could you introduce the interesting contribution a little?

something that comes to mind is that a conditional code path may seem relevant: the suggested changes might be incompatible with earlier versions?
is PipeCapMode rhino 8 specific?

--

the joy of reading Rhino 8 & python 3 -- so blissful!

@gidodielemans
Copy link
Contributor Author

gidodielemans commented May 15, 2024

hi @jf--- ,

We have noticed that the compas.scene.SceneObject related to the compas_robots.robot.model.RobotModel is not able to be visualized with the Py3 version in Rhino8, the IronPython2 version is able to. Now there are three issues we encountered:

  1. The context was not able to discover the appropriate RobotModelObject, this depends on the context the code is ran in.
    For "Rhino" it should refer to compas_robots.rhino.scene.RobotModelObject, and for "Grasshopper" to compas_robots.ghpython.scene.RobotModelObject.
    (You can change this context using the "scriptcontext.doc = Rhino.RhinoDoc.ActiveDoc" or "scriptcontext.doc = ghdoc")

  2. The second was that we were not able to import the compas_robots.ghpython.scene.RobotModelObject from the package.

  3. And lastly, when the two above were resolved, the Python3 implementation does not have access to the System module as it is related to the .NET implementation of IronPython vs. the C implementation of Py3

Now i have to verify registration of the RobotModelObject on a separate machine as maybe this has stuck even though i reverted this part of the code (as it seemingly works without). Though the fix here references step 3.

With regard to your question on PipeCapMode, I do not know if this is a Rhino8 addition, if so, then yes we would need a conditional to check for the Rhino version. My attempt was solely to remove the System dependency.

Hope this helps :)

@gidodielemans
Copy link
Contributor Author

Hi again @tomvanmele,
I merged the latest updates, and ran the tests, formatter and linter.
Seems all green.
Any update on the potential of this to be merged?

Thanks for the feedback!

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

cool, LGTM

@tomvanmele tomvanmele merged commit bb60abc into compas-dev:main May 29, 2024
15 checks passed
@tomvanmele
Copy link
Member

done :)

@gonzalocasas
Copy link
Member

@gidodielemans ah! You should add yourself to the author's list! Maybe send another tiny PR to add you?

@gidodielemans
Copy link
Contributor Author

@gidodielemans ah! You should add yourself to the author's list! Maybe send another tiny PR to add you?

If you insist, I didn't think I deserved credit for such a tiny change.
I'll be sure to send more substantial requests in the future to make it count!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants