self-review

Author: schaumb

CHANGELOG.md

@@ -9,6 +9,7 @@
 - Add missing canvas change function in htmlcanvas plugin. 
 - On split charts the first range was not part of the separation calculation.
 - When the first marker was disabled it was calculated as an enabled marker on the XY normalization.
+- Sorted or reversed marker connections behaved chaotic at coordinate system change. 
 
 ## [0.11.0] - 2024-05-23
 

src/chart/animator/morph.cpp

@@ -176,7 +176,9 @@ void Connection::transform(const Marker &source,
 	    target.prevMainMarkerIdx,
 	    factor);
 
-	actual.mainId = interpolate(source.mainId, target.mainId, factor);
+	actual.polarConnection = interpolate(source.polarConnection,
+	    target.polarConnection,
+	    factor);
 }
 
 void Vertical::transform(const Gen::Plot &source,

src/chart/animator/planner.cpp

@@ -292,7 +292,8 @@ void Planner::calcNeeded()
 	        {
 		        return source.prevMainMarkerIdx
 		                != target.prevMainMarkerIdx
-		            || source.mainId != target.mainId;
+		            || source.polarConnection
+		                   != target.polarConnection;
 	        })
 	    || srcOpt->isHorizontal() != trgOpt->isHorizontal();
 }

src/chart/generator/marker.cpp

@@ -73,7 +73,7 @@ Marker::Marker(const Options &options,
 	    data,
 	    stats,
 	    index,
-	    horizontal ? &mainId->value : subAxisId);
+	    horizontal ? &mainId : subAxisId);
 
 	auto yChannelRectDim =
 	    channels.at(ChannelId::y).isDimension()
@@ -93,7 +93,7 @@ Marker::Marker(const Options &options,
 	    data,
 	    stats,
 	    index,
-	    !horizontal ? &mainId->value : subAxisId);
+	    !horizontal ? &mainId : subAxisId);
 
 	auto xChannelRectDim =
 	    channels.at(ChannelId::x).isDimension()
@@ -127,15 +127,15 @@ void Marker::setNextMarker(bool first,
     bool horizontal,
     bool main)
 {
-	(main ? nextMainMarkerIdx : nextSubMarkerIdx) = marker.idx;
-
 	if (main) marker.prevMainMarkerIdx = idx;
 
 	if (!first) {
 		double Geom::Point::*const coord =
 		    horizontal ? &Geom::Point::x : &Geom::Point::y;
 		marker.position.*coord += position.*coord;
 	}
+	else if (main && this != &marker)
+		marker.polarConnection = true;
 }
 
 void Marker::resetSize(bool horizontal)
@@ -150,10 +150,6 @@ void Marker::setIdOffset(size_t offset)
 {
 	if (prevMainMarkerIdx.hasOneValue())
 		prevMainMarkerIdx->value += offset;
-	if (nextMainMarkerIdx.hasOneValue())
-		nextMainMarkerIdx->value += offset;
-	if (nextSubMarkerIdx.hasOneValue())
-		nextSubMarkerIdx->value += offset;
 }
 
 Conv::JSONObj &&Marker::appendToJSON(Conv::JSONObj &&jsonObj) const

src/chart/generator/marker.h

@@ -54,14 +54,13 @@ class Marker
 
 	using Id = Data::MarkerId;
 
-	::Anim::Interpolated<Id> mainId;
+	Id mainId;
 	Id subId;
 	Id sizeId;
 
 	MarkerIndex idx;
 	::Anim::Interpolated<MarkerIndex> prevMainMarkerIdx;
-	::Anim::Interpolated<MarkerIndex> nextMainMarkerIdx;
-	::Anim::Interpolated<MarkerIndex> nextSubMarkerIdx;
+	::Anim::Interpolated<bool> polarConnection;
 
 	void setNextMarker(bool first,
 	    Marker &marker,

src/chart/generator/plotbuilder.cpp

@@ -97,8 +97,8 @@ Buckets PlotBuilder::generateMarkers(std::size_t &mainBucketSize)
 		    markerId,
 		    needInfo);
 
-		mainBuckets[marker.mainId.get().seriesId]
-		           [marker.mainId.get().itemId] = ▮
+		mainBuckets[marker.mainId.seriesId][marker.mainId.itemId] =
+		    ▮
 		subBuckets[marker.subId.seriesId][marker.subId.itemId] =
 		    ▮
 
@@ -339,7 +339,7 @@ void PlotBuilder::calcDimensionAxis(ChannelId type)
 			const auto &id =
 			    (type == ChannelId::x)
 			            == plot->getOptions()->isHorizontal()
-			        ? marker.mainId.get()
+			        ? marker.mainId
 			        : marker.subId;
 
 			if (const auto &slice = id.label)

src/chart/rendering/markers/connectingmarker.cpp

@@ -43,9 +43,8 @@ ConnectingMarker::ConnectingMarker(const DrawingContext &ctx,
 			    enabled && (marker.enabled || prev->enabled);
 			connected =
 			    connected && (prev->enabled || marker.enabled);
-			if (prev->mainId.get_or_first(lineIndex).value.itemId
-			    > marker.mainId.get_or_first(lineIndex)
-			          .value.itemId) {
+			if (marker.polarConnection.get_or_first(lineIndex)
+			        .value) {
 				linear = linear || polar.more();
 				connected = connected && polar.more() && horizontal;
 				enabled = enabled && polar && horizontal;

test/e2e/tests/fixes.json

@@ -31,6 +31,9 @@
 		"530": {
 			"refs": ["1ad4684"]
 		},
+		"536": {
+			"refs": ["05d387c"]
+		},
 		"32303048": {
 			"refs": ["b5d95ea"]
 		},

test/e2e/tests/fixes/536.mjs

@@ -0,0 +1,37 @@
+const testSteps = [
+	(chart) =>
+		chart.animate({
+			data: {
+				series: [
+					{ name: 'Foo', values: ['Alice', 'Bob', 'Ted', 'Jonas'] },
+					{ name: 'Bar', values: [15, 32, 12, 55] }
+				]
+			}
+		}),
+	(chart) =>
+		chart.animate({
+			channels: {
+				x: 'Foo',
+				y: 'Bar'
+			},
+			geometry: 'area',
+			reverse: true
+		}),
+
+	(chart) =>
+		chart.animate({
+			coordSystem: 'polar'
+		}),
+	(chart) =>
+		chart.animate({
+			coordSystem: 'cartesian',
+			geometry: 'line',
+			sort: 'byValue'
+		}),
+	(chart) =>
+		chart.animate({
+			reverse: false
+		})
+]
+
+export default testSteps