WIP: Visualizer

[ivrolan]

Hi,

Please take a look to the initial draft Visualizer class in _visualizer_tools.py. This visualizer iterates over a list elements of AbstractDrawer that draw the required object with the function draw_at_t(t). I included the time as a parameter as I thought the properties of the object to draw are functions of time, so we need the specific point of time to draw it.

Each child class of AbstractDrawer should contain the object to draw and additional properties to draw it, i.e. the color of the ball.

However, I haven't been able to run the test because of type conflicts, still working on it. What do you think of this approach?

[knyazer]

ping me directly if you want me to take a look, otherwise i don't see it: either do @knyazer, or request me as a reviewer

give me a sec to take a look

[knyazer]

Overall quite a decent work. However, there are few suggestions about design:

firstly, we can make a drawer that would work independent of a particular body: any body must provide shape: UniversalShape and any UniversalShape have a bunch of parts that are AbstractConvexShapes, from which you can use get_support to figure out a point on the surface of the shape. Then you can draw anything via simply connecting a bunch of points on the surface of every convex shape. I agree that it requires a bit more work, but this also reduce the connectedness of the source code, hence it is much easier to support: once written, we can zero-shot support any shape

[knyazer]

Besides, before any PR make sure that tests pass, one way or another. They fail currently I guess because pygame is not installed, so adding it in requriements and pyproject should be enough for them to pass.

[knyazer]

But I am not going to accept a PR that fails tests: for obvious reasons, failing tests mean that something is clearly wrong.

[knyazer]

And somehow even after pre-commit hook you have issues with formatting: are you sure that you have followed CONTRIBUTING.md?

[knyazer]

you can take a look while tests fail in the 'checks' tab

[knyazer]

draw_at_t(t). I included the time as a parameter as I thought the properties of the object to draw are functions of time, so we need the specific point of time to draw it.

I don't precisely understand what you mean: while the thing we draw is indeed a function of time, we never have a dependency on it. We simply plot the current state of the system while it evolves through time.

[ivrolan]

draw_at_t(t). I included the time as a parameter as I thought the properties of the object to draw are functions of time, so we need the specific point of time to draw it.

I don't precisely understand what you mean: while the thing we draw is indeed a function of time, we never have a dependency on it. We simply plot the current state of the system while it evolves through time.

So can we take the values of position, velocity, etc as they are in the object? Will these values be changed in other places of the code to simulate in continous time?

And somehow even after pre-commit hook you have issues with formatting: are you sure that you have followed CONTRIBUTING.md?

Fixed! I was not using pre-commit as it was intended

But I am not going to accept a PR that fails tests: for obvious reasons, failing tests mean that something is clearly wrong.

I was not expecting otherwise, that is why this is marked as a Draft for now :)

Thanks for the feedback! I'll remove the t parameter of the drawing function and tweak the code to make it pass the tests first

[ivrolan]

Overall quite a decent work. However, there are few suggestions about design:

firstly, we can make a drawer that would work independent of a particular body: any body must provide shape: UniversalShape and any UniversalShape have a bunch of parts that are AbstractConvexShapes, from which you can use get_support to figure out a point on the surface of the shape. Then you can draw anything via simply connecting a bunch of points on the surface of every convex shape. I agree that it requires a bit more work, but this also reduce the connectedness of the source code, hence it is much easier to support: once written, we can zero-shot support any shape

Even if we can make an independent drawer based on the shape, I think we can also have more properties to take into account when drawing than the shape of the object. For example, for a robot, we can add a number id on top of it to visualize the agent or a change of color when driving with the goal.

Basically, it would be useful to have a general shape drawing tool, but we will need to add more things on top of it depending on the object to draw.

[knyazer]

So can we take the values of position, velocity, etc as they are in the object? Will these values be changed in other places of the code to simulate in continous time?

JAX is emulating functional programming paradigm, which means that most objects are immutable, and to do something with them, you pass them to functions. In this paradigm, when we want to draw something, we are going to make a call to a function that does drawing, and we would expect it to draw what we have passed. Nothing else, nothing more. While actually truly functional approach would be to use a function like draw(AbstractBody), setup_viz() in practice this is too ugly, hence we use the approach with rust-like classes (more like traits) where the functions are just joined into a single namespace. In JAX this is simulated with classes.

While visualizer does not directly use JAX (hence we don't have to adhere to its standards) I would still prefer to have a common codestyle between JAX parts and non-JAX parts. Thus the limitations. So, I would expect visualization to be something like:

class Visualizer:
     setup()
     drawConvex()
     drawUniversal()
     drawBody()
     destroy()

but the exact interface is on your discretion

[knyazer]

Almost LGTM: can you please replace BallDrawer with some sort of Drawer that would work on any AbstractBody? Sample a lot of point in the circle for every convex component of abstract body shape, and draw these things according to the body transformation matrix. Feel free to introduce a getter for such a transformation matrix into AbstractBody

[knyazer]

Closing since completed with #52