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

Get handle position/rotation contraints from asset definition #24

Merged
merged 7 commits into from
Feb 21, 2019

Conversation

stefaniapedrazzi
Copy link
Member

Addresses #7: remove hard-coded translation and rotation properties and get them from the asset definition.

@stefaniapedrazzi stefaniapedrazzi added the enhancement New feature or request label Feb 18, 2019
@stefaniapedrazzi stefaniapedrazzi self-assigned this Feb 18, 2019
@stefaniapedrazzi
Copy link
Member Author

It would be nice to have free rotation constraints for the axis slot, but the drawback is that after rotating the part the translation handles are no longer aligned with the parent.

@stefaniapedrazzi
Copy link
Member Author

👍

@fabienrohrer
Copy link
Member

👀

@fabienrohrer
Copy link
Member

This goes clearly in the right direction.

Nevertheless, I found issues (on purpose?):

  1. The translation gizmo is not reset when the undo/redo mechanism is used.
  2. In my point of view, it should not be possible to translate a tinkerbot slot:

capture d ecran 2019-02-19 a 13 46 46

  1. For the axle, I would expect to see the z translation handle, and not the x-y handle.

=> I think the last 2 issues could be solved easily by adding another information in assets.json: which axis should be visible, for example:

translationHandleVisibility: [false, false, true],
rotationGizmoVisibility: [false, false, false],
...

@stefaniapedrazzi
Copy link
Member Author

stefaniapedrazzi commented Feb 20, 2019

  1. For the axle, I would expect to see the z translation handle, and not the x-y handle.

In principle I agree, but then we should not show the x-y handles for the lego cross slot too.
So if we remove them from my point of view we will introduce some inconsistencies and unneeded limitations only to the axis slot and with the current slot status it will only be possible to add an axle in the middle hole of the round slots box and not in the other two.

@fabienrohrer
Copy link
Member

Just to be sure we are speaking about the same thing, for point 3, I'm speaking about the axle endSlot, when adding a wheel to it for example. In this case, it seems logical to be to be able to move the wheel on the z-axis only. Do you agree?

@stefaniapedrazzi
Copy link
Member Author

Just to be sure we are speaking about the same thing, for point 3, I'm speaking about the axle endSlot, when adding a wheel to it for example. In this case, it seems logical to be to be able to move the wheel on the z-axis only. Do you agree?

Yes!

@stefaniapedrazzi
Copy link
Member Author

  1. The translation gizmo is not reset when the undo/redo mechanism is used.

I'm not sure to correctly understand what do you mean. I didn't change anything related to updates of the handles.
I'm comparing with the master branch and as far as I can see I get the same behavior, even if in both cases it doesn't seem correct. But I'm fixing this.

@fabienrohrer
Copy link
Member

Sorry for this, I didn't check on master.

@stefaniapedrazzi
Copy link
Member Author

I checked different solutions, but at the end I think that the simpler to code and easier to understand for the user is to reset the selection and the gizmo when a part is removed.

Note that the selection worked correctly, but not showing the gizmo for the selected object after a part remove could not be easily understandable for a user.

@stefaniapedrazzi
Copy link
Member Author

👍

@fabienrohrer
Copy link
Member

👀

Copy link
Member

@fabienrohrer fabienrohrer left a comment

Choose a reason for hiding this comment

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

It seems very good to me. This is much more generic, and a good starting point to add more specific constraints.

Up-to-you, but renaming assets/assets.md to assets/README.md would allow to see this file in GitHub when browsing the assets directory.

@stefaniapedrazzi stefaniapedrazzi merged commit cd32c56 into master Feb 21, 2019
@stefaniapedrazzi stefaniapedrazzi deleted the enhancement-hard-coded-handle-constraints branch February 21, 2019 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants