[tryuan99]

This PR adds a script to generate an HTML file containing an interactive 3D animation of the telemetry file. The output looks as follows:

There could be many more improvements to enhance the appearance of the telemetry animation, but I am not a frontend engineer.

[stacklane-pr-stack-visualizer]

🧱 Stack PR · Base of stack

Stack Structure:

• [Tools] Add script to animate telemetry log #172 titan/animate_log ← YOU ARE HERE

[daniellovell]

This is sick! What do you think about replay_log instead of animate_log?

[coderabbitai]

Actionable comments posted: 4

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 469b83e and 37f2144.

⛔ Files ignored due to path filters (3)

• Assets/Plugins/libassignment.dylib is excluded by !**/*.dylib
• Assets/Plugins/libassignment.so is excluded by !**/*.so
• docs/images/telemetry_animation.png is excluded by !**/*.png

📒 Files selected for processing (2)

• Tools/replay_log.py (1 hunks)
• plugins/assignment/cp_assignment.cc (0 hunks)

💤 Files with no reviewable changes (1)

• plugins/assignment/cp_assignment.cc

🧰 Additional context used

🪛 Ruff (0.14.8)

Tools/replay_log.py

323-323: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Build plugins for windows-latest
• GitHub Check: Test plugins on ubuntu-latest
• GitHub Check: Test plugins on macos-latest
• GitHub Check: Test plugins on windows-latest

🔇 Additional comments (6)

Tools/replay_log.py (6)

1-20: LGTM! Clean module structure.

The imports are well-organized and the constants for JSON property names are appropriately defined.

---

22-47: LGTM! Efficient data aggregation.

The function correctly groups telemetry data by agent and combines trajectory information with agent properties.

---

91-133: LGTM! Solid interpolation implementation.

Both the binary search and linear interpolation logic correctly handle edge cases (agents not yet spawned, clamping at end of trajectory).

---

135-153: LGTM! Consistent property access.

Both functions now use dot notation consistently for accessing agent properties, and the property names correctly match the JSON structure generated from Python.

---

254-256: Good approach: Removing interpolated position before adding new data.

The pop operations remove the previously interpolated position before adding new actual data points. This prevents duplicate positions when the animation time advances through regions where new telemetry samples are available.

---

74-74: No action needed. Plotly.js version 3.3.0 is available on cdn.plot.ly and the CDN URL is correctly formed.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Review the animation timing strategy.

The combination of setTimeout and requestAnimationFrame may not achieve the target frame rate. requestAnimationFrame already throttles to the display refresh rate (typically 60Hz), and the setTimeout delay adds extra latency. This could make the animation run slower than the specified fps.

Consider using a timestamp-based approach or just setTimeout:

-    setTimeout(() => requestAnimationFrame(animate), 1000.0 / fps);
+    setTimeout(animate, 1000.0 / fps);

Or use requestAnimationFrame with timestamp tracking for more precise control:

let lastFrameTime = performance.now();
function animate(currentTime) {
    const elapsed = currentTime - lastFrameTime;
    if (elapsed < 1000.0 / fps) {
        requestAnimationFrame(animate);
        return;
    }
    lastFrameTime = currentTime;
    // ... rest of animation logic
    requestAnimationFrame(animate);
}

🤖 Prompt for AI Agents

In Tools/replay_log.py around line 286, the current animation scheduling uses
setTimeout wrapping requestAnimationFrame which adds extra latency and prevents
reliably hitting the target fps; replace this with a pure requestAnimationFrame
loop that uses the timestamp passed to the animate callback and a lastFrameTime
variable to throttle frames (compute elapsed = currentTime - lastFrameTime, skip
rendering if elapsed < 1000.0 / fps, otherwise update lastFrameTime and run
frame logic), and ensure the initial lastFrameTime is initialized appropriately
(e.g., with the first timestamp) and requestAnimationFrame is used for all
subsequent scheduling.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Ensure output directory exists before writing.

The code writes directly to the output path without verifying that the parent directory exists. If the user specifies a nested path that doesn't exist, this will raise a FileNotFoundError.

Apply this diff to create the directory if needed:

+    output_path = Path(output)
+    output_path.parent.mkdir(parents=True, exist_ok=True)
-    with open(output, "w") as f:
+    with open(output_path, "w") as f:
         f.write(html)
-    logging.info("Successfully generated HTML file: %s.", output)
+    logging.info("Successfully generated HTML file: %s.", output_path)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In Tools/replay_log.py around lines 309-311, the code opens the output file for
writing without ensuring the parent directory exists; update it to create the
output directory before writing (e.g., compute the parent path from output and
call os.makedirs(parent, exist_ok=True) or use
pathlib.Path(output).parent.mkdir(parents=True, exist_ok=True)), then proceed to
open and write the file and log success.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider using a shorter exception message or defining it separately.

The static analysis tool suggests moving the exception message outside the exception class for better maintainability, though this is a minor style concern given the message length.

+NO_TELEMETRY_FILE_ERROR = "No telemetry file was provided."
+
 def main(argv):
     # ...
     if not telemetry_file_path:
-        raise ValueError("No telemetry file was provided.")
+        raise ValueError(NO_TELEMETRY_FILE_ERROR)

Alternatively, use a shorter message:

-        raise ValueError("No telemetry file was provided.")
+        raise ValueError("Missing telemetry file")

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools

🪛 Ruff (0.14.8)

323-323: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents

In Tools/replay_log.py around line 323, the ValueError message is inlined;
extract the message into a module-level constant (e.g., NO_TELEMETRY_MSG = "No
telemetry file was provided.") and use that constant in the raise, or replace
the long message with a shorter phrase (e.g., "No telemetry file provided.") to
satisfy the style check; update any tests/uses accordingly.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Add validation for the fps parameter.

The fps (frames per second) parameter should be validated to ensure it's positive. A negative or zero value would cause incorrect animation timing (division by zero or negative delays).

Add validation in the main function:

 def main(argv):
     assert len(argv) == 1, argv
+    
+    if FLAGS.fps <= 0:
+        raise ValueError(f"fps must be positive, got {FLAGS.fps}")

     if FLAGS.telemetry_file:

Or use a validator in the flag definition:

-    flags.DEFINE_float("fps", 30, "Target frame rate.")
+    flags.DEFINE_float("fps", 30, "Target frame rate.", lower_bound=0.0)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	flags.DEFINE_float("fps", 30, "Target frame rate.")
	flags.DEFINE_float("fps", 30, "Target frame rate.", lower_bound=0.0)

🤖 Prompt for AI Agents

In Tools/replay_log.py around line 341, the fps flag is defined without
validation so zero or negative values will break timing; add validation by
either registering a validator (e.g., flags.register_validator("fps", lambda v:
v > 0, message="fps must be > 0")) or by checking in main after flag parsing (if
FLAGS.fps <= 0: raise SystemExit or ValueError with a clear message) before any
division or delay calculations that use FPS.