Skip to content

Commit 35a9809

Browse files
CopilotbedaHovorkaclaude
authored
[WIP] Fix train position calculation: Replace position-based with identity-based separator matching (#349)
* Migrate MockTrainOccupant and MockTrackBlock to MockK (Phase 1) Replace inline mock classes with MockK-based implementations to improve test maintainability and leverage Kotlin-native mocking capabilities. Changes: - PathTrackIntegrationTest.kt: * Remove MockTrainOccupant class (9 lines) * Replace 7 usages with mockk<TrackOccupant>(relaxed=true) * Clean up unused OrientedPathSeparator import - DefaultRailWayNetGridTest.kt: * Convert MockTrackBlock class to createMockTrackBlock() factory * Reduce from 43 lines to ~20 with relaxed mocks * Clean up unused imports (DynamicPathSeparator, PathElement, etc.) Test results: 1845/1849 passing (0 failures, 4 skipped) All integration tests passing (PathTrackIntegrationTest: 10/10) All unit tests passing (DefaultRailWayNetGridTest) Part of MockK migration plan (Issue #332, Phase 1 of 3) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Complete MockK migration Phase 2: remove Mockito dependency and migrate MockNodeCell This completes the MockK migration by removing the Mockito framework dependency and migrating all MockNodeCell usages to the createMockNodeCell() factory pattern. Changes: - Remove Mockito dependency from gradle.properties and build.gradle.kts - Add createMockNodeCell() factory function in TrackTestMocks.kt - Migrate 12 test files to use factory pattern (68+ usages total) - Update MockNodeCell class to accept spatialType parameter - Fix code style issues (long lines, unused imports) All 1849 unit tests and 325 integration tests passing with zero regressions. Follows Phase 1 pattern (createMockTrackBlock) for consistency. Issue #332. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Complete MockK migration Phase 3: migrate MockTrackOccupant to factory pattern Phase 3 completes the MockK migration by converting MockTrackOccupant from a manual mock class to a pure MockK factory function, following the pattern established in Phases 1 and 2. Changes: - Add createMockTrackOccupant() factory in TrackTestMocks.kt - Pure MockK implementation (TrackOccupant is interface) - Provides defaults: name="MockTrain", distanceToSemaphore=100.0 - Uses mockk(relaxed=true) with this@mockk qualifiers - Migrate 4 test files (15 usages total): - SimpleTrackEnterLeaveTest.kt (2 usages) - SimpleTrackStateTest.kt (2 usages) - RaceConditionTest.kt (4 usages, integration tests) - Remove MockTrackOccupant class (no longer needed) - Update TrackTestMocks.kt documentation with Phase 3 entry Final state after Phase 3: - 3 factory functions: createMockTrackBlock, createMockNodeCell, createMockTrackOccupant - 2 preserved classes: MockNodeCell (abstract base), MockSimulationContext (delegation) - 0 Mockito dependencies - Consistent factory pattern across all test mocks Test results: 1845 unit tests passing, 307 integration tests passing, 0 failures. Related: Issue #332 (MockK migration Phases 1-3 complete) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Complete MockK migration Phase 4: consolidate 17 inline mock factories Consolidates all inline MockK factory functions from 4 test files into the central TrackTestMocks.kt repository, completing the Phase 4 factory consolidation plan. Changes: - Added 13 new factory functions to TrackTestMocks.kt - Resolved 3 duplicate factories (2 merged, 1 split): * createMockTrack() - merged with optional maxSpeed parameter * createMockPath() - merged returning ArrayPath * createMockSemaphore() - split into createMockSemaphoreReal() and createMockSemaphoreMock() (incompatible implementations) - Removed 157 lines of duplicated code from 4 test files: * DeadlockDetectionTest.kt - removed 7 factories * TrainPathInteractionTest.kt - removed 4 factories * DefaultRailWayNetGridTest.kt - removed 1 factory * AnimatedSimulationCellRendererTest.kt - removed 3 factories - Organized into 5 categories: * Track Facilities (6 factories) * Semaphores (4 factories) * Path & Network Elements (3 factories) * Occupants & Nodes (2 factories) * Mock Implementations (1 class) Test results: ✅ All 1845 unit tests passing, 307 integration tests passing Code quality: ✅ detekt and ktlintCheck passing Outcome: Single centralized mock factory repository with consistent organization. Zero test regressions. Issue #332 (Phases 1-4) complete. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * backup * Fix Detekt failures: remove unused imports and migrate to PhysicsConstants (#360) * Initial plan * Fix Detekt issues: remove unused imports and update deprecated constants - Remove unused imports from test files (AnimationIntegrationTest, StatusBarTest, etc.) - Update deprecated constants to use domain.PhysicsConstants: * COMMON_MAX_SPEED in BaseContext.kt * MINIMAL_MAX_SPEED in AbstractPath.kt and SimpleTrack.kt * MIN_TRACK_LENGTH in SimpleTrack.kt - All Detekt checks now pass Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * Initial plan * Fix train position calculation with identity-based separator matching and add lerp() to PointF Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * Add comprehensive tests for identity-based separator matching (Issue #324) Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * Fix type mismatch in TrainPositionCalculatorTest: Cast PathSeparator to DynamicPathSeparator Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * Fix type inference error: Add non-null assertion for differentSeparator in test Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * Fix compilation error: Replace getAllCells() with iterator-based cell collection Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com> * fix detekt * fix compile * fix: resolve build failures in PR #349 test files - TrackTestMocks.kt: remove duplicate function definitions (entire factory function set was accidentally duplicated, causing overload ambiguity) - MenuBarTest.kt: add missing isNotNull import from assertk.assertions - AbstractPathTest.kt, ArrayPathTest.kt: remove duplicate createMockNodeCell import (caused detekt NoUnusedImports violation) All 1999 tests pass, detekt and ktlint clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add lerp() unit tests for PointF to meet SonarCloud coverage gate Add 5 parameterized-style tests covering t=0 (returns start), t=1 (returns end), t=0.5 (midpoint), t=0.25 (quarter point), and equal-points identity. Brings PointF line coverage to 100% (was 66.7%), satisfying the ≥80% SonarCloud quality gate requirement for PR #349. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: remove dead positionsMatch() method and unused abs import The identity-based comparison (===) introduced in the previous commit made positionsMatch() unreachable. Remove the dead method, its POSITION_MATCH_EPSILON constant, the empty companion object, and the now-unused 'import kotlin.math.abs'. This fixes the SonarCloud C Reliability Rating on new code caused by the dead private method. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address Copilot review comments on PR #349 - Fix import ordering in AbstractPathTest.kt and ArrayPathTest.kt: MockSimulationContext now appears before createMockNodeCell (alphabetical) - Strengthen consistentOrderWithEntrySeparator test: use 25% asymmetric position instead of 50% midpoint, then assert the two trains' positions differ by > 0.1 grid cells — verifies direction selection actually matters Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address remaining code review issues on PR #349 - Use PointF.lerp() in TrainPositionCalculator instead of inline arithmetic (the method was added by this PR but not yet called in production code) - Update stale comment: "Try both orderings" → explain identity comparison - Assert as? DynamicPathSeparator casts succeed in tests before use (prevents silent null from masking a wrong code path being exercised) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address remaining code review issues on PR #349 - Replace lerp() call with direct arithmetic to avoid 2 extra PointF allocations in the 30fps hot path - Strengthen identity-separator tests: use ratio=0.25 (asymmetric) and assert directional closeness instead of trivial x/y >= 0 checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Bedrich Hovorka <bedrich.hovorka@gmail.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com>
1 parent 20d600f commit 35a9809

5 files changed

Lines changed: 296 additions & 59 deletions

File tree

src/main/kotlin/cz/vutbr/fit/interlockSim/gui/animation/TrainPositionCalculator.kt

Lines changed: 19 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import cz.vutbr.fit.interlockSim.objects.tracks.TrackSection
1515
import cz.vutbr.fit.interlockSim.util.DynamicWrapperUtils
1616
import cz.vutbr.fit.interlockSim.util.Point
1717
import cz.vutbr.fit.interlockSim.util.PointF
18-
import kotlin.math.abs
1918

2019
/**
2120
* Utility for calculating train grid positions via linear interpolation.
@@ -80,21 +79,6 @@ class TrainPositionCalculator(
8079
private val context: SimulationContext,
8180
private val separatorPositionCache: Map<PathSeparator, Point>
8281
) {
83-
companion object {
84-
/**
85-
* Tolerance for comparing PathSeparator grid positions (in grid cell units).
86-
*
87-
* This epsilon handles:
88-
* - Floating-point precision errors from coordinate calculations
89-
* - Static/dynamic wrapper mismatches (both reference the same grid cell)
90-
*
91-
* Value: 0.01 grid cells (< 1% of a cell width/height)
92-
* Rationale: Grid cells are integer-indexed, so any difference < 0.5 rounds to same cell.
93-
* Using 0.01 provides safety margin while ensuring same-cell separators always match.
94-
*/
95-
private const val POSITION_MATCH_EPSILON = 0.01
96-
}
97-
9882
/**
9983
* Calculate train's grid location via linear interpolation along a track section.
10084
*
@@ -176,27 +160,31 @@ class TrainPositionCalculator(
176160
return null
177161
}
178162

179-
// Try to use entry separator to determine correct direction
180-
// Try both orderings and pick the one where first separator matches entry
163+
// Use identity-based comparison (===) to determine interpolation direction.
164+
// Unwrap dynamic wrappers to static refs, then compare with === to find which
165+
// end the train entered from — that end becomes the start of interpolation.
181166
val (entryPos, exitPos) =
182167
if (entrySeparator != null) {
183-
// Try to find which end matches the entry separator
184-
// Compare by getting grid positions (handles static/dynamic wrapper mismatch)
185-
val entryGridPos = getGridPosition(entrySeparator)
168+
// Unwrap to static references for identity comparison
169+
val entryStatic = DynamicWrapperUtils.unwrapToStatic(entrySeparator)
170+
val end0Static = DynamicWrapperUtils.unwrapToStatic(ends[0])
171+
val end1Static = DynamicWrapperUtils.unwrapToStatic(ends[1])
172+
173+
// Get grid positions for the final result
186174
val end0GridPos = getGridPosition(ends[0])
187175
val end1GridPos = getGridPosition(ends[1])
188176

189177
when {
190-
positionsMatch(entryGridPos, end0GridPos) -> {
178+
entryStatic === end0Static -> {
191179
// entrySeparator matches ends[0] - use normal order
192-
Pair(end0GridPos!!, end1GridPos ?: return null)
180+
Pair(end0GridPos ?: return null, end1GridPos ?: return null)
193181
}
194-
positionsMatch(entryGridPos, end1GridPos) -> {
182+
entryStatic === end1Static -> {
195183
// entrySeparator matches ends[1] - use reversed order
196-
Pair(end1GridPos!!, end0GridPos ?: return null)
184+
Pair(end1GridPos ?: return null, end0GridPos ?: return null)
197185
}
198186
else -> {
199-
// Can't match entry separator (race condition) - use arbitrary order
187+
// Fallback: Can't match entry separator - use arbitrary order
200188
Pair(end0GridPos ?: return null, end1GridPos ?: return null)
201189
}
202190
}
@@ -215,32 +203,11 @@ class TrainPositionCalculator(
215203
// Calculate progress ratio (clamped to [0.0, 1.0])
216204
val ratio = (distanceAlongSection / sectionLength).coerceIn(0.0, 1.0)
217205

218-
// Linear interpolation from entry to exit
219-
val interpolatedX = entryPos.x + (exitPos.x - entryPos.x) * ratio
220-
val interpolatedY = entryPos.y + (exitPos.y - entryPos.y) * ratio
221-
222-
// Return continuous coordinates (preserves sub-cell positioning)
223-
return PointF(interpolatedX.toFloat(), interpolatedY.toFloat())
224-
}
225-
226-
/**
227-
* Compare two grid positions for equality within epsilon tolerance.
228-
*
229-
* This handles:
230-
* - Floating-point precision errors from coordinate calculations
231-
* - Static/dynamic wrapper references to the same grid cell
232-
*
233-
* @param p1 First position (or null)
234-
* @param p2 Second position (or null)
235-
* @return True if positions match within POSITION_MATCH_EPSILON tolerance, false otherwise
236-
*/
237-
private fun positionsMatch(
238-
p1: Point?,
239-
p2: Point?
240-
): Boolean {
241-
if (p1 == null || p2 == null) return false
242-
return abs(p1.x - p2.x) < POSITION_MATCH_EPSILON &&
243-
abs(p1.y - p2.y) < POSITION_MATCH_EPSILON
206+
// Linear interpolation from entry to exit (ratio=0 → entry, ratio=1 → exit)
207+
val ratioF = ratio.toFloat()
208+
val x = entryPos.x.toFloat() + (exitPos.x.toFloat() - entryPos.x.toFloat()) * ratioF
209+
val y = entryPos.y.toFloat() + (exitPos.y.toFloat() - entryPos.y.toFloat()) * ratioF
210+
return PointF(x, y)
244211
}
245212

246213
/**

src/main/kotlin/cz/vutbr/fit/interlockSim/util/PointF.kt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,16 @@ data class PointF(
4040
val dy = this.y - other.y
4141
return sqrt(dx * dx + dy * dy)
4242
}
43+
44+
/**
45+
* Linear interpolation between this point and another.
46+
*
47+
* @param other Target point
48+
* @param t Interpolation parameter (0.0 = this point, 1.0 = other point)
49+
* @return Interpolated point at position t
50+
*/
51+
fun lerp(other: PointF, t: Float): PointF = PointF(
52+
x + (other.x - x) * t,
53+
y + (other.y - y) * t
54+
)
4355
}

0 commit comments

Comments
 (0)