these are no longe "user" locations, are they? The normalise_location method would turn them into fully normalized and avar-mapped locations

avar-mapping happens as a step after normalise_location so that we we don't have to calculate it before the lookup in model_cache; maybe the complexity of the half-way approach isn't worth it though

I think this convinces me that model_cache should use one of the following as a key instead:

• Pre-normalized but unmapped user locations
  • Cheap to compute cache key, but more misses
  • This is what was used before
• Fully-normalized and mapped user locations
  • More expensive to compute cache key, but fewer misses

I suspect the cache was added to optimise for the case where there is a large machine-generated feature code file (e.g. variable kerning), and so I lean towards the former

oh, so here the "normalized user" coordinates are the user coordinates normalized using the fvar's user-space axis triplets only, before doing the avar normalization. Usually when I see "normalized" I think about the end result, i.e. the internal normalized coordinates after the avar mapping. The user coordinates pre-normalized are just an intermediate step to avar-normalized coordinates, so it's strange to seem them used here like this..

I think before the model_cache was keyed on pre-normalized user locations, the same that VariableScalar values are, not "pre-normalized design locations" as you said above. Isn't VariableScalar specified in user-space coordinates? If not, why would avar be needed?

Sorry, that was my typo above, I meant 'user' everywhere I said 'design' 🔀

it might be a contrived example but scalar.values could in theory contain duplicate locations e.g. if repeated twice with implicit/explicit defaults:

feature kern {
    pos two <0 (wght=200:10 wght=200,wdth=100:20) 0 0>;
} kern;

scalar = VariableScalar()
scalar.add_value({"wght": 200.0}, 10)  # omits wdth (defaults to 100)
scalar.add_value({"wght": 200.0, "width": 100.0}, 20)  # explicitly sets wdth to default

That in turn would make VariationModel.getDeltas() trigger an assertion at assert len(masterValues) == len(self.deltaWeights).

Did you use the raw scalar.values intentionally? Or maybe you meant to do self.normalised_values(scalar) which would deduplicate keys (effectively the last wins):

In the main branch, a situation like the one above would trigger FeatureLibError("Failed to compute deltas for variable scalar") because constructing the VariationModel throws "Location must be unique". Maybe that's what we actually want here, instead of silently overwriting.
In your code, constructing the VariationModel doesn't fail like that with duplicate locations, because you first call full_values = self.normalised_values(scalar) which deduplicates them, but then we get the assertion error in get_deltas_and_supports.

this passes on main but throws AssertionError: (2, 1) in your branch:

diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py
index 05f645af3..5cc744b56 100644
--- a/Tests/feaLib/builder_test.py
+++ b/Tests/feaLib/builder_test.py
@@ -1308,6 +1308,22 @@ class BuilderTest(unittest.TestCase):
         assert self.get_region(var_region_axis_wght) == (0.0, 0.90625, 1.0)
         assert self.get_region(var_region_axis_wdth) == (0.0, 0.5, 1.0)
 
+    def test_variable_scalar_duplicate_location(self):
+        features = """
+            languagesystem DFLT dflt;
+
+            feature kern {
+                pos two <0 (wght=200:10 wght=200,wdth=100:20) 0 0>;
+            } kern;
+        """
+
+        font = self.make_mock_vf()
+
+        with self.assertRaisesRegex(
+            FeatureLibError, "Failed to compute deltas for variable scalar"
+        ):
+            addOpenTypeFeaturesFromString(font, features)
+
     def test_ligatureCaretByPos_variable_scalar(self):
         """Test that the `avar` table is consulted when normalizing user-space
         values."""

feel free to reuse the test if you like. I think it should still fail and with a more meaninful message like the one we had on main, rather than AssertionError.

I suspect the bug was here, where it was passing non-avar-mapped normalized location to interpolateFromMasters?

Yep, that was it exactly

The newly added test should fail on main in kind

(Although it uses the new designspace instantiation and builder, so will not run without some adapting to the old API)