try out alchemist Length type

[sargunv]

I like how this enables map scale arithmetic between length and dp. But there's some blockers before we can merge it:

• Support JVM 11 kevincianfarini/alchemist#54 (hard)
• Allow multiplying by Double, not just Int kevincianfarini/alchemist#53 (soft)

I also want to see whether it can make implementations of our scale bar measure interface simpler (generate stops based on provided units).

[sargunv]

• try out alchemist Length type #234 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[westnordost]

I don't know, it looks like an interesting finger exercise, but I don't see the point:

• it increases code complexity, and is also more code
• it adds a dependency (every additional dependency is a liability), and this dependency is very fresh (API may change rapidly, bugs)

and most importantly, what about my 🍌 bananas unit? It is certainly not supported by Alchemist! 😥

[sargunv]

it increases code complexity, and is also more code

re: "more code", it's primarily due to the addition in the expressions AST, only necessary if we want Length in those fill extrusion expressions specified in meters. (EDIT, also due to auto generating stops, see below)

re: "complexity": personally I like the simplicity and compile time safety of arithmetic like length = scale * dp or dp = length / scale where length, scale, and dp all have types represented by value classes.

it adds a dependency (every additional dependency is a liability), and this dependency is very fresh (API may change rapidly, bugs)

but yeah, this is my main concern, and the primary reason why this PR is just a draft. My thinking here is that it's also true of our library, and this pre v1 stage is the best time to experiment with API things like this. Worst case, we push a breaking change in the future to drop the dependency if it doesn't work out (or fork its Length into a new package if we find it valuable enough but it stops being maintained).

but as it is, I'm not yet sure if that arithmetic safety is worth the dependency. a more ergonomic API for scale bar definitions might be, which leads into:

and most importantly, what about my 🍌 bananas unit? It is certainly not supported by Alchemist! 😥

I actually think, with the right API, it'll be easier to add new scale bar measures (including custom units, as long as your custom unit is divisible by nanometers). Bananas are silly, but realistically I could see this being used by consumers of the library to create scale bars for less common units like nautical miles, football fields, or US survey feet / miles if that restriction to units divisible by nanometers is lifted.

Something like, for already supported units:

data object FeetYardsMilesSystem : ScaleBarMeasurementSystem {
  override val units: Set<Length> = setOf(Foot, Yard, Mile)

  // stops are generated automatically based on real size of these units
  // optional override to generate alternative stops, not implemented here
  // override val stops: List<Length> = listOf(..)

  // optional override to provide localized symbols
  @Composable
  override fun formatStop(stop: Length) = when(stop) {
    < 1.yards -> "${length.toDouble(Foot).toShortString()} ${stringResource(Res.values.feet_symbol)}"
    < 1.mile -> "${length.toDouble(Yard).toShortString()} ${stringResource(Res.values.yards_symbol)}"
    else -> "${length.toDouble(Mile).toShortString()} ${stringResource(Res.values.miles_symbol)}"
  }
}

and for measures with custom units:

data object Banana : LengthUnit { 
  override val nanometerScale: Long get() = 19.centimeters.toLong(Nanometer)
  override val symbol = "🍌"
}

data object BananasSystem : ScaleBarMeasurementSystem {
  override val units = setOf(Banana)

  // default `format` uses `LengthUnit.symbol` without localization
}

[sargunv]

Here it is, kilobananas and football fields in the latest draft (just to demo the API), with auto generated stops (who knew, 500 bananas are about the size of a US football field).

where the custom units are configured with only:

data object Banana : LengthUnit {
  override val nanometerScale: Long = 19.centimeters.toLong(Nanometer)
  override val symbol: String = "🍌"
}

data object Kilobanana : LengthUnit {
  override val nanometerScale: Long = 1000 * Banana.nanometerScale
  override val symbol: String = "K🍌"
}

data object FootballField : LengthUnit {
  override val nanometerScale: Long = 100 * Yard.nanometerScale
  override val symbol: String = "🏈"
}

and used like:

ScaleBar(
  scale = cameraState.scaleAtTarget,
  measures =
    ScaleBarMeasures(
      primary = ScaleBarMeasure.Default(Banana, Kilobanana),
      secondary = ScaleBarMeasure.Default(FootballField),
    ),
  modifier = Modifier.align(Alignment.BottomStart),
)

These use default, non-localized symbols, but for our built in measures we override getText to localize the symbols (and users can do this too):

public data object Metric : Default(Meter, Kilometer) {
  @Composable
  override fun getText(stop: Double, unit: LengthUnit): String {
    val symbol =
      when (unit) {
        Meter -> stringResource(Res.string.meters_symbol)
        Kilometer -> stringResource(Res.string.kilometers_symbol)
        else -> error("impossible")
      }
    return "${stop.toShortString()} $symbol"
  }
}

[westnordost]

Good to hear that bananas survived. 😆
Anyway,

only necessary if we want Length in those fill extrusion expressions specified in meters

My argument still stands, though. Is it worth the effort (, added dependencies, added code complexity) to have a different type for the height in FillExtrudeLayer, i.e. a single property? For example, when specifying pixel values in Compose, we also don't have a Px type but just use floats. It's not worth to introduce types for the sake of it. You've seen that it complicates other things, such as having to provide (operator, etc.) overloads for each of those types - or at the end deciding not to do it because it is too much effort, which then in any case makes it less comfortable to use for users than if it wasn't an own type to begin with.
The reason why there is a Dp type in Compose is to clearly distinguish it from pixels and make clear that it must be divided by the screen pixel density for drawing.

[sargunv]

I think it's not worth the effort just for that single property. I think it is worth the effort for the scale bar API and implementation. That's the place we have multiple length units involved and arithmetic with them.

So the question from there is, assuming we keep the dependency for the scale bar, is it worth also using the dependency for the map scale type and the fill extrusion property.

If both are no, we could keep this as a dependency of the material3 extensions module and kick it out of the core module (but, would it return to core if some of this scale bar logic needs to be reused in a different UI module?)

If map scale is yes, imo the fill extrusion height should use it just for consistency.

I'm not convinced map scale should use it though; I'm leaning keep it in the material3 extensions module and revert the changes in core for now.

If alchemist hits an api-stable v1 before we do*, I'd be more likely to bring it back into the map scale type.

*I think we can't guarantee API stability before our dependencies do, due to how gradle resolves conflicting versions. So what I mean is if we're ready for v1 and alchemist being <v1 is our only blocker, I'd fork Length and LengthUnit into our project and go to v1. Realistically, I think we have a longer road to get to v1 than alchemist, so I hope that doesn't come up.