jsk-ros-pkg / jsk_model_tools

JSK model utilities
https://github.com/jsk-ros-pkg/jsk_model_tools
BSD 3-Clause "New" or "Revised" License
6 stars 29 forks source link

[euscollada] Add size check to end-coords translation/rotation #98

Closed orikuma closed 9 years ago

orikuma commented 9 years ago

docのメンバからNodeを作ったあとにNodeのサイズを確認するように変更をしました.

euscolladaのend-coordsの生成部分に関して, 現在はtranslate, rotateの記述がない場合docの該当メンバにアクセスした時点でExceptionが出ることを期待している実装になっているようですが, yaml-cpp-0.5ではdocのメンバにアクセスした時点ではExceptionが出ず, nodeのインデックスにアクセスした段階でExceptionが出るようになっています. したがって中途半端にend-coordsの部分のeusのコードが生成されてしまい, 括弧対応が取れず正しいeusのモデルファイルになりません.

YoheiKakiuchi commented 9 years ago

インデント変わってしまっていませんか?

orikuma commented 9 years ago

ifが入ったのでインデントは1段ずれていると思います.

YoheiKakiuchi commented 9 years ago
n.size() == 0

のときに,exceptionをスローすればいいということはない?

orikuma commented 9 years ago

fprintf(output_fp, " (send %s-end-coords :translate (float-vector", limb_name.c_str()); が呼ばれる前に抜けられればいいので, Exceptionを投げても良い気がします.

YoheiKakiuchi commented 9 years ago

これでいいんじゃないかな?

value = n[i].as<double>(); // check element existing
orikuma commented 9 years ago

n[i]を使うならfor内で代入する必要がありますが, それだとfprintf(output_fp, " (send %s-end-coords :translate (float-vector", limb_name.c_str());より後になるのでダメだと思います. fprintf前にn[0]にアクセスしに行く方法もありますが, それよりはsizeをチェックしてExceptionを投げるほうが良い気がします.

YoheiKakiuchi commented 9 years ago

fprintf前にn[0]にアクセスしに行く方法もありますが, それよりはsizeをチェックしてExceptionを投げるほうが良い気がします.

どうして?

orikuma commented 9 years ago

やりたいことが"Nodeが空であるかどうかの確認"なので, exceptionを期待してn[0]にアクセスする よりはsizeを確認したほうがコードとして意味がわかりやすいかと思ったのですが, どうでしょうか.

k-okada commented 9 years ago

いろいろ議論が続いていますが,マージしても大丈夫でしょうか?それとも直す方向性があれば教えて下さい. テストとしては,end-coordsがあるけど,rotation/translationがどっちかしかない,みたいなものがあればいいのかな?そういうyamlでパッと有るのはどれだろう.

YoheiKakiuchi commented 9 years ago

ぱっと見直すと、サイズチェックのときに n.size() == 3 にするように思えてきました。 yamlが不正, translationのサイズが2だったときなどに、正しくエラーにできないなどがあるので。

k-okada commented 9 years ago

https://github.com/jsk-ros-pkg/jsk_model_tools/pull/114 でテストコードをつくってみました.まだtravis先生は何も言っていませんが,こちらの手者では,ちゃんと動いているように見えますね. サイズのチェックはtranslate, rotateなどの指定があるかどうかで,translateそのもののサイズが3以外のチェックは前も今もされていないので,今回のPRでは良いかという気がしました.

k-okada commented 9 years ago

テストはhrpsys_ros_bridge_tutorialsからもってきた, https://github.com/k-okada/jsk_model_tools/blob/709880f0334fda2e0244591360d4c8194ea1aca0/euscollada/test/sample1.yaml をつかっていて,

##
## end-coords
##
larm-end-coords:
  translate : [0, 0, -0.12]
  rotate    : [0, 1,  0, 90]
  parent    : LARM_LINK6
rarm-end-coords:
  translate : [0, 0, -0.12]
  rotate    : [0, 1,  0, 90]
  parent    : RARM_LINK6
lleg-end-coords:
  translate : [0.00, 0, -0.07]
rleg-end-coords:
  translate : [0.00, 0, -0.07]
head-end-coords:
  translate : [0.08, 0, 0.13]
  rotate    : [0, 1, 0, 90]

と,一応前パターン(torsoはない)がある気がします.rotateだけが指定されている,というのは無いですが.