-
Notifications
You must be signed in to change notification settings - Fork 6
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 taxel centers and normals #6
Conversation
L2 and L1 taxels had wrong origin and scale/rot not applied causing the pos/ori to be wrong. L3 pos/ort were already correct. Additionally all taxels were cleaned to be only faces, not closed cuboids that had too many vertices and normals, unwanted for the pos/ori computation
@rhaschke This is not ready for a PR yet. We wanted to discuss the need or not to change also the taxel center with projection or not. Hence me not creating a PR |
model/tactile_markers.urdf.xacro
Outdated
<!--tmm does not exist for L1 thumb, but can be tmo which is larger and contains tmm --> | ||
<xacro:if value="${layout=='L1' and finger=='thumb'}"> | ||
<xacro:taxel mesh="${F}mm" idx="${tactMap.get(F+'mo', -1)}"/> | ||
</xacro:if> | ||
<xacro:taxel mesh="${F}mm" idx="${tactMap.get(F+'mm', -1)}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!--tmm does not exist for L1 thumb, but can be tmo which is larger and contains tmm --> | |
<xacro:if value="${layout=='L1' and finger=='thumb'}"> | |
<xacro:taxel mesh="${F}mm" idx="${tactMap.get(F+'mo', -1)}"/> | |
</xacro:if> | |
<xacro:taxel mesh="${F}mm" idx="${tactMap.get(F+'mm', -1)}"/> | |
<!--tmm does not exist for L1, but is covered by tmo as well --> | |
<xacro:taxel mesh="${F}mm" idx="${tactMap.get(F+'mm', tactMap.get(F+'mo', -1) if layout=='L1' else -1)}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of the text are clear in fact. and I don't find how to do another suggestion or reject this one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it took me some time to ensure this optimization of the code is doing the same due mentally needing to evaluate the code to understand what it does, while the if case is straight forward because it explicitly refers to the thumb.
With the optimization we have to access an external document, to verify that there is no other ${F}mm that is not missing in another finger to be sure it works only for the thumb. For me, this opacifies the understanding for future developments, and would for sure be missed in case one new layout has a pinky tmm missing or so.
I confirm it should do the job for now, but don't like it.
Open issues:
|
Otherwise looks good to me. |
Some rot/scale were still not applied on the L2right model as well as 2 origins + rot/scale on L3right. L2left had all normals flipped and outside recomputation not active in the script.
15906fa
to
993af0c
Compare
L2 and L1 taxels had a wrong origin and scale/rot not applied causing the positions/normals to be wrong. Additionally, all taxels were cleaned to be only faces, not closed cuboids. The latter had vertices and normals which shouldn't be considered for pos/normal computation.
Centers and normals were off in L2 layout.