Add AequilibraE support #1
Conversation
| redraw: { type: Number, required: true }, | ||
| screenshot: { type: Number, required: true }, | ||
| viewId: { type: Number, required: true }, | ||
| lineWidthUnits: { type: String, required: false, default: 'pixels' }, |
There was a problem hiding this comment.
We're changing a separate/unrelated plugin here. Not ideal, but the default leaves it effectively unchanged
There was a problem hiding this comment.
Pull request overview
This PR adds support for the AequilibraE transportation modeling software to SimWrapper. The implementation includes a new plugin system for visualizing AequilibraE SQLite database outputs on interactive maps, S3 bucket support for cloud-hosted data, and enhancements to existing components (Plotly, LegendColors, Tiles) to support the new functionality. However, the PR also includes significant customizations (splash page, example data sources, branding) that the author acknowledges should not be merged back to the main SimWrapper project.
Key changes:
- New AequilibraE plugin with SQLite database reading, GeoJSON rendering, and advanced styling capabilities
- S3 bucket support added to HTTPFileSystem for cloud data access
- Extensions to Plotly charts for axis range configuration and existing components for meter-based units
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/aequilibrae/* | New plugin files implementing AequilibraE database reading, styling engine, and map visualization |
| src/plugins/pluginRegistry.ts | Registers the new aeq-reader plugin with file pattern matching |
| src/dash-panels/aequilibrae.vue | Dashboard panel wrapper for AequilibraE viewer |
| src/dash-panels/_allPanels.ts | Adds aequilibrae to panel lookup registry |
| src/types/spl.d.ts, cartocolor.d.ts | TypeScript declarations for new dependencies |
| src/js/HTTPFileSystem.ts | Adds S3 bucket support with XML directory listing via regex parsing |
| src/js/util-worker.ts | New worker-safe utility functions (not used in this PR) |
| src/Globals.ts | Adds isS3 flag to FileSystemConfig interface |
| src/plugins/shape-file/DeckMapComponent.vue | Adds configurable line/point units (pixels vs meters) |
| src/plugins/plotly/PlotlyDiagram.vue | Adds support for xMin/xMax/yMin/yMax axis range configuration |
| src/components/LegendColors.vue | Extends legend to support subtitles and multiple shape types |
| src/dash-panels/tile.vue | Changes default tile colors, fonts, and styling (affects all dashboards) |
| src/layout-manager/SplashPage.vue | Removes most content - customization not intended for merge |
| src/fileSystemConfig.ts | Replaces default data sources with AequilibraE example - should be reverted |
| index.html | Changes title to "AequilibraE Explore" - should be reverted |
| package.json | Adds cartocolor, spl.js, and problematic sqlite3/spatialite dependencies |
Comments suppressed due to low confidence (1)
src/plugins/aequilibrae/styling.ts:88
- Unused variable nums.
const nums = values.filter((v): v is number => v !== null)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (hasXRange) { | ||
| mergedLayout.xaxis.range = [ | ||
| xMin !== undefined ? xMin : null, | ||
| xMax !== undefined ? xMax : null | ||
| ] | ||
| mergedLayout.xaxis.autorange = false |
There was a problem hiding this comment.
Setting range array elements to null when only one boundary (xMin or xMax) is provided could cause issues with Plotly. Plotly's range property expects either a valid [min, max] array or to be omitted entirely. When only xMin is set, the range becomes [value, null] which may not work as expected. Consider using the calculated min/max values as fallbacks instead of null, or only set the range when both boundaries are provided.
| const buildCategoryEncoder = (colors: Record<string, string>, defaultColor: string = '#808080') => { | ||
| const colorMap = new Map<string, RGBA>() | ||
| for (const [key, hex] of Object.entries(colors)) { | ||
| colorMap.set(String(key), hexToRgb(hex)) | ||
| } | ||
| const defaultRgba = hexToRgb(defaultColor) | ||
|
|
||
| return (value: any) => { | ||
| return colorMap.get(String(value)) || defaultRgba | ||
| } | ||
| } |
There was a problem hiding this comment.
The function buildCategoryEncoder returns RGB but is used in contexts expecting RGBA. At line 69, it creates a Map of RGBA values but at line 71 it stores hexToRgb results which return RGB (3 elements). This causes a type mismatch when the encoder is used at lines 180, 182, 207, and 209 where the result is passed to set() on arrays expecting 4-element (fillColors) or 3-element (lineColors) arrays. Consider changing line 71 to use hexToRgba for consistency with the return type, or adjust the return type to match the actual RGB implementation.
| if (this.traces[i].xaxis_min !== undefined && this.traces[i].xaxis_min < this.minXValue) | ||
| this.minXValue = this.traces[i].xaxis_min | ||
| if (this.traces[i].yaxis_min !== undefined && this.traces[i].yaxis_min < this.minYValue) | ||
| this.minYValue = this.traces[i].yaxis_min |
There was a problem hiding this comment.
The trailing space at the end of line 274 should be removed for code cleanliness.
| this.minYValue = this.traces[i].yaxis_min | |
| this.minYValue = this.traces[i].yaxis_min |
| if (hasYRange) { | ||
| mergedLayout.yaxis.range = [ | ||
| yMin !== undefined ? yMin : null, | ||
| yMax !== undefined ? yMax : null | ||
| ] | ||
| mergedLayout.yaxis.autorange = false | ||
| } else if (yRangeFromLayout) { | ||
| mergedLayout.yaxis.autorange = false | ||
| } else { | ||
| mergedLayout.yaxis.autorange = true | ||
| } | ||
| // bug #357: scatterplots fail if rangemode is set | ||
| if (!this.traces.find(a => a?.type == 'scatter')) { | ||
| if (!mergedLayout.yaxis.rangemode) mergedLayout.yaxis.rangemode = 'tozero' | ||
| } | ||
| if (!mergedLayout.yaxis.title) mergedLayout.yaxis.title = this.layout.yaxis.title | ||
| } else { | ||
| mergedLayout.yaxis = this.layout.yaxis | ||
| mergedLayout.yaxis = { ...this.layout.yaxis } | ||
| if (hasYRange) { | ||
| mergedLayout.yaxis.range = [ | ||
| yMin !== undefined ? yMin : null, | ||
| yMax !== undefined ? yMax : null | ||
| ] | ||
| mergedLayout.yaxis.autorange = false | ||
| } |
There was a problem hiding this comment.
The same null-in-range issue exists for yaxis. When only yMin or yMax is provided, the range array will contain null values which Plotly may not handle correctly. This is duplicated in multiple places (lines 338-341, 352-356, 366-369, 385-389, 416-420, 424-428). Consider consolidating this logic and using calculated values as fallbacks instead of null.
| stride: number, | ||
| offset: number | ||
| ) => { | ||
| const nums = values.filter((v): v is number => v !== null) |
There was a problem hiding this comment.
The variable 'nums' is created but never used. This filtering step appears unnecessary since the values aren't used afterward. Consider removing this line to improve code clarity.
| const nums = values.filter((v): v is number => v !== null) |
| "spatialite": "^0.1.0", | ||
| "spl.js": "^0.1.2", | ||
| "sql.js": "^1.13.0", | ||
| "sqlite3": "^5.1.7", |
There was a problem hiding this comment.
The package.json includes sqlite3 and spatialite packages which are Node.js native modules that cannot run in browser environments. SimWrapper is a client-side browser application as mentioned in the PR description. These dependencies will likely cause build failures or runtime errors. The spl.js package appears to be the correct browser-compatible SQLite library - consider removing sqlite3 and spatialite from dependencies unless they're used in a Node.js build process.
| '#dddddd00', // light gray | ||
| '#dddddd00', // light gray | ||
| '#dddddd00', // light gray | ||
| '#dddddd00', // light gray |
There was a problem hiding this comment.
The tile component now uses transparent background colors (#dddddd00) for the first 4 tiles. This dramatically changes the visual appearance of all tile panels throughout the application, not just for AequilibraE. This could be a breaking change for existing dashboards. If this styling is specific to AequilibraE needs, consider implementing it as an optional tile configuration rather than changing the default colors for all tiles.
| */ | ||
|
|
||
| import HTTPFileSystem from '../../js/HTTPFileSystem' | ||
| import { FileSystemConfig, DirectoryEntry } from '@/Globals' |
There was a problem hiding this comment.
Unused import DirectoryEntry.
| import { FileSystemConfig, DirectoryEntry } from '@/Globals' | |
| import { FileSystemConfig } from '@/Globals' |
| @@ -0,0 +1,309 @@ | |||
| import * as cartoColors from 'cartocolor' | |||
| import { RGBA, RGB, ColorStyle, NumericStyle, LayerStyle, BuildArgs, BuildResult } from './types' | |||
There was a problem hiding this comment.
Unused import NumericStyle.
| import { RGBA, RGB, ColorStyle, NumericStyle, LayerStyle, BuildArgs, BuildResult } from './types' | |
| import { RGBA, RGB, ColorStyle, LayerStyle, BuildArgs, BuildResult } from './types' |
| } | ||
|
|
||
| // Apply per-layer styles | ||
| for (const [layerName, bucket] of buckets.entries()) { |
There was a problem hiding this comment.
Unused variable layerName.
| for (const [layerName, bucket] of buckets.entries()) { | |
| for (const bucket of buckets.values()) { |
…umentation and refactoring
This PR adds AequilibraE support to SimWrapper. This involves,
src/plugins/aequilibrae/...src/plugins/pluginRegistry.tssrc/dash-panels/aequilibrae.vue(and_allPanels.ts)HTTPFileSystem.tsto add support for S3 bucketsIn my testing I've modified a few other scripts (Splash, deploy scripts, etc) but we don't want to suggest merging these back to the original SimWrapper project, so I will revert. Additionally, current work needs a good refactor, particularly