Allow re-usable shield definitions

[ZeLonewolf]

Chipping away at #889.

We should be able to define a shield definition by name, and then reference that name in other parts of the ShieldJSON make a list of the networks that should be drawn in that style.

The current structure is:

{
  "networks": {
    "default": {
      "key": "value"
    },
    "network1": {
      "key": "value"
    },
    "network2": {
      "key": "value"
    }
  }
}

A reorganized format which eliminates repetition might look something like this

{
  "defaults": {
    "key": "value"
  },
  "definitions": {
    "white_circle": {
      "mapping": {
        [
          {
            "networks": ["US:NJ", "US:DE"],
          },
          {
            "networks": ["US:NJ:Truck", "US:DE:Truck"],
            "banners": ["TRK"]
          }
        ]
      }
      "key": "value"
    },

  }
}

In that form, the "key":"value" represents all the elements of the current definition language that define the color, shape, etc., of the shields.

This inverts the current structure and would necessitate a full re-write or replacement of the shield_defs.js file that currently generates the ShieldJSON. Suggestions welcome on the best way to structure this.

[1ec5]

Even an intermediate refactor would enable the definitions to be reused more broadly. For example, this definition:

openstreetmap-americana/src/js/shield_defs.js

Lines 169 to 175 in 9df8b99

	shields["CA:MB:Winnipeg"] = trapezoidDownShield(
	10,
	Color.shields.white,
	Color.shields.black,
	Color.shields.black,
	2
	);

which calls this function directly:

openstreetmap-americana/shieldlib/src/shield_helper.d.ts

Lines 50 to 57 in 9df8b99

	export declare function trapezoidDownShield(
	sideAngle: number,
	fillColor: string,
	strokeColor: string,
	textColor: string,
	radius: number,
	rectWidth: number
	): ShieldDefinition;

could be rewritten as:

{
  "CA:MB:Winnipeg": {
    "type": "trapezoidDownShield",
    "sideAngle": 10,
    "fillColor": "white",
    "strokeColor": "black",
    "textColor": "black",
    "radius": 2
  }
}

It would be more verbose than what we have now, but it would be easier to validate and transform to various formats, and we could more naturally split it up into multiple files, similar to how name-suggestion-index and id-tagging-schema are structured. However, if we prefer the current format for less redundancy, we could have it generate this JSON instead. The problem with the current implementation is that each of these drawing methods inserts a private object structure into the definition that other code has to tiptoe around. It would be hacky for this other code to have to root around in shieldDef.shapeBlank.params.fillColor for example.

[ZeLonewolf]

I'm not clear on what is being proposed with the suggested rewrite. As written, all it is doing is flattening the JSON structure down to a single level of hierarchy rather than a more nested structure. I don't object to flattening the JSON structure if that's beneficial, but it doesn't attack the problem of repeated data.

I also don't understand the comment that it's "more verbose". If anything, it's slightly less verbose because it removes some nesting and brings all of the shape attributes down to the top level of hierarchy.

I also don't understand how it achieves this ticket's goal of re-usable shield definitions, since we would still be repeating the entire shield structure for each network that has the same shield, which is the main cause of our ShieldJSON being >1MB400kb in size.

My goal in opening this ticket is to reduce the overall size of the style.json shields.json

[zekefarwell]

ShieldJSON being >1MB in size

If you're referring to dist/shields.json, it looks to me like that file is a little over 400KB.

My goal in opening this ticket is to reduce the overall size of the style.json

Isn't shields.json entirely separate from the style.json? It seems style.json manages to be 1MB without any help from shields.json.

[ZeLonewolf]

Thanks - typos on my part - this ticket is solely about the ShieldJSON.

[zekefarwell]

Is the shields.json file measurably contributing to slow performance in some way? All the redundancy means the ~400KB gzips down to ~17KB transfer size so shouldn't be too much of a file download burden.

Perhaps the initial parse time can be significant though? Or if not, maybe the current file size isn't much of a drag on performance (compared to the style.json which definitely is). Either way, refactoring could still be helpful just from an organizational perspective.

[ZeLonewolf]

I have honestly not benchmarked any of this and am assuming the smallest possible definition file is desirable from a transfer time and memory perspective. But maybe this is negligible in a mordern context. Perhaps a more fruitful first step that isn't controversial would be to have network keys that can accept a wildcard match to eliminate many of the county shield cases that bloat the ShieldJSON. The current scheme also (IMO) precludes us just checking in the shields.json into the repo. It's unwieldy to work with in its verbosity and necessitates code to generate.

[1ec5]

As written, all it is doing is flattening the JSON structure down to a single level of hierarchy rather than a more nested structure.

No, that’s not at all what my suggestion is about. Currently, each definition resolves to an object, but only after calling one or more functions that are defined elsewhere and come with plenty of dependencies.

As a mobile developer or someone writing a different kind of tool around shields, I want a file that has all the same data that’s contained in shield_defs.js, without any dependencies. The current JSON file is unsuitable because it has already been transformed by various functions. For example, colors have already been resolved to colors specific to the Americana style. Some shapes have been decomposed into more basic shapes. Default parameters for those shapes are now baked into the definition. I want to be able to perform those transformations separately or not at all.

[ZeLonewolf]

Currently, each definition resolves to an object, but only after calling one or more functions that are defined elsewhere and come with plenty of dependencies.

I don't understand this comment. Each definition contains a set of values that completely describe the shape and visual appearance of the shield, regardless of how that JSON structure is generated.

The current JSON file is unsuitable because it has already been transformed by various functions. For example, colors have already been resolved to colors specific to the Americana style.

Yes, that is what the shields.json does - it describes the exact appearance that tells maplibre what to draw for a shield. If another user wants to draw shields with different colors, they would need to generate their own version of shields.json with their own color schemes. That was always the intent, that the shield definition described how you went from a network value to what specifically is drawn (or not drawn) on the screen.

Some shapes have been decomposed into more basic shapes. Default parameters for those shapes are now baked into the definition. I want to be able to perform those transformations separately or not at all.

I don't understand this description either or what you mean by decomposed. I think at this point it would be helpful if you could more fully compose your vision for how shield styling should be described from end to end, and leaving nothing out in the description about how the user agent decides what to draw. It sounds like you're envisioning a completely different way of describing how to draw shields that I'm not comprehending here.

[1ec5]

I think at this point it would be helpful if you could more fully compose your vision for how shield styling should be described from end to end, and leaving nothing out in the description about how the user agent decides what to draw.

#1062 (comment) is worth another look, because it highlights the pain point that others will soon face trying to reuse our shield functionality. But to be extra clear, here is my proposed acceptance criterion:

• shield_defs.js is deleted and replaced by shield_def.json or a series of such JSON files, which specify the same details that shield_defs.js used to specify.

shields.json is not what I’m after; if it were, we’d have a test verifying that we can reconstruct shield_defs.js based on shields.json, but that would be difficult and of course pointless. shields.json cannot distinguish between a rounded rectangle and a pill because this helper function has already been evaluated:

openstreetmap-americana/shieldlib/src/shield_helper.ts

Lines 668 to 694 in 7488b82

	export function pillShield(
	fillColor: string,
	strokeColor: string,
	textColor: string,
	rectWidth: number
	): Partial<ShieldDefinition> {
	textColor = textColor ?? strokeColor;
	return {
	shapeBlank: {
	drawFunc: "roundedRectangle",
	params: {
	fillColor,
	strokeColor,
	rectWidth,
	radius: 10,
	},
	},
	textLayout: textConstraint("ellipse"),
	padding: {
	left: 2,
	right: 2,
	top: 2,
	bottom: 2,
	},
	textColor,
	};
	}

One part of this codebase already has to reverse-engineer this default:

openstreetmap-americana/scripts/taginfo.js

Line 142 in 7488b82

if (shapeDef.params.radius == 10) {

This is not mutually exclusive of what you’ve proposed. To the extent that anything in this JSON file would be truly specific to the Americana style, we could factor it out as you’ve proposed, but I see that as a followup task. I see a disconnect between the title of this issue, which speaks of reusability, and the contents of it, which is about performance optimization.

If another user wants to draw shields with different colors, they would need to generate their own version of shields.json with their own color schemes.

If a mobile developer wants to be able to support Dynamic Type on their map by enlarging shields without necessarily enlarging every shape’s metrics by the same scale factor or thickening every stroke by the same factor, would we require the developer to maintain a series of forks of Americana just to do that?

[ZeLonewolf]

• shield_defs.js is deleted and replaced by shield_def.json or a series of such JSON files, which specify the same details that shield_defs.js used to specify.

Checking in the generated shield.json would accomplish this point, but that would be worse because it would be less maintainable than the current setup. I don't know why you're conflating WHAT is in shield.json with HOW it is generated/managed. These are completely independent concerns. Converting the format of shield.json into something more manageable IMO is a pre-requisite for checking in a JSON file vice having code to generate the JSON file.

shields.json cannot distinguish between a rounded rectangle and a pill

One part of this codebase already has to reverse-engineer this default:

openstreetmap-americana/scripts/taginfo.js

Line 142 in 7488b82

if (shapeDef.params.radius == 10) {

We could just fix that issue and allow the JSON to declare a "pill" rather than overloading rounded rectangle. It wouldn't be a difficult change.

This is not mutually exclusive of what you’ve proposed. To the extent that anything in this JSON file would be truly specific to the Americana style, we could factor it out as you’ve proposed, but I see that as a followup task. I see a disconnect between the title of this issue, which speaks of reusability, and the contents of it, which is about performance optimization.

Right, I seek to re-use definitions within the JSON file in order reduce repetition and make the shield.json small enough that it would be sane to check it in (or decompose it into several JSON files that would be sane to check in). I view the contents of shield.json fundamentally specific to the Americana style, and if someone desires a different style, they would need to have their own version of shield.json to do that.

If a mobile developer wants to be able to support Dynamic Type on their map by enlarging shields without necessarily enlarging every shape’s metrics by the same scale factor or thickening every stroke by the same factor, would we require the developer to maintain a series of forks of Americana just to do that?

That sounds like a global transform that should happen at the library level, not at the shield definition level.

I specifically openend this ticket going after internal re-usability within the existing information held in shield.json -- it sounds like you're looking for external re-usability. That's a valid and important goal, but we would need to be very specific about what information is externally re-usable and therefore needs to be segregated from the big heap.

[1ec5]

I specifically openend this ticket going after internal re-usability within the existing information held in shield.json -- it sounds like you're looking for external re-usability.

Ah, that explains it – I totally misunderstood what you meant by “reusable”. Carry on!

[zekefarwell]

We should be able to define a shield definition by name, and then make a list of the networks that should be drawn in that style.

Currently the top level object keys are network codes which makes for fast matching of network code to shield definition at runtime. I believe this is where it happens:

openstreetmap-americana/shieldlib/src/shield.ts

Lines 172 to 179 in 7488b82

	function getShieldDef(shields: ShieldDefinitions, routeDef: RouteDefinition): ShieldDefinition {
	if (!shields) {
	//This occurs if the ShieldJSON is loaded from the network and hasn't loaded yet.
	return null;
	}
	var shieldDef = shields[routeDef.network];

This idea would move the network codes into a series of sub-arrays nested under their respective shield type so this simple key lookup would no longer work. Were you thinking of transforming the data structure at build time so that such a lookup would still be possible? Or do you have a different idea for how the runtime lookup would work?

[ZeLonewolf]

Were you thinking of transforming the data structure at build time so that such a lookup would still be possible? Or do you have a different idea for how the runtime lookup would work?

I envision that, at runtime, a Map would be populated on initial load. I would expect that population a map is trivially fast and would be "paid for" in reduced download time of the ShieldJSON. Though it remains to be seen whether there is actually any change in size after compression.