Skip to content

Conversation

@elmashadmo
Copy link

  • Add option to switch between simple acceleration controls and jerk-based physics
  • Change action space from [7,13] to [4,3] for longitudinal/lateral jerk when in GIGAFLOW
  • Add new observation features for current acceleration and steering
  • Include comprehensive test suite with 9 test cases for math verification
  • Maintain backward compatibility with existing classic dynamics
  • Include 4 maps
  • Update .gitignore

Comment on lines 1050 to 1055
// Clamp indexes
// if (long_jerk_idx < 0) long_jerk_idx = 0;
// if (long_jerk_idx > 3) long_jerk_idx = 3;

// if (lat_jerk_idx < 0) lat_jerk_idx = 0;
// if (lat_jerk_idx > 2) lat_jerk_idx = 2;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be commented out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was addressing a bug I had but I ended up fixing it

Comment on lines 1134 to 1132
// TODO Need to test this in environment
// Rotate to world frame
// float temp_x = dx;
// dx = temp_x * cosf(agent->heading) - dy * sinf(agent->heading);
// dy = temp_x * sinf(agent->heading) + dy * cosf(agent->heading);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, unsure if PR is ready

Comment on lines 1218 to 1219
// 3 additional state variables per agent in GIGAFLOW
int max_obs = 7 + 3 + 7*(MAX_CARS - 1) + 7*MAX_ROAD_SEGMENT_OBSERVATIONS;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this size be conditional on the dynamics model?


// Relative Pos of other cars
int obs_idx = 7; // Start after goal distances
int obs_idx = 9; // Start after goal distances + GIGAFLOW additions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be +2 or +3?

.gitignore Outdated

# macOS Build Artifacts
*.dSYM/
test_gigaflow_dynamics

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the correct path?

* Add option to switch between simple acceleration controls and jerk-based physics
* Change action space from [7,13] to [4,3] for longitudinal/lateral jerk when in GIGAFLOW
* Add new observation features for current acceleration and steering
* Include comprehensive test suite with 9 test cases for math verification
* Maintain backward compatibility with existing classic dynamics
* Include 4 maps
* Update .gitignore
sprintf(map_file, "resources/drive/binaries/map_%03d.bin", map_id);
env->num_agents = max_agents;
env->map_name = strdup(map_file);
env->dt = 0.1f;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this could be passed as an env arg

float a_lat; // Current lateral acceleration
float prev_a_lat; // Previous lateral acceleration

float phi; // Current steering angle
Copy link
Collaborator

@nadarenator nadarenator Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renaming this to steering_angle would be more readable

Copy link
Collaborator

@nadarenator nadarenator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of requested changes:

  • remove classic dynamics
  • pass dt as an env arg
  • rename gigaflow dynamics to something more descriptive
  • rename phi
  • remove randomization coefficients and sampling
  • rename obs_size to ego_obs_size or self_obs_size
  • single move_dynamics function with branching
  • remove TODO on test_performance function if done

float c_throttle; // Throttle responsiveness
float c_steer; // Steering responsiveness
float c_acc; // Acceleration limits multiplier
float c_vel; // Velocity limits multiplier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think randomization coefficients are necessary in the main branch, can be scoped to a specific project. Anything that would be generally useful across projects should be in the main branch imo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reasoning for removing it due to it being more impactful to how the agents train and less impactful to the realism/dynamics portion? I removed it though for the time being!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right yeah. To give you some more context, in my opinion the main branch should be minimal, with features that would be useful across different projects. The jerk-based dynamics is a good example of such a feature. But dynamics randomization or even behavior conditioning coefficients is specific only to training a population of agents, which a lot of projects may not require.

entity->prev_phi = 0;
entity->prev_v = 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the previous comment, I think random sampling and coefficients should be scoped to a project and not main

e->collision_state = 0;
e->respawn_timestep = -1;
// Initialize GIGAFLOW stateful variables if using GIGAFLOW dynamics
if (env->dynamics_model == GIGAFLOW && is_active) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For people who haven't read the paper, GigaFlow doesn't really mean anything. A more informative name for dynamics could be jerk, jerk_bicycle or jerk_curvature.

init(env);
int max_obs = 7 + 7*(MAX_CARS - 1) + 7*MAX_ROAD_SEGMENT_OBSERVATIONS;
// 3 additional state variables per agent in GIGAFLOW
int obs_size = (env->dynamics_model == GIGAFLOW) ? 10 : 7;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this to self_obs_size or ego_obs_size?

return heading;
}

void move_gigaflow_dynamics(Drive* env, int action_idx, int agent_idx){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better if we stick to a single move_dynamics function that branches.

self.single_observation_space = gymnasium.spaces.Box(low=-1, high=1,
self.dynamics_model = dynamics_model

if dynamics_model == "classic":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need classic dynamics at all in the env, since the gigaflow dynamics is better all-round.

# except Exception as e:
# print(f"Error processing {map_path.name}: {e}")

# TODO: Update test_performance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this done?

Pragnay Mandavilli and others added 3 commits September 9, 2025 16:50
…ent-road

Split entities to agent and road
This commit addresses changes in the environment dynamics for
improved clarity, control, and simplicity.

* Passes dt as an environment argument for more flexible simulation

* Removes randomization coefficients and related sampling logic

* Improves variable naming for better readability (gigaflow_dynamics, phi, obs_size)

* Removes a completed TODO in the test_performance function
@nadarenator nadarenator changed the base branch from 3.0 to main September 23, 2025 19:16
@eugenevinitsky
Copy link

Closing because duplicated with another PR. Props all the same @elmashadmo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants