compas-dev / compas_fab

Robotic fabrication package for the COMPAS Framework.
https://compas.dev/compas_fab/
MIT License
108 stars 32 forks source link

Fix default acm to empty list if key is present but set to None #269

Closed gonzalocasas closed 3 years ago

gonzalocasas commented 3 years ago

What type of change is this?

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.

gonzalocasas commented 3 years ago

I think you ought to take a look at robot.py too. inverse_kinematics, etc...

Fixed a bunch more of these here: https://github.com/compas-dev/compas_fab/pull/269/commits/c61ff578328a8b0cf7b233059e3686302d016ff8. I hope that's all of them, I did a regex search across the code base for \.get\(.*,.

gonzalocasas commented 3 years ago

robot.py has the line options.get('attached_collision_meshes', []) in inverse_kinematics, plan_motion and plan_cartesian_motion.

that is fixed already, on the first commit. i think ;)

beverlylytle commented 3 years ago

robot.py has the line options.get('attached_collision_meshes', []) in inverse_kinematics, plan_motion and plan_cartesian_motion.

that is fixed already, on the first commit. i think ;)

oh, some how that wasn't the first diff i was looking at.

gonzalocasas commented 3 years ago

Should I revert the second commit with the various other fixes? It smells like more problems than solutions...

beverlylytle commented 3 years ago

Should I revert the second commit with the various other fixes? It smells like more problems than solutions...

yeah, i think for a hot fix, this is the better option. when there's more time to investigate the other potential sources of bugs, they can be addressed then.

gonzalocasas commented 3 years ago

done! let's see if tests are happy. The robot.py changes should be in. For a proper fix, as you (gladly spotted on time) this needs to be carefully done in all cases were a valid value of the option is a falsy value.

beverlylytle commented 3 years ago

tests are happy!