Skip to content

Commit

Permalink
Fix interpolation status for discrete axes
Browse files Browse the repository at this point in the history
  • Loading branch information
justvanrossum committed Nov 25, 2023
1 parent a68c683 commit 262017a
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 13 deletions.
9 changes: 8 additions & 1 deletion src/fontra/client/core/discrete-variation-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,16 @@ export class DiscreteVariationModel {
let index = 0;
return this._locationKeys.map((k) => (k === key ? contributions[index++] : null));
}

getDefaultSourceIndexForDiscreteLocation(discreteLocation) {
const key = JSON.stringify(discreteLocation);
const model = this._getModel(key);
const localIndex = model.reverseMapping[0] || 0;
return this._locationIndices[key][localIndex];
}
}

function splitDiscreteLocation(location, discreteAxes) {
export function splitDiscreteLocation(location, discreteAxes) {
const discreteLocation = {};
location = { ...location };
for (const axis of discreteAxes) {
Expand Down
52 changes: 40 additions & 12 deletions src/fontra/client/core/glyph-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
import {
DiscreteVariationModel,
sparsifyLocation,
splitDiscreteLocation,
} from "./discrete-variation-model.js";
import { PathHitTester } from "./path-hit-tester.js";
import { sectRect } from "./rectangle.js";
Expand Down Expand Up @@ -267,17 +268,36 @@ export class VariableGlyphController {
}

_computeSourceInterpolationStatus() {
const status = new Array(this.sources.length);
status.fill({});
for (const sourcesInfo of this._splitSourcesByDiscreteLocation()) {
const { errors, referenceLayerName } = this._computeInterpolStatusForSources(
sourcesInfo.sources,
sourcesInfo.defaultSourceLayerName
);

for (const { sourceIndex, source } of sourcesInfo.sources) {
if (this._modelErrors?.[i]) {

This comment has been minimized.

Copy link
@simoncozens

simoncozens Mar 6, 2024

The i variable isn't bound here.

This comment has been minimized.

Copy link
@justvanrossum

justvanrossum Mar 6, 2024

Author Collaborator

Thanks! The this._modelErrors property doesn't exist, so this will never trigger at runtime. It's a bit of code that should have been removed. Did you find this with a tool, or with your sharp eyes? We still haven't set up a proper JS linter.

This comment has been minimized.

Copy link
@simoncozens

simoncozens Mar 6, 2024

I've been trying to understand the code so I've been putting it through tsc to infer types. It's finding all kinds of interesting things...

status[sourceIndex] = { error: this._modelErrors[i], isModelError: true };
} else {
const error = errors[referenceLayerName][source.layerName];
status[sourceIndex] = error ? { error } : {};
}
}
}
return status;
}

_computeInterpolStatusForSources(sources, defaultSourceLayerName) {
const layerGlyphs = {};
for (const source of this.sources) {
for (const { source } of sources) {
if (source.layerName in layerGlyphs) {
continue;
}
layerGlyphs[source.layerName] = stripComponentLocations(
this.layers[source.layerName].glyph
);
}
const defaultSourceIndex = this.model?.reverseMapping[0] || 0;
const defaultSourceLayerName = this.sources[defaultSourceIndex].layerName;

let layerNames = Object.keys(layerGlyphs);
layerNames = [
Expand All @@ -293,23 +313,31 @@ export class VariableGlyphController {
layerGlyphs,
errors
);
if (Object.keys(errors[layerName]).length <= this.sources.length / 2) {
if (Object.keys(errors[layerName]).length <= sources.length / 2) {
// The number of incompatible sources is half of all sources or less:
// we've found the optimal reference layer.
referenceLayerName = layerName;
break;
}
}
const status = [];
for (const [i, source] of enumerate(this.sources)) {
if (this._modelErrors?.[i]) {
status.push({ error: this._modelErrors[i], isModelError: true });
} else {
const error = errors[referenceLayerName][source.layerName];
status.push(error ? { error } : {});
return { errors, referenceLayerName };
}

_splitSourcesByDiscreteLocation() {
const splitSources = {};
for (const [sourceIndex, source] of enumerate(this.sources)) {
const splitLoc = splitDiscreteLocation(source.location, this.discreteAxes);
const key = JSON.stringify(splitLoc.discreteLocation);
if (!(key in splitSources)) {
const defaultSourceIndex = this.model.getDefaultSourceIndexForDiscreteLocation(
splitLoc.discreteLocation
);
const defaultSourceLayerName = this.sources[defaultSourceIndex].layerName;
splitSources[key] = { sources: [], defaultSourceLayerName };
}
splitSources[key].sources.push({ sourceIndex, source });
}
return status;
return Object.values(splitSources);
}

getInterpolationContributions(location) {
Expand Down

0 comments on commit 262017a

Please sign in to comment.