search

RoboStack / jupyter-ros

Jupyter widget helpers for ROS, the Robot Operating System
https://jupyter-ros.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
589 stars 115 forks source link

Turtle #142

Closed ldania closed 1 year ago

ldania commented 1 year ago

System changes to allow Turtle Sim in Jupyros2

IsabelParedes commented 1 year ago

Hi @ldania! thanks for you PR. Before we can merge, though, we'll have to do a bit of cleanup. It looks like you commit history is much longer than it should be. To me only the last 6 commits seem relevant for this PR (from https://github.com/RoboStack/jupyter-ros/pull/142/commits/cbbafeab93ba075a8ed881c9a0e02afadb5731ec). Also, taking a quick look at the notebooks, only the two turtle notebooks should be part of this PR and the others shouldn't have changes. But if the other notebooks do need changes, that should be part of a separate PR. And for consistency, don't forget to clear the notebook outputs. I'll do a more detailed review once you make these changes.

Thanks :)

ldania commented 1 year ago

Hi Isabel,

The PRs have been fixed and should be ready for approval. The Publisher and Keyboard control notebooks are the same as the latest PR change (small changes PR).

ldania commented 1 year ago

Sorry for the downtime, but If im correct the current PR should be implementable, kindly an update?

IsabelParedes commented 1 year ago

Hi @ldania, Apologies for the late feedback. Looks like your PR is working great, it should be implementable. The only suggestion I have (but it's not necessary) would be to move the keyboard control to be part of the jupyros package itself. And then the scaling could be turned into a speed widget for example. Other than that, it looks good!

hbcarlos commented 1 year ago

Thanks!!