Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions engine/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ fn build_api(
move |params: SimulationParams| {
let time_step = params.time_step.unwrap_or(0.1);
let duration = params.duration.unwrap_or(10.0);
simulation_manager.start_simulation(time_step, duration);
"Simulation started!"
match simulation_manager.start_simulation(time_step, duration) {
Ok(()) => "Simulation started!",
Err(_) => "Failed to start simulation",
}
}
})
.or(warp::path("stop").and(warp::get()).map({
Expand Down
30 changes: 26 additions & 4 deletions engine/managers/simulation_manager.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
use crate::physics::physics_engine::PhysicsEngine;
use std::sync::atomic::{AtomicU32, Ordering};
use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

const MAX_SIMULATIONS: usize = 5;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SimulationStartError {
InvalidParameters,
MaxSimulationsReached,
}

#[derive(Clone, Debug, PartialEq)]
pub enum SimulationState {
Running,
Expand Down Expand Up @@ -63,6 +72,7 @@ impl Simulation {
#[derive(Clone)]
pub struct SimulationManager {
simulations: Arc<Mutex<Vec<Arc<Mutex<Simulation>>>>>,
next_id: Arc<AtomicU32>,
}

impl Default for SimulationManager {
Expand All @@ -75,15 +85,26 @@ impl SimulationManager {
pub fn new() -> Self {
SimulationManager {
simulations: Arc::new(Mutex::new(Vec::new())),
next_id: Arc::new(AtomicU32::new(1)),
}
}

/// Starts a new simulation with the given time step and duration.
/// This method spawns a new thread to run the simulation.
pub fn start_simulation(&self, time_step: f32, duration: f32) {
pub fn start_simulation(
&self,
time_step: f32,
duration: f32,
) -> Result<(), SimulationStartError> {
if time_step <= 0.0 || duration <= 0.0 {
return Err(SimulationStartError::InvalidParameters);
}
let mut simulations = self.simulations.lock().unwrap();
if simulations.len() >= MAX_SIMULATIONS {
return Err(SimulationStartError::MaxSimulationsReached);
}
Comment on lines +99 to +105

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

While adding validation is a great improvement, silently returning on failure can make debugging difficult for the caller. It's not clear whether the simulation started or not, and if not, why.

Consider changing the function to return a Result to explicitly communicate success or failure. This is more idiomatic in Rust for functions that can fail.

You could define a custom error enum like this:

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SimulationStartError {
    InvalidParameters,
    MaxSimulationsReached,
}

Then, the function signature would become:
pub fn start_simulation(&self, time_step: f32, duration: f32) -> Result<(), SimulationStartError>

And the implementation would change to:

if time_step <= 0.0 || duration <= 0.0 {
    return Err(SimulationStartError::InvalidParameters);
}
let mut simulations = self.simulations.lock().unwrap();
if simulations.len() >= MAX_SIMULATIONS {
    return Err(SimulationStartError::MaxSimulationsReached);
}
// ... rest of the function
Ok(())

This would provide clear feedback to the caller, allowing for proper error handling.

let new_simulation = Arc::new(Mutex::new(Simulation::new(
simulations.len() as u32 + 1,
self.next_id.fetch_add(1, Ordering::Relaxed),
time_step,
duration,
)));
Expand All @@ -95,14 +116,15 @@ impl SimulationManager {
}
});
simulations.push(Arc::clone(&new_simulation));
Ok(())
}

pub fn stop_simulation(&self) {
let mut simulations = self.simulations.lock().unwrap();
if let Some(sim_arc) = simulations.last() {
for sim_arc in simulations.iter() {
sim_arc.lock().unwrap().state = SimulationState::Stopped;
}
simulations.pop();
simulations.clear();
}

pub fn get_simulations(&self) -> Vec<Simulation> {
Expand Down
2 changes: 1 addition & 1 deletion examples/basic_simulation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn main() {
let manager = SimulationManager::new();

// Start a simulation
manager.start_simulation(0.01, 2.0); // 0.01s time step, 2s duration
let _ = manager.start_simulation(0.01, 2.0); // 0.01s time step, 2s duration

// Wait for simulation to complete
thread::sleep(Duration::from_secs(3));
Expand Down
Empty file.
Empty file.
Empty file.
Empty file.
Binary file modified target/debug/simulation_engine
Binary file not shown.
2 changes: 1 addition & 1 deletion tests/e2e_tests.rs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

keep going with the cargo test output for shoutout

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

concurred.

$ cargo test
Compiling simulation_engine v0.1.14 (/Users/niladri/Desktop/simulation-engine)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 2.76s
     Running unittests engine/lib.rs (target/debug/deps/simulation_engine-16a5ad0d88731fa2)
running 14 tests
test physics::data_cleaner::tests::test_cleanup_inactive_objects ... ok
test physics::data_cleaner::tests::test_data_cleaner_configuration ... ok
test physics::data_cleaner::tests::test_data_cleaner_creation ... ok
test physics::data_cleaner::tests::test_cleanup_stats ... ok
test physics::physics_engine::tests::test_add_object ... ok
test physics::physics_engine::tests::test_collision_resolution ... ok
test physics::physics_engine::tests::test_gravity ... ok
test physics::data_cleaner::tests::test_needs_cleanup ... ok
test physics::physics_engine::tests::test_invalid_mass ... ok
test physics::physics_engine::tests::test_invalid_time_step ... ok
test physics::physics_engine::tests::test_set_gravity ... ok
test physics::physics_engine::tests::test_stress_large_timestep ... ok
test physics::physics_engine::tests::test_velocity_clamping ... ok
test physics::physics_engine::tests::test_wall_collision ... ok
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
     Running unittests engine/main.rs (target/debug/deps/simulation_engine-6f20b0108946e79a)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
     Running tests/e2e_tests.rs (target/debug/deps/e2e_tests-fefafb32831242f2)
running 5 tests
test e2e_tests::test_e2e_pause_simulation ... ok
test e2e_tests::test_e2e_reset_simulation ... ok
test e2e_tests::test_e2e_start_simulation ... ok
test e2e_tests::test_e2e_get_simulations ... ok
test e2e_tests::test_e2e_stop_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s
     Running tests/sdk_tests.rs (target/debug/deps/sdk_tests-107819e817854c5a)
running 5 tests
test test_physics_engine_add_object ... ok
test test_physics_engine_creation ... ok
test test_physics_engine_simulate ... ok
test test_simulation_manager_creation ... ok
test test_simulation_manager_start_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.21s
     Running tests/simulation_tests.rs (target/debug/deps/simulation_tests-942e8a94248cf1a5)
running 5 tests
test tests::test_stop_simulation ... ok
test tests::test_pause_simulation ... ok
test tests::test_get_simulations ... ok
test tests::test_reset_simulation ... ok
test tests::test_start_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
   Doc-tests simulation_engine
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ready for merge

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ mod e2e_tests {
move |params: SimulationParams| {
let time_step = params.time_step.unwrap_or(0.1);
let duration = params.duration.unwrap_or(10.0);
simulation_manager.start_simulation(time_step, duration);
let _ = simulation_manager.start_simulation(time_step, duration);
"Simulation started!"
}
})
Expand Down
2 changes: 1 addition & 1 deletion tests/sdk_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ fn test_physics_engine_simulate() {
#[test]
fn test_simulation_manager_start_simulation() {
let manager = SimulationManager::new();
manager.start_simulation(0.01, 0.1); // Very short simulation
let _ = manager.start_simulation(0.01, 0.1); // Very short simulation
std::thread::sleep(std::time::Duration::from_millis(200)); // Wait for completion
let _simulations = manager.get_simulations();
// Note: This test is timing-dependent and may be flaky
Expand Down
2 changes: 1 addition & 1 deletion tests/simulation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ mod tests {
move |params: SimulationParams| {
let time_step = params.time_step.unwrap_or(0.1);
let duration = params.duration.unwrap_or(10.0);
simulation_manager.start_simulation(time_step, duration);
let _ = simulation_manager.start_simulation(time_step, duration);
"Simulation started!"
}
});
Expand Down