duckietown / gym-duckietown

Self-driving car simulator for the Duckietown universe
http://duckietown.org
Other
51 stars 19 forks source link

Dynamic obstacles #47

Closed bhairavmehta95 closed 6 years ago

bhairavmehta95 commented 6 years ago

Adding an object oriented version of dynamic obstacles.

As requested, static obstacles are still checked for collisions / safety radius penalties in batch with numpy, and dynamic objects are checked in sequence.

We've abstracted the objects into classes, with the main class being a normal static object, and an inherited PedestrianObj class (with room for a future dynamic DuckiebotObj class for other non-playable vehicles on the road).

Most importantly, duckies now wiggle as they walk across the road, all slightly differently, so each one is truly unique.

maximecb commented 6 years ago

Whew, this is a big PR :O

The biggest thing I would change:

self.start = np.copy(self.obj['pos'])
self.center = self.obj['pos']

I would add all the fields to the base WorldObj class. Having to go through an internal obj dictionary is annoying.

Why differentiate between step() and update_rendering()? Would be simpler if these two were the same method?

The draw_bbox variable should probably be passed as a parameter to render(), because it doesn't really have much to do with an object' state, it just affects the way it's rendered.

The rest of my comments are more nitpicky:

The safe_driving method should maybe be renamed proximity or something. It's just not a very descriptive name. I would also like the comment to provide more of an explanation. What does this function return? Is the value higher when closer?

PedestrianObj should be named DuckieObj, to match with the name of the object in the map file Don't have a pedestrian flag. Just static: True or static: False, with False assumed by default if not specified.

For brevity, I would rename loop_obstacle_pedestrians => loop_pedestrians.

And lastly, I would would rather we put the code for all objects in gym_duckietown/objects.py. I don't like splitting things into too many small files, because it means people most often don't see much of the code when browsing around.

bhairavmehta95 commented 6 years ago

PedestrianObj should be named DuckieObj, to match with the name of the object in the map file Don't have a pedestrian flag. Just static: True or static: False, with False assumed by default if not specified.

Just on this, won't we want to have a pedestrian flag, as in the future we'll have duckiebot objects that are moving around?

Agreed with everything else, will fix.

maximecb commented 6 years ago

The duckiebot object has a different name. It can also use the static flag to indicate whether it's a static duckiebot or it's trying to follow some path on the road.

bhairavmehta95 commented 6 years ago

Also, wouldn't we want to assume that the object is static, unless explicitly specified?

Everything else is fixed (I left the default behavior to be static = False) -- just waiting for the CircleCI tests to finish.

maximecb commented 6 years ago

Looks very good. The wiggle definitely adds something.

Two last requests:

Besides that, perfect :+1:

bhairavmehta95 commented 6 years ago

Take a look when you get a chance - fixed everything requested.

maximecb commented 6 years ago

Well done :) :+1:

bhairavmehta95 commented 6 years ago

Hooray!!