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

feat(update): added update fingerjoint command #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

it-ony
Copy link

@it-ony it-ony commented Oct 2, 2023

Added an update fingerjoint command

When fingerjoint are created, the tool body persists:

  • the settings as json string
  • body0, body1, direction as references

On execution of the update fingerjoint command all features of the timelime are iterated through. If the feature is a base feature and has the settings + references stored as an attribute of the feature, the tool bodies are recreated. As the combine features references the tool bodies, the finger joints update.

If no tool body can be created the former fingerjoint features (toolbody + 2x combines) are removed.

@FlorianPommerening
Copy link
Owner

Sounds like a great addition. I think a separate command is a good compromise between waiting for custom features to finally make it out of preview mode and hacking something into the existing command. I would like to do a quick review of the code before merging and cannot get to it right now, but I'll try on the weekend.

From the description, I wondered if the error case (removing the joints if the recomputation fails) is correct: shouldn't they stick around, maybe with some warning? For example, if I vary a parameter for the thickness of some parts and with a certain thickness the joint will fail, I would be surprised if I cannot reset the thickness to a valid value and get back a working joint.

@it-ony
Copy link
Author

it-ony commented Oct 3, 2023

Sure, I can change it to warning instead. Fascinating is that now everything can be a user parameter and updating is easy.

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.

3 participants