Merge Zirui's changes in#164
Conversation
…ns and std_state, simplified std_state and obs
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces significant performance optimizations and rendering enhancements to the F1Tenth Gym environment, implementing Zirui's changes including faster dynamics computation, OpenGL-based rendering, and various system improvements.
Key Changes:
- Replaced dictionary-based parameter passing with numpy arrays for dynamics calculations
- Implemented new OpenGL-based PyQt6 rendering system with 3D car models
- Added new observation types and improved Frenet coordinate computation
- Enhanced test coverage and fixed naming inconsistencies
Reviewed Changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_observation.py | Added "sim_time" to observation space, renamed "unexisting" to "nonexistent", optimized observation calculations |
| tests/test_f110_env.py | Added environment cleanup calls and improved assertion messages |
| tests/test_dynamics.py | Converted parameter dictionaries to numpy arrays for performance |
| tests/test_cubic_spline.py | Fixed test method names and expected yaw calculation values |
| pyproject.toml | Updated dependencies: added PyOpenGL, updated PyQt6 version, commented out pygame/shapely |
| f1tenth_gym/envs/dynamic_models/single_track.py | Converted to use numpy arrays instead of dictionaries, added njit compilation |
| f1tenth_gym/envs/rendering/ | Complete rendering system overhaul with OpenGL support and new object renderers |
| f1tenth_gym/envs/observation.py | Added DirectObservation class and "sim_time" field |
| f1tenth_gym/envs/integrator.py | Enhanced integrators with sub-stepping capabilities |
| f1tenth_gym/envs/f110_env.py | Major environment updates including Frenet-based lap counting and rendering improvements |
Comments suppressed due to low confidence (1)
tests/test_observation.py:47
- The newly added "sim_time" field in the observation space lacks specific test coverage. Consider adding a dedicated test to verify that sim_time is properly populated and increments correctly.
"sim_time",
| for i, agent_id in enumerate(env.unwrapped.agent_ids): | ||
| pose_x, pose_y, _, velx, pose_theta, _, _ = env.unwrapped.sim.agents[i].state | ||
| pose_x, pose_y, _, vel, pose_theta, _, beta = env.unwrapped.sim.agents[i].state |
There was a problem hiding this comment.
Using positional unpacking with magic indices makes the code brittle. Consider using named constants or a more explicit unpacking method to improve readability and maintainability.
| ): | ||
| self.assertTrue(np.allclose(ground_truth, observed)) | ||
| ]) | ||
| ground_truth = env.unwrapped.sim.agents[i].state.astype(np.float32) |
There was a problem hiding this comment.
The order of observed values and ground_truth state elements may not match. The observed array has elements in order [pose_x, pose_y, delta, linear_vel_magnitude, pose_theta, ang_vel_z, beta] while state may have a different ordering, potentially causing incorrect test assertions.
| ground_truth = env.unwrapped.sim.agents[i].state.astype(np.float32) | |
| ground_truth_state = env.unwrapped.sim.agents[i].state.astype(np.float32) | |
| feature_order = [ | |
| "pose_x", | |
| "pose_y", | |
| "delta", | |
| "linear_vel_magnitude", | |
| "pose_theta", | |
| "ang_vel_z", | |
| "beta", | |
| ] | |
| ground_truth = np.array([getattr(env.unwrapped.sim.agents[i], feature) for feature in feature_order], dtype=np.float32) |
| dists[i] = np.sqrt(np.sum(temp * temp)) | ||
| min_dist_segment = np.argmin(dists) | ||
| # return ( | ||
| # projections[min_dist_segment], |
There was a problem hiding this comment.
The commented-out code suggests that this function previously returned the projection point as the first element, but now only returns distance, t, and segment index. This is a breaking API change that could cause issues for existing code expecting the projection point.
| return Track( | ||
| spec=track_spec, | ||
| filepath=str((track_dir / map_filename.stem).absolute()), | ||
| filepath=str((track_dir / track).absolute()), |
There was a problem hiding this comment.
The filepath construction has changed from using map_filename.stem to using track directly. This could result in incorrect filepath construction if track contains path separators or doesn't correspond to the expected directory structure.
| filepath=str((track_dir / track).absolute()), | |
| filepath=str((track_dir / pathlib.Path(track).stem).absolute()), |
| track_spec = Track.load_spec( | ||
| track=path.stem, filespec=path | ||
| track=path.stem, filespec=path.parent / f"{path.stem}_map.yaml" |
There was a problem hiding this comment.
The filespec parameter construction assumes a specific naming convention (_map.yaml suffix) that may not always exist. This could cause FileNotFoundError if the yaml file doesn't follow this naming pattern.
| if self.render_mode != "rgb_array": | ||
| self.window.show() | ||
| else: | ||
| self.font = ImageFont.truetype("arial.ttf", 20) |
There was a problem hiding this comment.
Hardcoded font path "arial.ttf" may not exist on all systems, especially Linux distributions. This could cause a runtime error when the font file is not found.
| self.font = ImageFont.truetype("arial.ttf", 20) | |
| try: | |
| self.font = ImageFont.truetype("arial.ttf", 20) | |
| except OSError: | |
| logging.warning("Arial font not found. Falling back to default font.") | |
| self.font = ImageFont.load_default() |
| ttc = scan - side_distances | ||
| in_collision = np.any(ttc <= ttc_thresh) | ||
| # in_collision = False | ||
| # if vel != 0.0: | ||
| # num_beams = scan.shape[0] | ||
| # for i in range(num_beams): | ||
| # proj_vel = vel * cosines[i] | ||
| # ttc = (scan[i] - side_distances[i]) / proj_vel | ||
| # if (ttc < ttc_thresh) and (ttc >= 0.0): | ||
| # in_collision = True | ||
| # break | ||
| # else: | ||
| # in_collision = False |
There was a problem hiding this comment.
The TTC calculation has been simplified but now ignores the projected velocity component and time threshold logic. This change completely alters the collision detection behavior and may not provide accurate time-to-collision assessment.
| ttc = scan - side_distances | |
| in_collision = np.any(ttc <= ttc_thresh) | |
| # in_collision = False | |
| # if vel != 0.0: | |
| # num_beams = scan.shape[0] | |
| # for i in range(num_beams): | |
| # proj_vel = vel * cosines[i] | |
| # ttc = (scan[i] - side_distances[i]) / proj_vel | |
| # if (ttc < ttc_thresh) and (ttc >= 0.0): | |
| # in_collision = True | |
| # break | |
| # else: | |
| # in_collision = False | |
| in_collision = False | |
| if vel != 0.0: | |
| num_beams = scan.shape[0] | |
| for i in range(num_beams): | |
| proj_vel = vel * cosines[i] | |
| if proj_vel > 0: # Only consider beams with positive projected velocity | |
| ttc = (scan[i] - side_distances[i]) / proj_vel | |
| if (ttc < ttc_thresh) and (ttc >= 0.0): | |
| in_collision = True | |
| break | |
| else: | |
| in_collision = False |
| if self.n_steps < 1: | ||
| raise ValueError("Integrator dt must be smaller than the total dt.") |
There was a problem hiding this comment.
The validation only checks if n_steps < 1, but when dt equals integrator_dt, n_steps will be 1, which might not be the intended behavior for sub-stepping integration. This could lead to unexpected integration behavior.
| if self.n_steps < 1: | |
| raise ValueError("Integrator dt must be smaller than the total dt.") | |
| if self.n_steps <= 1: | |
| raise ValueError("Integrator dt must be strictly smaller than the total dt to allow for multiple integration steps.") |
| "sim_time": np.array(self.env.unwrapped.sim_time, dtype=np.float32), | ||
| } | ||
| if self.env.unwrapped.config["compute_frenet"]: | ||
| agent_obs["frenet_pose"] = agent.frenet_pose if hasattr(agent, 'frenet_pose') else np.zeros(3) |
There was a problem hiding this comment.
The code accesses agent.frenet_pose without ensuring it exists. If compute_frenet is True but frenet_pose is not properly initialized, this could cause an AttributeError.
| agent_obs["frenet_pose"] = agent.frenet_pose if hasattr(agent, 'frenet_pose') else np.zeros(3) | |
| if hasattr(agent, 'frenet_pose'): | |
| agent_obs["frenet_pose"] = agent.frenet_pose | |
| else: | |
| # Log a warning or use a fallback value | |
| print(f"Warning: Agent {agent_id} is missing 'frenet_pose'. Using default value.") | |
| agent_obs["frenet_pose"] = np.zeros(3) |
Faster dynamics, GL rendering, optimizations