Stabilize beacon sorting and sort only once

[joelverhagen]

Context

The current beacon candidate sorting algorithm does not yield consistent results, i.e. there is no absolute ordering.

factorio-blueprint-editor/packages/editor/src/core/generators/beacon.ts

Lines 176 to 183 in 83343e6

	possibleBeacons = possibleBeacons
	.sort((a, b) => {
	if (a.effectsGiven === 1 || b.effectsGiven === 1) {
	return b.avgDistToEntities - a.avgDistToEntities
	}
	return a.nrOfOverlaps - b.nrOfOverlaps
	})
	.sort((a, b) => b.effectsGiven - a.effectsGiven)

The comparer implemented appears to differ in absolute ordering based on which comparisons are selected by the array sorting algorithm (a traditional comparison sort). I believe the core of the problem is the a.effectsGiven === 1 || b.effectsGiven === 1 part since it violates the transitivity requirement in comparison sorting algorithms. To demonstrate what I mean, I made a little sample app.

Consider these beacon candidates:

let possibleBeacons = [
  { label: 'a', effectsGiven: 1, avgDistToEntities: 6, nrOfOverlaps: 7 },
  { label: 'b', effectsGiven: 2, avgDistToEntities: 4, nrOfOverlaps: 5 },
  { label: 'c', effectsGiven: 2, avgDistToEntities: 8, nrOfOverlaps: 9 },
]

They may sort as b c a or c b a depending on the original ordering of the array. My sample app has this output, showing some comparisons performs to yield these two alternate orderings:

b c a
  6529 hits: total
  1678 hits: input was b c a, comparisons were [c-b a-c] then [c-b a-c]
  1651 hits: input was a c b, comparisons were [c-a b-c] then [c-b a-c]
  1620 hits: input was a b c, comparisons were [b-a c-b] then [b-a c-b c-a c-b]
  1580 hits: input was c b a, comparisons were [b-c a-b] then [b-a c-b c-a c-b]
c b a
  3471 hits: total
  1740 hits: input was c a b, comparisons were [a-c b-a] then [a-c b-a b-a b-c]
  1731 hits: input was b a c, comparisons were [a-b c-a] then [a-c b-a b-a b-c]

Sample app:

// Source: https://stackoverflow.com/a/12646864
function shuffleArray(array) {
  for (let i = array.length - 1; i > 0; i--) {
    const j = Math.floor(Math.random() * (i + 1));
    [array[i], array[j]] = [array[j], array[i]];
  }
}

let possibleBeacons = [
  { label: 'a', effectsGiven: 1, avgDistToEntities: 6, nrOfOverlaps: 7 },
  { label: 'b', effectsGiven: 2, avgDistToEntities: 4, nrOfOverlaps: 5 },
  { label: 'c', effectsGiven: 2, avgDistToEntities: 8, nrOfOverlaps: 9 },
]

let counts = {}

for (let i = 0; i < 10_000; i++) {
  shuffleArray(possibleBeacons)
  const originalOrdering = possibleBeacons.map(b => b.label).join(' ')

  const comparisonsA = []
  const comparisonsB = []
  possibleBeacons = possibleBeacons
    .sort((a, b) => {
      comparisonsA.push(`${a.label}-${b.label}`)
      if (a.effectsGiven === 1 || b.effectsGiven === 1) {
        return b.avgDistToEntities - a.avgDistToEntities
      }
      return a.nrOfOverlaps - b.nrOfOverlaps
    })
    .sort((a, b) => {
      comparisonsB.push(`${a.label}-${b.label}`)
      return b.effectsGiven - a.effectsGiven
    })

  const outputLabel = possibleBeacons.map(b => b.label).join(' ')
  const comparisonsLabel = `input was ${originalOrdering}, comparisons were [${comparisonsA.join(' ')}] then [${comparisonsB.join(' ')}]` 

  let allComparisons = counts[outputLabel]
  if (!allComparisons) {
    allComparisons = { total: 1 }
    counts[outputLabel] = allComparisons
  } else {
    allComparisons.total++
  }

  if (!allComparisons[comparisonsLabel]) {
    allComparisons[comparisonsLabel] = 1
  } else {
    allComparisons[comparisonsLabel]++
  }
}

for (const pair of Object.entries(counts).sort((a, b) => b[1].total - a[1].total)) {
  console.log(pair[0])
  for (const innerPair of Object.entries(pair[1]).sort((a, b) => b[1] - a[1])) {
    console.log(`  ${innerPair[1]} hits: ${innerPair[0]}`)
  }
}

Run on JSFiddle

This means that to maintain current (unpredictable) behavior, the sorting must be done inside the loop instead of once at the beginning since the sorting effectively shuffles in addition to sorting. I don't imagine this is intentional.

Proposed change

I propose an alternate implementation that has 3 main differences:

1. It observes transitivity thus yielding consistent results no matter the order of the input.
2. nrOfOverlaps is preferred for sorting over avgDistToEntities even for candidates with only 1 effect given (to resolve the stability issue). I tried to maintain the "spirit" of the original implementation while attaining transitivity.
3. The sorting is done once before the loop. This gives better performance for large oil fields.

Regarding the quality of the results, my data set of 759 oil field blueprints:

• 624 had better results with this change
• 135 had worse results with this change

However, I don't want to oversell the improvement because the average effect count over the whole data set before and after the change goes from 147 effects given to 151 effects given (pretty incremental).

All in all, I think the main virtue of this change is providing consistent results no matter what the order of the pumpjacks is in a given blueprint.

Example performance diff:

• Blueprint: 0eJyUmttu2zAMht/F17nQ0bLyKsNQ9GAM3ho3SNJhRZF3n2lR2dAW8KfLpulXSeRPUqTeu4fn1/F4muZLt3/vpseX+dztv7135+nHfP8sn833h7Hbd8fXw/Hn/eOvbtdd3o7yyXQZD911103z0/in29vr9103zpfpMo2Fsf7wdje/Hh7G0/KF3Res48t5+YOXWf7TAnEx7Lq35bt+AT9Np/Gx/NJcd594DvHcynN2m+cJL/jCA+sLiJcwL7bwhm1eT3i9WXnebfMSskc5vwDsMaD9lvUFcH4Z8YbCC9s8a9ABFgcMGQCRQvqikEhWiCTi+gKMAIg04m0B9gAYGowSEwDGBq+OxCh9w5Z7A4BIJ6FYuQe6s0goyXAgU4puGbiNI0rRyJXAAh0Rilce0IkjOglFJunDft1XPCITzUwDWR9SSYmsAxCJi9weA8gkEkE2eRI7Fl4m6yMS0UxsDXFAIhFvyo6tAWHBEY14o2u0IC54lE6y1kcWyESC0vYxViDIoB7lk5QaiCihRAUCS3silVSkbC3IUB4llJSVCFzHE7UM6jmOeE7i4dCiShhllEFP0RFXRCklqaFRdU3kUl3RgSAr+WwTmE0DkKjF1hDhgC9KyAPEvoFI5OKNbSASvXipWTAR6aUaBqQXyWzbS9Sy2HogGClQt5dYgSCMBZZeAidGopeh1CTWA7NEopdeBehBXJSCCJglNhCJXvQ+ZQOItNKdAM5dicBzYmypTAIItZHIxcvlFROJXpLGMXITj0gudc8g1MbcZBcQI3pWjaleAmm3EL0MGmlJu0A6C9tAlUsE3t2jYmzQU4zAu0Vb2zlVPScCV+xRNTZodiE9kh5llwoEcVFCHlC0aSASuaS6ROI5ucEVCVCaC9srrKH74xX6SyJRS5LGghAHEHQSkUuqWZ9cyyXkbRMl8a5EECMSur3UK9tALEMEk2o5lsk5ooaY2G8lEluj/KL9A0ZEFxiR/koEGpQLHgg8mROlYgXeo1mQtE1EsWDX1TLAH6UeBP6YG4hIM/WymoGHSz4C3lNijzPAwyVngjX6BiKbtZRzdIbYmtVkpoHILv06TyM9MlEDiLiBE+X6DfxRd026bhnlGe11O9LSkpoQ6LrEHmdBnpECCTdPHGlqSRcR52tGRPeYqhnShMpEM0kaS5iYWvyRNKEy6itL93klgsyViWaCtBAp0UqpiS9HjkyOrTSEcAdlGY4RJLr96/gYIoluwm0CD+xtpeOyjbxtHJkHDWN0wOhIZ2btAIJ5VhMSjWS0MnWeTGoNSzgqHo/8EvUBbqskSDTl9zIlXpFozI/Uo0NviETq0bE3RBL1eK1Pl3cYBInUo4U+RCL11GxLOnLLVAhtXJ2I9Lssmvn/O0u/PWK2aOpf23zLcwKySqQemXxiJJv81yQeSLxkw/8aL0mvb5lgIfX4FiTKPTWdkf7hMnNCZ6kWJw0/ix4C3M6SdPyWiU7D/Gd5DkaQLfMah94xsQcBtcxCL5nYi4B6k0BI9CTAa4MJIlnlpn6JHkihVwFeviXInoQN9CzAy5h6RaKXa2h0o498XPrkl8vj1vXB6/6/F7O77vd4OusXrn8BAAD//wMAiiYBsw==
• Before: Timer: Beacon generation finished in 292 ms
• After: Timer: Beacon generation finished in 198 ms