Scene plotter visualiztion features #134
Conversation
invertedai/utils.py
Outdated
| @@ -713,7 +718,17 @@ def plot_frame(self, idx, ax=None, numbers=False, direction_vec=False, | |||
|
|
|||
| def animate_scene(self, output_name=None, start_idx=0, end_idx=-1, ax=None, | |||
There was a problem hiding this comment.
It would be great to add a docstring and some type hints here.
(I apologize for not doing that when I originally made this, but it's not too late to start :) )
There was a problem hiding this comment.
As I am going through the attributes and methods making docstrings, I have two questions as of now:
- What does open_drive do? It appears to be a flag that enables different behaviour but what is the high level function of these different execution paths?
- It seems like there are a lot of parameters from the LocationResponse dataclass are input separately into the class. My question is why is this done separately instead of just inputting the LocationResponse directly? Is there any use case where a user needs to modify these parameters after getting the LocationResponse individually since it is opening this tool up to misuse and seems overall clunky.
There was a problem hiding this comment.
-
I don't know what
open_drivedoes unfortunately. Maybe that's something Alireza added? -
If you can refactor that I would say go for it, seems like a sensible thing to do!
There was a problem hiding this comment.
This was said in-person but for documentation purposes, I will be taking this feedback and doing a larger refactor of ScenePlotter.
invertedai/utils.py
Outdated
| c = self.agent_c | ||
| lw = 1 | ||
| fc = self._get_color(agent_idx,self.agent_face_colors) | ||
| if not fc: |
There was a problem hiding this comment.
You should use if fc is None here. Although in this case I think you'd be OK, it is generally not a good idea to check for None as a boolean (the problem is that 0, [] and '' are also False, but they are not None). See: https://peps.python.org/pep-0008/#programming-recommendations
|
Do you have more changes coming? If so, I'll hold off on reviewing for now. |
I do have some more changes coming and am doing a larger refactor. |
|
OK, let me know when it's ready. |
|
This PR is ready for a re-review. Noteable changes include:
|
…isappearing lights on large maps.
There was a problem hiding this comment.
This looks awesome. I have not had time to look at the notebooks, but the updates to ScenePLotter look awesome, it's great to have some actual docstrings now.
I could come back to the notebooks, if you want, but that seems like it might benefit from Frank / Robert's opinion more than mine.
|
@bzwartsenberg coming back to this and seeing if there is time to review this? |
|
@adscib I am going through old PR's and I've changed this to be more backwards compatible, requiring less concern about merging. Essentially this adds documentation and cleaned up the scene plotter, as well as adding left-hand option for visualizing Carla maps more easily. |
|
@adscib Pinging this PR again to get the go-ahead to merge this into develop. Added some changes to update this branch to fit with changes to the develop branch. |
KieranRatcliffeInvertedAI
deleted the
scene_plotter_visualiztion_features
branch
Added the capability to change the colours of all agents, including the colour of their edges, to ScenePlotter. This should not force any existing implementations of ScenePlotter but give some visualization flexibility internally or externally if the user chooses to do so.
Are there any other features that might be interesting to add to this PR such as adding agent path visualization to ScenePlotter?