averbraeck / opentrafficsim

Open Source Multi-Level Traffic Simulator
BSD 3-Clause "New" or "Revised" License
28 stars 8 forks source link

Simplifying Sensor class structure and names #38

Closed WJSchakel closed 1 year ago

WJSchakel commented 1 year ago

The current class structure is (> means has subtype): Sensor (interface) > SingleSensor (interface) > AbstractSensor (abstract class) > {our actual sensors}

Sensor is a tagging interface to toggle animation. As sensors can be of all sorts of types, it is better to keep Sensor, although there are currently only sensors that are a subclass of AbstractSensor.

I suggest the following name change:

WJSchakel commented 1 year ago

In light of issue #12, the word 'Sensor' has to become 'Detector.

averbraeck commented 1 year ago

That is totally correct. A sensor is a specific type of detector (and not the other way around). So, let's try to not derive Detector from Sensor.

And I don't understand the name of the class SensorAnimationToggle...

WJSchakel commented 1 year ago

DetectorAnimationToggle is nothing more than a tagging interface to show/hide all detectors in the animation. Some of those might not be lane-based, as the abstract class Detector is. Perhaps better names would be Detector for the tagging interface, and LaneDetector for the lane-based implementation. @averbraeck agreed?

WJSchakel commented 1 year ago
WJSchakel commented 1 year ago

I have removed DetectorAnimationToggle. Toggling of detector animation now occurs on Detector. The only class that was a DetectorAnimationToggle but not a Detector is TrafficLightDetector. However, one of those consists out of 4 StartEndDetector that are a Detector. Hence, those 4 are toggled for animation, and constitute the "traffic light detector".

averbraeck commented 1 year ago

I think that this is much more clear. Let's be careful, though, to not include any animation-related code in the ots-core and ots-road projects. The animation belongs in ots-animation and ots-draw. Animation need to be loosely coupled to the logic. There could be two animation screens open, one with detector animation turned on, and the other with animation turned off. We should always ensure that that remains possible!

WJSchakel commented 1 year ago

As mentioned at the end of issue #43 (but belongs more here actually), the existence of TrafficLightDetector made me reintroduce an overarching interface. It is called Detector, and it has a subclass LaneDetector. All currently existing detectors stem from LaneDetector, except for TrafficLightDetector. Although this makes the animation logic easier, it also makes more sense for the detector hierarchy. A LaneDetector really is just a specific sort of functionality, which happens to be suitable for most of our purposes. The DetectorType does belong to Detector, which makes TrafficLightDetector a detector with a suitable type. It's implementation uses two detectors of the lane-based type, and some pub/sub to keep track of all GTU's over some area. But a detector could also be just a circle that triggers whenever a GTU relative position is within some distance from its middle. So it really makes sense to have Detector and DetectorType above LaneDetector.