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

サンプルプログラムについて #2

Closed
RyodoTanaka opened this issue Jun 13, 2017 · 35 comments
Closed

サンプルプログラムについて #2

RyodoTanaka opened this issue Jun 13, 2017 · 35 comments
Assignees

Comments

@RyodoTanaka
Copy link
Member

RyodoTanaka commented Jun 13, 2017

@Nishida-Lab/ros_book
各章のサンプルプログラムですが,C++とPythonの両方を用意しておいたほうが良いと思うのですがいかがでしょうか?

@MoriKen254
Copy link
Member

それができたらメチャ付加価値高い+正直私も興味をもっていました.個人的にはやりたい…のですが.

ただし,優先度は一番低くて良いと思います.まずは現在掲載されている内容の妥当性を検証するのが最優先かと思います.

その上でもし時間があれば移植側もやってみましょう.

@yamacir-kit
Copy link
Member

yamacir-kit commented Mar 7, 2018

特に重要な要素でないので無視していただいても構わないのですが、C++ 側の実装に気になる実装があります。

引数で(それなりに大きいと思われる)オブジェクトをコピー渡ししているため速度的に難があると思われます。
一般的には参照渡しが使われます。

以下のコードで実験したところ2倍程度コピー渡しのほうが遅いようです。
https://gist.github.com/yamacir-kit/07e1e5db981dd4ace1ba7d6fbb0f71fa

@yamacir-kit
Copy link
Member

その他にも

  • インデントにスペースとタブが混在
  • 中括弧を置く位置の統一
  • for ループのスタイルの非統一
  • C++標準ライブラリに存在する関数の再発明

などがあります。

また、頻出するオブジェクトの命名則の統一(ros::NodeHandlen にするか nh にするか、など)にも気を配ると良いかと思います。

@RyodoTanaka
Copy link
Member Author

RyodoTanaka commented Mar 7, 2018

@yamacir-kit
ご指摘ありがとうございます.

  • インデントにスペースとタブが混在
  • 中括弧を置く位置の統一
  • for ループのスタイルの非統一

これはご指摘の通り,修正する必要があります.
READMEに書いてあるコーディング規約に沿って修正する予定です.

  • C++標準ライブラリに存在する関数の再発明
  • 引数で(それなりに大きいと思われる)オブジェクトをコピー渡ししているため速度的に難があると思われます。

これらについては,読者のレベルをどのように設定するかによって変わる気もします.
基本的には, @yamacir-kit にご指摘頂いているように変更すべきと思いますが,西田先生と話し合って決定したいと思います.

@MoriKen254
Copy link
Member

これ、どこまで行われているでしょうか。

スタイルの統一については、たぶんやるにしても、本文はこのままで、サンプルソース側だけの修正になると思います。

あと、引数でオブジェクトをコピー渡ししているところは、確かにお行儀が悪いですね(私がやらかしていたらすみません汗)。そこは、しれっとconst & で参照渡しをするだけで解決しそうな気がします。

実際、C++で引数を参照渡しをしているコードが掲載されている章もありますので、その水平展開と捉えれば的はずれな解決方法では無さそうです。

あとは、今の段階でどこまでやるべきかです。本文は据え置きで、処理の結果が変わらないようにコードの方を修正するのが、落とし所だと思っています。

本文では、冒頭のどこかで、

  • 公開しているサンプルコードは品質改善のために予告なく修正される可能性がある。

と、一言添えておけば嘘はないかなと思います。初期ROS本でも、バグを報告したら修正してくれていたようです。

ひとまず情報展開。

@TakeshiNishida
Copy link

11章から13章までで、例えば本の215ページで、フォルダ名が違うという指摘があります。すみませんが、修正の方針を教えてもらえませんでしょうか。pluginlib_arrayutil/arrayuti/include/..というフォルダが無いです。

@MoriKen254
Copy link
Member

pluginlib_arrayutil/arrayuti/include/..というフォルダが無いです。

「pluginlib_arrayutil/arrayuti/include/..」を始め、「arrayutil」付きはタイプミスです。その辺りの修正については、「Dropbox\rosbookworkspace\最終校正\3校ゲラ(取扱い注意)_森田修正_o.pdf」内の「7/7付き赤字」電子コメントをご参照ください。

すみませんが、修正の方針を教えてもらえませんでしょうか。

大きく分けて2つありそうです。

  1. ソースコードのスタイル統一化
    • スペースの位置とか、カッコの位置とか、そっち方面の話。世の中のスタイルチェックツールを使えばチェックは可。
    • ソースコードの命名規則の準拠。これは少々骨が折れる話です。
  2. パッケージの命名規則の修正

お手数ですが、土日のどこかで一度お打ち合わせさせて頂けますか?落とし所の雰囲気を掴めたら幸いと思います。

よろしくお願いいたいます。

@yamacir-kit
Copy link
Member

サンプルプログラム,C++の方でしたら僕の方でやりましょうか?

@TakeshiNishida
Copy link

コメントがあるところを確認しました。
本の方の修正は、残すところ1点のみです。
本の236ページの下のクローンコマンドは以下であっていますでしょうか?
$ git clone https://github.com/Nishida-Lab/rosbook_pkgs/tree/master/chapter12/arrayutil_python.git

修正の方針については理解しました。打ち合わせは特に必要でもなさそうですが、お気軽に連絡ください。

@MoriKen254
Copy link
Member

@yamacir-kit

お願いします!ブランチ切って、整形していただけると、助かります。

@TakeshiNishida
すみません、古い情報のままでした。
ご提示のコマンドは正しく動作しません汗。

正確には、

git clone https://github.com/Nishida-Lab/rosbook_pkgs.git

によって本書のサンプルコードを一括でcloneして、chapter12に入れば当該コードにたどり着けます。

他の章でも個別で上記cloneに関する記述が無いので、12章におけるその記述は削除しても良さそうですね。

代わりに、もっと冒頭のどこかで、このコマンドで全部ダウンロードできますよ、という説明があったほうが親切かもしれません。

打ち合わせも、不要そうですね。せっかくなのでただ久々に雑談したいという気持ちもありますので笑、時間できたら声をかけさせてもらいますね。

@TakeshiNishida
Copy link

冒頭で説明することに賛成です。その場合、gitのインストールとか、ディレクトリの準備が必要だと思いますので、3章の後半でしょうか?

@MoriKen254
Copy link
Member

はい、私も3章だと思います。そしてgitが必要だったり、ディレクトリを指定したりなど、3章までの技術をフル動員して、コードの取得とビルドをする形かと思っています。

そう言えば、現状gitのインストールの解説が無さそう(?)ですが、それも含めて3章の後半に当該説明を記すイメージ?

取り急ぎ。

@TakeshiNishida
Copy link

はい、その通りです。案をお願いしてもいいですか?これができれば、出版社に返信しようと思います。修正が多くなってしまいましたので、早く伝えたいと思います。

@MoriKen254
Copy link
Member

はい、お任せ下さい。私が責任を持って、本日中(日はまたぐかもですが)に原案を作成します。

現状の構想

  • 3.5. Githubからのサンプルコードの取得

    • 3.5.1. Git とGithub の基礎

      • 概要
      • Githubアカウントの取得とGitのインストール
      • 基本コマンド
    • 3.5.2. サンプルコードの取得とビルド

      • clone, catkin_make, souce, を一気通貫で実施

@MoriKen254
Copy link
Member

@yamacir-kit

C++コード整形、いつ頃できそうですか?

手が回らなそうなら、できるだけ早く連絡ください。手が回りそうでも、連絡くださいね笑。

@yamacir-kit
Copy link
Member

@MoriKen254

この土日中に終わらせようと思います.

この本が対象にしているROSのディストリビューションはKineticですか?

@MoriKen254
Copy link
Member

ありがとうございます。よろしくお願いしますね!

以下、取り急ぎ情報共有。

ROS ディストリビューション

Kineticです。

規約

下記に準拠の予定です。

オートフォーマッタ

clang-format で自動整形可能の模様なのですが、使ったことないので、もしノウハウあれば教えてくださいね!^^

CIのフォーマットチェッカ

最後はTravis CIでclang-formatチェックをかけれればと思っています。ただ、industrial_ciのドキュメントには記載せれておらず汗。

movit_ciの時代には下記のTESTオプションがあったので、それを踏襲していると勝手に想像しているのですが…。

env:
  matrix:
    - ROS_DISTRO=kinetic  ROS_REPO=ros  TEST=clang-format

@MoriKen254
Copy link
Member

参考/メモ

Pythonはpep8. Auto formatter は世の中にあり. movit_ci, Industrial_ci との連携は不明。

roslint というコード解析パッケージあり。pep8チェックに使えそうな匂いの記述はあるが、詳細が書かれていないので、実際に使ってみないとよくわからない。

@MoriKen254
Copy link
Member

3章追記分の原稿を下記に格納しました。ご査収ください。

Dropbox\rosbookworkspace\最終校正\180715_3章追加

画像は無し、2ページ強です。

本当にまっさらの環境からコマンドが有効かはまだ検証できていません。今Ubuntu のVMイメージを作っているので、明日には検証できます。

ひとまず以上です。

@MoriKen254
Copy link
Member

まっさらな環境で実行したら注意点が見えたので、それを反映させました。

Dropbox\rosbookworkspace\最終校正\180715_3章追加

@yamacir-kit
Copy link
Member

サンプルコードの変更点が本文に与える影響を確認したいので,本文のテキストを参照したいです.

@MoriKen254
Copy link
Member

Dropboxで共有しましょう。

Dropboxアカウントのメールアドレスを、下記アドレス宛に送付いただけますか?コミッターの共有スペースに招待します。

> 西田先生
問題あれば連絡下さい。問題なければこのまま招待します。

@yamacir-kit
Copy link
Member

途中経過というかほぼ断念の報告です。

現在、cxx_cleanup ブランチにてC++ソースコードのスタイル修正を行っています。
現時点で終了したのは4、6章の部分で、これは本文に対して一斉影響のない修正です。影響がないのは、本文中でC++ソースコードが引用されていないためです。

続く章の部分の変更も行おうと思ったのですが、7、8、11章はほぼC++ソースコードの解説が主となっている部分ですので、些細な修正も本文の修正が必要になってしまうことから断念しました。

僕はこのプロジェクトに関わり始めたのがここ数日からなので、進捗状況がよく分かっていないのですが、「最終校正」の名前のついたファイルが見えるので、もう詰めの段階に入っているのだと思います。
ここでソースコードに手を加えてしまうと7、8、11章は本文含め殆どを修正するような事態になりかねません。

見栄えは悪くなりますが、正直ソースコードの修正は動かないものを動くようにすることを最低限の修正として、スタイルの不統一などは諦めなければならないのかもしれません。

@yamacir-kit
Copy link
Member

それと、オートフォーマッタの件なのですが、ROSの場合は扱う引数の多さから、例えば次のような変則インデントを採用する場合が多々あるため、手で直接変更するのがやはり一番だと思います。

int num = function(other_very_long_long_function(),
                   other_very_long_long_function(),
                   other_very_long_long_function())

@TakeshiNishida
Copy link

ご協力のおかげで、修正原稿はできました。最終確認をして、明日にでも森北出版に提出します。
一方で、書籍との違いが出たとしても、読者にとって利益があるならば、修正していきたいと思いますが、修正されていることを読者が認識できる手段を考える必要があると思っています。

とり急ぎ、ここまでのお礼を申し上げます。また引き続き、どうぞよろしくお願いいたします。

@MoriKen254
Copy link
Member

MoriKen254 commented Jul 15, 2018

スタイルの件、了解です。ご検討ありがとうございます。

あと、すみません、13章にも修正が入ります。修正は軽微です。

\Dropbox\rosbookworkspace\最終校正\180716_13章校正

Ubuntu 16.04になったことで、Docker周りの勝手がちょっと変わっており、修正事項のコマンドで実行しないとindustrial_ciのコマンドが権限の問題で実行できません。

※最終校正で、直感的に怪しそうだと思ったところを突いて行きましたが、Kineticに変えたことによる歪みがなくなったと言い切れるかは、分かりません汗。ある程度残っている可能性については、覚悟が必要かもしれませんね ^^;

@MoriKen254
Copy link
Member

あと、索引は修正の余地はありそうですが、こちらはもう少し先でも大丈夫でしたか?

@TakeshiNishida
Copy link

本への修正の反映は終わりました。本当にありがとうございます。索引については、もう少し大丈夫だと思いますが、急ぎ、修正箇所を出版社に伝えたいと思います。今回の修正で16ページ単位という本のページ数の増加は避けられず、値段が少しだけ上がると予想されます。しかし、しょうががありません。

@MoriKen254
Copy link
Member

ありがとうございます。

#27 にて、本文記載の欠落したソースコードの追加につき、軽微な修正を報告しております。本文のページ数には一切影響しない、単語レベルの入れ替えのみです。ご検討頂ければ幸いです。

優先度的に #27 を先に行いましたので、索引については明日急ぎ着手します。

@TakeshiNishida
Copy link

ありがとうございます。修正について、出版社に連絡しました。索引について、引き続きお待ちしています。

@MoriKen254
Copy link
Member

おまたせ致しました。急ぎ索引を見直しました。ご質問・ご要望等あればご指示下さい。

  • Dropbox\rosbookworkspace\最終校正\索引_森田二校.xlsx
    • 「【結果】二校」シートが、現段階で上がっている単語に対するアウトプット
    • それ以外は変更履歴

一旦まとめられるものはまとめてみました。カテゴライズの根拠は、黄色ハンチング部をご参照下さい。

カテゴリの統廃合はお任せします。そのご判断材料として、カテゴリ内の単語数の比率を掲載しました。必要に応じてご利用下さい。

1

@TakeshiNishida
Copy link

もう、素晴らしすぎて言葉になりません。ありがとうございます。

@MoriKen254
Copy link
Member

弊社の技術管理部門のお上から、当方の著者紹介欄に社名を入れることにつき、問題は無いとお墨付きを頂けました。

一応任意の指摘事項として、商標の問題が挙げられました。詳細は下記フォルダをご参照ください。

  • \Dropbox\rosbookworkspace\最終校正後\商標

@MoriKen254
Copy link
Member

MoriKen254 commented Jul 24, 2018

議論が発散したので、 #31 でここの残件を整理しました。

少し様子を見た上で、こちらはお蔵入りとする予定です。

@MoriKen254
Copy link
Member

色々落ち着いたので、閉じます。

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

No branches or pull requests

4 participants