-
Notifications
You must be signed in to change notification settings - Fork 15
Pragnay/remove hardcoded offsets #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors hardcoded offset arrays by replacing them with dynamically generated grids. The hardcoded 5x5 collision offset grid (25 elements) and 15x15 z-coordinate offset grid (225 elements) are replaced with two defines (COLLISION_RANGE and Z_RANGE) and a reusable generate_offsets function that creates these grids at runtime during initialization.
Changes:
- Added COLLISION_RANGE and Z_RANGE defines to specify grid dimensions
- Created a
generate_offsetsfunction to dynamically generate offset grids - Removed 280+ lines of hardcoded offset array definitions
- Initialized offset arrays in the
initfunction using the new generator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pufferlib/ocean/drive/drive.h
Outdated
| void generate_offsets(int offsets[][2], int offset_range) { | ||
| int half_grid = offset_range / 2; | ||
| int left_most = -half_grid; | ||
| int right_most = offset_range + left_most - 1; | ||
| int index = 0; | ||
| for (int dy = left_most; dy <= right_most; dy++) { | ||
| for (int dx = left_most; dx <= right_most; dx++) { | ||
| offsets[index][0] = dx; | ||
| offsets[index][1] = dy; | ||
| index++; | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function definition in a header file will cause multiple definition linker errors when the header is included in multiple translation units (which it is - in binding.c and through drivenet.h). The function should either be declared as static inline or moved to a .c file with only a declaration in the header.
pufferlib/ocean/drive/drive.h
Outdated
| int collision_offsets[25][2] = {0}; | ||
| int z_offsets[225][2] = {0}; |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These non-const, non-static global variable definitions in a header file will cause multiple definition linker errors when the header is included in multiple translation units (binding.c and drivenet.h). These should either be declared as extern in the header with definitions in a .c file, or declared as static to have file scope in each translation unit (though the latter would create separate copies).
| // Offsets | ||
| #define COLLISION_RANGE 5 |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new defines COLLISION_RANGE and Z_RANGE lack documentation explaining what they represent. Consider adding comments explaining that these define the grid dimensions for collision detection and z-coordinate updates, respectively. For example: "Grid dimension for collision detection (creates a 5x5 grid)" and "Grid dimension for z-coordinate updates (creates a 15x15 grid)".
| // Offsets | |
| #define COLLISION_RANGE 5 | |
| // Offsets | |
| // Grid dimension for collision detection (creates a 5x5 grid) | |
| #define COLLISION_RANGE 5 | |
| // Grid dimension for z-coordinate updates (creates a 15x15 grid) |
| {1, -1}, // bottom-right | ||
| {-1, -1} // bottom-left | ||
| }; | ||
|
|
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generate_offsets function lacks documentation explaining its purpose, parameters, and behavior. Consider adding a comment block above the function explaining that it generates a square grid of 2D offsets centered at (0,0), the offset_range parameter determines the grid dimensions (e.g., 5 creates a 5x5 grid from -2 to 2), and the offsets array must be pre-allocated to size offset_range^2.
| /** | |
| * Generates a square grid of 2D integer offsets centered at (0,0). | |
| * | |
| * The resulting grid has dimensions offset_range x offset_range. For example, | |
| * an offset_range of 5 will generate offsets in the inclusive range [-2, 2] | |
| * for both x and y, producing 25 (dx, dy) pairs. | |
| * | |
| * Parameters: | |
| * offsets Pre-allocated array of size offset_range^2 by 2 that will | |
| * be filled with the generated (dx, dy) offset pairs. | |
| * offset_range Side length of the square grid. Typically a positive odd | |
| * integer so that the grid is symmetrically centered on (0,0). | |
| */ |
Greptile SummaryReplaced 250 lines of hardcoded collision and z-offset arrays with a programmatic Key Changes:
Critical Issues:
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Init as init()
participant GenOff as generate_offsets()
participant CollArr as collision_offsets[25][2]
participant ZArr as z_offsets[225][2]
participant Compute as compute_agent_metrics()
participant Step as step()
participant CheckN as checkNeighbors()
Note over Init: System Initialization
Init->>GenOff: generate_offsets(collision_offsets, COLLISION_RANGE=5)
GenOff->>CollArr: Fill array with 5x5 grid (-2 to 2)
GenOff-->>Init: Array populated
Init->>GenOff: generate_offsets(z_offsets, Z_RANGE=15)
GenOff->>ZArr: Fill array with 15x15 grid (-7 to 7)
GenOff-->>Init: Array populated
Note over Compute,Step: Runtime Usage
Compute->>CheckN: checkNeighbors(x, y, collision_offsets, 25)
CheckN->>CollArr: Read offsets for collision detection
CheckN-->>Compute: Return nearby entities
Step->>CheckN: checkNeighbors(x, y, z_offsets, 225)
CheckN->>ZArr: Read offsets for z-coordinate calculation
CheckN-->>Step: Return nearby road segments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
pufferlib/ocean/drive/drive.h, line 1736 (link)style: Use
Z_RANGE * Z_RANGEconstant instead of hardcoded 225 for consistency with line 1177.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 file reviewed, 3 comments
pufferlib/ocean/drive/drive.h
Outdated
| int collision_offsets[25][2] = {0}; | ||
| int z_offsets[225][2] = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Non-static global variables in header will cause multiple definition linker errors. Since drive.h is included by binding.c, drive.c (via drivenet.h), and visualize.c (via drivenet.h), these variables will be defined in multiple compilation units.
| int collision_offsets[25][2] = {0}; | |
| int z_offsets[225][2] = {0}; | |
| // Offset arrays (defined in drive.c) | |
| extern int collision_offsets[25][2]; | |
| extern int z_offsets[225][2]; |
Then add the actual definitions in a .c file:
// In drive.c or a new offsets.c file
int collision_offsets[25][2] = {0};
int z_offsets[225][2] = {0};Prompt To Fix With AI
This is a comment left during a code review.
Path: pufferlib/ocean/drive/drive.h
Line: 138:139
Comment:
**logic:** Non-static global variables in header will cause multiple definition linker errors. Since `drive.h` is included by `binding.c`, `drive.c` (via `drivenet.h`), and `visualize.c` (via `drivenet.h`), these variables will be defined in multiple compilation units.
```suggestion
// Offset arrays (defined in drive.c)
extern int collision_offsets[25][2];
extern int z_offsets[225][2];
```
Then add the actual definitions in a `.c` file:
```c
// In drive.c or a new offsets.c file
int collision_offsets[25][2] = {0};
int z_offsets[225][2] = {0};
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
pufferlib/ocean/drive/drive.h
Outdated
| void generate_offsets(int offsets[][2], int offset_range) { | ||
| int half_grid = offset_range / 2; | ||
| int left_most = -half_grid; | ||
| int right_most = offset_range + left_most - 1; | ||
| int index = 0; | ||
| for (int dy = left_most; dy <= right_most; dy++) { | ||
| for (int dx = left_most; dx <= right_most; dx++) { | ||
| offsets[index][0] = dx; | ||
| offsets[index][1] = dy; | ||
| index++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Function definition in header will cause multiple definition linker errors when included in multiple compilation units. Should be either static inline or declared here and defined in a .c file.
| void generate_offsets(int offsets[][2], int offset_range) { | |
| int half_grid = offset_range / 2; | |
| int left_most = -half_grid; | |
| int right_most = offset_range + left_most - 1; | |
| int index = 0; | |
| for (int dy = left_most; dy <= right_most; dy++) { | |
| for (int dx = left_most; dx <= right_most; dx++) { | |
| offsets[index][0] = dx; | |
| offsets[index][1] = dy; | |
| index++; | |
| } | |
| } | |
| } | |
| // Function declaration (definition should be in drive.c) | |
| void generate_offsets(int offsets[][2], int offset_range); |
Or use static inline:
| void generate_offsets(int offsets[][2], int offset_range) { | |
| int half_grid = offset_range / 2; | |
| int left_most = -half_grid; | |
| int right_most = offset_range + left_most - 1; | |
| int index = 0; | |
| for (int dy = left_most; dy <= right_most; dy++) { | |
| for (int dx = left_most; dx <= right_most; dx++) { | |
| offsets[index][0] = dx; | |
| offsets[index][1] = dy; | |
| index++; | |
| } | |
| } | |
| } | |
| static inline void generate_offsets(int offsets[][2], int offset_range) { | |
| int half_grid = offset_range / 2; | |
| int left_most = -half_grid; | |
| int right_most = offset_range + left_most - 1; | |
| int index = 0; | |
| for (int dy = left_most; dy <= right_most; dy++) { | |
| for (int dx = left_most; dx <= right_most; dx++) { | |
| offsets[index][0] = dx; | |
| offsets[index][1] = dy; | |
| index++; | |
| } | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pufferlib/ocean/drive/drive.h
Line: 123:135
Comment:
**logic:** Function definition in header will cause multiple definition linker errors when included in multiple compilation units. Should be either `static inline` or declared here and defined in a `.c` file.
```suggestion
// Function declaration (definition should be in drive.c)
void generate_offsets(int offsets[][2], int offset_range);
```
Or use `static inline`:
```suggestion
static inline void generate_offsets(int offsets[][2], int offset_range) {
int half_grid = offset_range / 2;
int left_most = -half_grid;
int right_most = offset_range + left_most - 1;
int index = 0;
for (int dy = left_most; dy <= right_most; dy++) {
for (int dx = left_most; dx <= right_most; dx++) {
offsets[index][0] = dx;
offsets[index][1] = dy;
index++;
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Removed hardcoded offsets
Code Cleanup