Add a Variable phase #433

BruceBrown · 2020-12-24T01:15:39Z

Variable provides a min duration, a delay duration, and an additional duration. The maximum cycle time is min + additional. Once min has been exhausted, if there is demand, the cycle is extended by delay until there isn't any demand or the additional duration has been consumed.

Fix divide by zero.
Switch seattle_traffic_signals back.

Variable provides a min duration, a delay duration, and an additional duration. The maximum cycle time is min + additional. Once min has been exhausted, if there is demand, the cycle is extended by delay until there isn't any demand or the additional duration has been consumed. Fix divide by zero. Switch seattle_traffic_signals back.

dabreegster

It's awesome to see what feels like a complicated feature get implemented so simply. Thanks for doing this!

Could you run cargo update -p seattle_traffic_signals so the changes to Cargo.lock are in this PR too? Otherwise the change in the other git repo aren't picked up

dabreegster · 2020-12-24T03:14:24Z

sim/src/mechanics/intersection.rs

@@ -72,6 +72,7 @@ struct State {
 struct SignalState {
    current_stage: usize,
    stage_ends_at: Time,
+    iteration_count: usize,


The other two variables are reasonably self-explanatory from the name. It's not clear what the iteration is without reading the code later. What if we called this extensions_count and had a comment like "The number of times a variable stage has been extended". Or maybe the var name is fine and just the comment would help

dabreegster · 2020-12-24T03:17:45Z

game/src/edit/traffic_signals/edits.rs

+                .small_heading()
+                .draw(ctx)]),
+            Widget::row(vec![
+                "Seconds:".draw_text(ctx),


I think it'd look slightly better with a .centered_vert() here and for the other two; mind adding it?

dabreegster · 2020-12-24T03:19:37Z

game/src/edit/traffic_signals/edits.rs

@@ -79,7 +114,9 @@ impl SimpleState<App> for ChangeDuration {
                let new_type = if panel.is_checked("phase type") {
                    PhaseType::Fixed(dt)
                } else {
-                    PhaseType::Adaptive(dt)


Ah, this confused me a fair bit. I set the two new spinners but didn't toggle to adaptive, and I couldn't figure out why my changes weren't taking effect. How about we get rid of the Checkbox::toggle entirely and decide between Fixed and Variable based on if delay and additional are nonzero.

I want to think about that. I agree the toggle is sub-optimal. I actually had wanted a radio button, to preserve Adaptive, but opted out of it. I've done that several times as well, forgetting to toggle. In my head I'm trying to reconcile if just having additional != 0 is sufficient. However, it leaves delay dangling out there with no meaning for a fixed signal, which I dislike almost as much as having two fields being used to indicate fixed. Originally, I considered disabling/enabling delay and additional based upon the checkbox.

This is likely something that @muzlee1113 can help with. The API for doing it isn't very ergonomic, but you can make the two extra sliders appear/disappear based on something being checked. You'd listen for Outcome::Changed, then do panel.replace() to make things appear or disappear.

For the purposes of this PR, we can leave it as it is.

dabreegster · 2020-12-24T03:24:00Z

map_gui/src/render/traffic_signal.rs

@@ -212,7 +222,13 @@ fn draw_time_left(
 ) {
    let radius = Distance::meters(2.0);
    let center = app.map().get_i(i).polygon.center();
-    let percent = time_left / stage.phase_type.simple_duration();
+    let duration = stage.phase_type.simple_duration();
+    let percent = time_left


I think let percent = if duration > Duration::ZERO { time_left / duration } else { 1.0 } would be more clear

dabreegster · 2020-12-24T03:24:20Z

map_gui/src/render/traffic_signal.rs

-                    t <= Duration::seconds(5.0),
-                    (t / stage.phase_type.simple_duration()) as f32,
-                )
+                if stage.phase_type.simple_duration() > Duration::const_seconds(0.0) {


You can use Duration::ZERO here

dabreegster · 2020-12-24T03:25:57Z

map_gui/src/render/traffic_signal.rs

            } else {
                (false, 1.0)
            };
-            let yellow = Color::YELLOW;
+            // The warning color for fixed is yellow, for anything else its orange to clue the


I set a generous additional and delay time as an example, and watched the color become darker green, then jump to orange, then back to green (as the extra time was over 5s), then back to orange again, before finally going to the next stage. A little unintuitive at first, but I think it works. It's nice to be able to clearly see more detail!

This can be changed by checking the counter. We can make it part of a second PR.

dabreegster · 2020-12-24T03:26:33Z

map_model/src/objects/traffic_signals.rs

 }

 impl PhaseType {
    // TODO Maybe don't have this; force callers to acknowledge different policies
    pub fn simple_duration(&self) -> Duration {
        match self {
            PhaseType::Fixed(d) | PhaseType::Adaptive(d) => *d,
+            PhaseType::Variable(duration, delay, _) => {
+                if *duration > Duration::const_seconds(0.0) {


Duration::ZERO

dabreegster · 2020-12-24T03:27:07Z

sim/src/mechanics/intersection.rs

@@ -134,6 +135,7 @@ impl IntersectionSimState {
        }
        if self.break_turn_conflict_cycles {
            if let AgentID::Car(car) = agent {
+                //self.blocked_by.drain_filter(|(_, c)| *c == car);


Please remove the tmp debugging bit

actually, that drain_filter is a Rust experimental feature, which replaces this:
retain_btreeset(&mut self.blocked_by, |(_, c)| *c != car);
I was playing with it and forgot to remove the commented out line. Eventually, it should be used instead, although the predicate has opposite meaning, so caution should be used. I think I'll leave a todo in its place with appropriate commentary.

BruceBrown · 2020-12-24T19:22:24Z

It's awesome to see what feels like a complicated feature get implemented so simply. Thanks for doing this!

Could you run cargo update -p seattle_traffic_signals so the changes to Cargo.lock are in this PR too? Otherwise the change in the other git repo aren't picked up

I didn't realize you committed the .lock file -- I guess I assumed you'd just generate on a checkout. I'll make the change.

dabreegster · 2020-12-24T19:34:55Z

I didn't realize you committed the .lock file -- I guess I assumed you'd just generate on a checkout.

I was confused about this for a while too, but then someone pointed me at https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

Additionally, swapped order of variable edit, to be Additional time, followed by delay.

dabreegster requested changes Dec 24, 2020

View reviewed changes

Address review comments

82e20a1

Additionally, swapped order of variable edit, to be Additional time, followed by delay.

dabreegster merged commit 3be45b8 into a-b-street:master Dec 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Variable phase #433

Add a Variable phase #433

BruceBrown commented Dec 24, 2020

dabreegster left a comment

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

BruceBrown Dec 24, 2020

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

BruceBrown Dec 24, 2020

dabreegster Dec 24, 2020

dabreegster Dec 24, 2020

BruceBrown Dec 24, 2020 •

edited

Loading

BruceBrown commented Dec 24, 2020

dabreegster commented Dec 24, 2020

Add a Variable phase #433

Add a Variable phase #433

Conversation

BruceBrown commented Dec 24, 2020

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceBrown Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

BruceBrown commented Dec 24, 2020

dabreegster commented Dec 24, 2020

BruceBrown Dec 24, 2020 •

edited

Loading