googlefonts / googlefonts/fontations

[skrifa] Location/LocationRef type ergonomics

Open
#866 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
826
Forks
75
Avg merge
22h 33m
Merged PRs (30d)
75

Description

The `Location` and `LocationRef` types were intended to serve a similar purpose to other owned/borrowed type pairs in Rust (`String`/`&str`, `Vec`/`&[T]`, etc) but actual usage doesn't actually follow the same patterns leading to confusion and frustration.

The main issue is that `Location` cannot impl `Deref` due to the requirements of the `Deref` trait. Here's one potential solution along with some proposed renaming for consistency and to make the purpose of these types more clear at a glance:

```rust
/// Coordinate that represents the location of a single axis in variation
/// space, normalized to the range `-1.0..=1.0`.
pub type VarCoord = F2Dot14;

/// Borrowed type for a collection of normalized coordinates, one for each
/// axis in the same order as the `fvar` table, which represents a location
/// in variation space.
///
/// For the owned version of this type, see [`VarLocation`].
pub type VarLocationRef = [VarCoord];

/// Owned type for a collection of normalized coordinates, one for each
/// axis in the same order as the `fvar` table, which represents a location
/// in variation space.
///
/// (Other docs for how to construct this, examples, etc)
///
/// For the borrowed version of this type, see [`VarLocationRef`].
#[derive(Default)]
pub struct VarLocation(SmallArray);

impl Deref for VarLocation {
type Target = VarLocationRef;

fn deref(&self) -> &Self::Target {
&self.0
}
}
```

This renames `NormalizedCoord` -> `VarCoord` to make the relation clear and adds a `Var` prefix to the location types.

Notably `VarLocationRef` now just becomes a type alias for `[VarCoord]` which is an unsized type similar to `str` leading to the same usage patterns (you can never use it without a `&`).

All types that accept a location would now take a `&VarLocationRef` parameter which can be passed as follows:

```rust
pub fn uses_variations(var_loc: &VarLocationRef) {}

// with an owned location
let owned_loc = font.axes().location(...);
uses_variations(&owned_loc);

// vello style with coords accumulated in a scene
struct Scene {
// ...
all_coords: Vec,
}
uses_variations(&scene.all_coords[start..end]);

// passing default (or empty) locations
uses_variations(Default::default());
uses_variations(&VarLocation::default());
```

This change can be done incrementally without breaking Skia.

We _could_ make `VarLocationRef` itself a real unsized type to also enable this pattern but that would require unsafe code and IMO a borrowed location is really just a slice of coordinates.

@rsheeter wdyt? @cmyr thoughts on general rustiness of this approach?

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by locating the skrifa definitions and usages of Location, LocationRef, and NormalizedCoord. Compare their current API patterns with the proposed VarCoord, VarLocation, and VarLocationRef aliases, then trace the types that accept locations. Done means the ownership and borrowing ergonomics are consistent across callers without breaking the stated incremental compatibility with Skia.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.