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

Fix assets.json #23

Merged
merged 7 commits into from
Feb 14, 2019
Merged

Fix assets.json #23

merged 7 commits into from
Feb 14, 2019

Conversation

stefaniapedrazzi
Copy link
Member

@stefaniapedrazzi stefaniapedrazzi commented Feb 13, 2019

Fix incorrect slots definitions in assets.json.

@stefaniapedrazzi stefaniapedrazzi added the bug Something isn't working label Feb 13, 2019
@fabienrohrer
Copy link
Member

fabienrohrer commented Feb 13, 2019

TinkerbotsCubieBoxWithRoundSlots are badly designed in Webots. left/right slots should be replaced by front/back slots otherwise there is an overlap.
For its axis slot, a rotation (currently disabled) would be required to put it at the right place.

I recommend to accept this PR as-is and to fix these issues later, as it is already a good improvement. The biggest issue is the motor slot swap.

@fabienrohrer
Copy link
Member

👍

@fabienrohrer
Copy link
Member

fabienrohrer commented Feb 13, 2019

👎 Just found other bugs (cube front/back tinkerbots slots are inverted). Webots fixes are required.

"translation": "0.005 0 0.015",
"type": "lego cross female"
},
"leftSlot": {
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 checked in Webots as well and the TinkerbotsCubieBoxWithRoundSlots model has an up, left and right slots that matches the ones in the TinkerbotsCubieBoxWithCrossSlots.
So I understand that you removed the front and back slots and replaced them by the axis slot, but I understand why you also removed the up, left and right slots.
With this status, it is not possible to build the racer and roadster models.

Copy link
Member

Choose a reason for hiding this comment

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

You're right (except I didn't remove the up slot). The axisSlot and upSlot should be sufficient to create racer and roadster (except it's not possible to rotate the axis slot due to #7 so the wheels and axis will be badly oriented).

About the left and right removal, my concern is that there is an issue in Webots: TinkerbotsCubieBoxWithRoundSlots.leftSlot and TinkerbotsCubieBoxWithRoundSlots.axisSlot are wrongly defined at the same position. We should probably add front and back slots instead of left and right slots in Webots (I have to log this somewhere).

So I decided to remove them until we fix the Webots model.

Copy link
Member

Choose a reason for hiding this comment

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

I just added an issue here: https://github.com/omichel/webots/issues/215

@stefaniapedrazzi
Copy link
Member Author

Why you didn't use the Webots mechanism to specify the gender for the Tinkerbots slots?
https://www.cyberbotics.com/doc/reference/slot#field-summary

@fabienrohrer
Copy link
Member

To be honest, I was not aware of the gender mechanism, sorry for this. We could improve the models this way.

@stefaniapedrazzi
Copy link
Member Author

Ok thank you for the explanation.
I think it would be better to fix all the robot designer and Webots slots issues now (in develop branch if needed) instead of having to do it again later.

@fabienrohrer
Copy link
Member

Yes it would be good to fix this soon, but I would recommend to accept this PR because it fixes also lot of other small obvious issues not related with Webots.

@fabienrohrer
Copy link
Member

👍

@stefaniapedrazzi
Copy link
Member Author

👀

@stefaniapedrazzi
Copy link
Member Author

My fix for the male cube was wrong: left and right where simply inverted.
Not it matches the status in Webots.
But there are inconsistencies between slot position in male and female cube to solve (Webots as well).

👍

@stefaniapedrazzi
Copy link
Member Author

Oops it is still wrong, I'm fixing it.

@stefaniapedrazzi
Copy link
Member Author

👍

@fabienrohrer
Copy link
Member

Well spotted, thank you

@fabienrohrer fabienrohrer merged commit f95e7d2 into master Feb 14, 2019
@fabienrohrer fabienrohrer deleted the fix-assets-json branch February 14, 2019 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

2 participants