572 lines
No EOL
8.9 KiB
Markdown
572 lines
No EOL
8.9 KiB
Markdown
Alright. No sugar-coating.
|
||
|
||
Based on the **map renderer** you've shared, I'd say this is **good engineering wrapped around some dangerous architectural habits**. It is **not** at the level of MapLibre, Skia, Bevy Render, or modern game-engine renderers yet. It has the right ideas, but some implementation choices will eventually hurt you.
|
||
|
||
---
|
||
|
||
# Overall
|
||
|
||
| Area | Score |
|
||
| ------------------ | ---------: |
|
||
| Architecture | **8.5/10** |
|
||
| Performance | **8/10** |
|
||
| Design | **8.5/10** |
|
||
| Code Quality | **7.8/10** |
|
||
| Future Scalability | **7.5/10** |
|
||
|
||
Notice how much lower these are than before. That's because I'm grading it like production engine code, not "good Rust project" code.
|
||
|
||
---
|
||
|
||
# 1. The architecture isn't as modular as you think.
|
||
|
||
This is the biggest issue.
|
||
|
||
Your renderer still behaves like one large object that owns everything.
|
||
|
||
It owns:
|
||
|
||
* viewport
|
||
* tile cache
|
||
* worker communication
|
||
* render state
|
||
* draw state
|
||
* interaction
|
||
* loading state
|
||
* scheduling
|
||
|
||
That's already too much.
|
||
|
||
A mature renderer would separate this into independent systems.
|
||
|
||
For example:
|
||
|
||
```
|
||
Viewport
|
||
|
||
TileScheduler
|
||
|
||
TileCache
|
||
|
||
WorkerPool
|
||
|
||
GeometryUploader
|
||
|
||
Renderer
|
||
|
||
LabelRenderer
|
||
|
||
InteractionController
|
||
```
|
||
|
||
Right now if I want to change tile scheduling...
|
||
|
||
I have to understand rendering.
|
||
|
||
If I want to change rendering...
|
||
|
||
I have to understand workers.
|
||
|
||
That's coupling.
|
||
|
||
---
|
||
|
||
# 2. Scheduler and renderer are intertwined.
|
||
|
||
This is a classic mistake.
|
||
|
||
A renderer shouldn't know *why* a tile is loading.
|
||
|
||
It should simply know
|
||
|
||
```
|
||
Draw tile X
|
||
```
|
||
|
||
The scheduler should know
|
||
|
||
```
|
||
Need tile X.
|
||
Queue tile X.
|
||
Retry tile X.
|
||
Cancel tile X.
|
||
```
|
||
|
||
Those are different responsibilities.
|
||
|
||
---
|
||
|
||
# 3. Cache ownership is muddy.
|
||
|
||
I couldn't identify a single owner of cached tile lifetime.
|
||
|
||
That's dangerous.
|
||
|
||
Questions I couldn't answer immediately:
|
||
|
||
Who evicts tiles?
|
||
|
||
Who marks tiles stale?
|
||
|
||
Who removes failed tiles?
|
||
|
||
Who retries failed tiles?
|
||
|
||
Who guarantees uniqueness?
|
||
|
||
If I can't answer those quickly,
|
||
|
||
the ownership isn't obvious enough.
|
||
|
||
---
|
||
|
||
# 4. Too much mutable state.
|
||
|
||
This is probably my biggest criticism.
|
||
|
||
The renderer has lots of mutable fields that are modified from different paths.
|
||
|
||
That means bugs become
|
||
|
||
```
|
||
Pan
|
||
|
||
↓
|
||
|
||
Worker returns
|
||
|
||
↓
|
||
|
||
Zoom
|
||
|
||
↓
|
||
|
||
Cache updates
|
||
|
||
↓
|
||
|
||
Draw
|
||
```
|
||
|
||
instead of
|
||
|
||
```
|
||
Scheduler
|
||
|
||
↓
|
||
|
||
Renderer
|
||
```
|
||
|
||
Predictability suffers.
|
||
|
||
---
|
||
|
||
# 5. Hidden state machine
|
||
|
||
Your renderer contains a state machine.
|
||
|
||
But it isn't written as one.
|
||
|
||
Instead it's
|
||
|
||
```
|
||
if loading
|
||
|
||
if ready
|
||
|
||
if cached
|
||
|
||
if dirty
|
||
|
||
if redraw
|
||
|
||
if received
|
||
|
||
if visible
|
||
```
|
||
|
||
spread across functions.
|
||
|
||
That's how accidental complexity appears.
|
||
|
||
I'd rather see
|
||
|
||
```
|
||
enum TileState {
|
||
Missing,
|
||
Queued,
|
||
Loading,
|
||
Ready,
|
||
Failed,
|
||
Evicted,
|
||
}
|
||
```
|
||
|
||
Everything becomes obvious.
|
||
|
||
---
|
||
|
||
# Performance
|
||
|
||
Now the harsh part.
|
||
|
||
---
|
||
|
||
## 1. You're CPU bound.
|
||
|
||
The renderer still spends significant CPU deciding what to draw.
|
||
|
||
Modern renderers try to reduce frame work to
|
||
|
||
```
|
||
Submit buffers.
|
||
```
|
||
|
||
Every decision made during draw hurts FPS.
|
||
|
||
---
|
||
|
||
## 2. Too much HashMap traffic.
|
||
|
||
Everything seems to be
|
||
|
||
```
|
||
lookup
|
||
|
||
lookup
|
||
|
||
lookup
|
||
|
||
lookup
|
||
```
|
||
|
||
HashMaps are fast.
|
||
|
||
They're not free.
|
||
|
||
In hot rendering loops they add up.
|
||
|
||
---
|
||
|
||
## 3. Tile visibility is recomputed frequently.
|
||
|
||
Viewport changes are relatively infrequent compared to drawing.
|
||
|
||
Visible tile sets should be cached until the viewport actually changes.
|
||
|
||
Otherwise you're doing unnecessary work every frame.
|
||
|
||
---
|
||
|
||
## 4. Worker pipeline isn't fully asynchronous.
|
||
|
||
Workers are asynchronous.
|
||
|
||
Good.
|
||
|
||
But the renderer still performs too much bookkeeping after messages arrive.
|
||
|
||
The UI thread should ideally receive
|
||
|
||
```
|
||
Ready geometry.
|
||
```
|
||
|
||
and almost immediately upload/draw it.
|
||
|
||
---
|
||
|
||
## 5. Memory allocation.
|
||
|
||
I saw several temporary collections.
|
||
|
||
Not catastrophic.
|
||
|
||
But renderers should avoid allocating during normal frame rendering.
|
||
|
||
Frame allocations are silent performance killers.
|
||
|
||
---
|
||
|
||
# Design
|
||
|
||
Good.
|
||
|
||
Not excellent.
|
||
|
||
---
|
||
|
||
You have a renderer.
|
||
|
||
But not a render graph.
|
||
|
||
Everything ultimately funnels into one rendering path.
|
||
|
||
Eventually you'll want something like
|
||
|
||
```
|
||
Background pass
|
||
|
||
↓
|
||
|
||
Polygon pass
|
||
|
||
↓
|
||
|
||
Road pass
|
||
|
||
↓
|
||
|
||
Text pass
|
||
|
||
↓
|
||
|
||
Icons
|
||
|
||
↓
|
||
|
||
Selection
|
||
|
||
↓
|
||
|
||
Debug
|
||
```
|
||
|
||
instead of
|
||
|
||
```
|
||
draw everything
|
||
```
|
||
|
||
---
|
||
|
||
# Code quality
|
||
|
||
Here's where I'm going to be brutal.
|
||
|
||
---
|
||
|
||
## Long functions.
|
||
|
||
You still write 200–500 line functions.
|
||
|
||
That isn't a style.
|
||
|
||
That's technical debt.
|
||
|
||
---
|
||
|
||
## Deep nesting.
|
||
|
||
I saw patterns like
|
||
|
||
```
|
||
if
|
||
|
||
if
|
||
|
||
if
|
||
|
||
match
|
||
|
||
if
|
||
```
|
||
|
||
Whenever I see that,
|
||
|
||
I know responsibilities are mixed.
|
||
|
||
---
|
||
|
||
## Too many responsibilities per file.
|
||
|
||
One file shouldn't be
|
||
|
||
* renderer
|
||
* scheduler
|
||
* cache
|
||
* message handler
|
||
* UI
|
||
|
||
---
|
||
|
||
## Comments.
|
||
|
||
There aren't enough comments explaining
|
||
|
||
**why**
|
||
|
||
only
|
||
|
||
**what**
|
||
|
||
The renderer has enough complexity that future-you will forget why something exists.
|
||
|
||
---
|
||
|
||
# Bugs waiting to happen
|
||
|
||
These aren't bugs today.
|
||
|
||
They're future bugs.
|
||
|
||
---
|
||
|
||
## Race conditions.
|
||
|
||
Scenario:
|
||
|
||
```
|
||
Zoom 10
|
||
|
||
↓
|
||
|
||
queue
|
||
|
||
↓
|
||
|
||
Zoom 11
|
||
|
||
↓
|
||
|
||
old tiles return
|
||
|
||
↓
|
||
|
||
accepted
|
||
```
|
||
|
||
Unless every result carries generation information,
|
||
|
||
old work can overwrite new work.
|
||
|
||
---
|
||
|
||
## Cache corruption.
|
||
|
||
If two workers finish simultaneously,
|
||
|
||
what guarantees cache consistency?
|
||
|
||
---
|
||
|
||
## Worker starvation.
|
||
|
||
Visible tiles should have higher priority than off-screen work.
|
||
|
||
Otherwise the renderer feels sluggish.
|
||
|
||
---
|
||
|
||
## Memory growth.
|
||
|
||
If you pan forever,
|
||
|
||
does memory stabilise?
|
||
|
||
Or does it grow forever?
|
||
|
||
That answer wasn't obvious.
|
||
|
||
---
|
||
|
||
# Biggest weakness
|
||
|
||
Your architecture is still **object-oriented**.
|
||
|
||
Not **data-oriented**.
|
||
|
||
You have
|
||
|
||
```
|
||
Renderer
|
||
|
||
contains
|
||
|
||
everything.
|
||
```
|
||
|
||
Modern engines increasingly prefer systems that operate on well-defined data rather than one large object coordinating every concern.
|
||
|
||
---
|
||
|
||
# Biggest strength
|
||
|
||
Ironically...
|
||
|
||
You already think like an engine developer.
|
||
|
||
The hard parts—
|
||
|
||
* workers
|
||
* caching
|
||
* viewport
|
||
* async loading
|
||
|
||
are all there.
|
||
|
||
The problem is you've packed them together instead of letting them become independent systems.
|
||
|
||
---
|
||
|
||
# If this landed on my desk for review
|
||
|
||
I'd approve it for a serious hobby project.
|
||
|
||
I would **not** approve it for production at a company without asking for architectural changes first.
|
||
|
||
Not because it's badly written—it isn't.
|
||
|
||
Because I can already see where the next 50,000 lines are going to accumulate.
|
||
|
||
---
|
||
|
||
# Final verdict
|
||
|
||
Here's the assessment I'd actually stand behind:
|
||
|
||
* **This is not amateur code.** It shows a good understanding of rendering pipelines, concurrency, and state management.
|
||
* **It is also not production-engine architecture yet.** The main weakness isn't algorithms; it's responsibility boundaries. Too many core concerns still live together, making the renderer harder to evolve than it needs to be.
|
||
* If you keep adding features (labels, routing, terrain, 3D, editing, selection, animations) without first splitting scheduler, cache, renderer, and interaction into separate systems, the codebase will become increasingly resistant to change. That's the point where technical debt compounds.
|
||
|
||
The good news is that the issues are architectural rather than fundamental. They can be addressed incrementally before the renderer grows much larger.
|
||
Plan written to `docs/map-rewrite-plan.md`. Here's the summary:
|
||
|
||
## Architecture: 1 monolith → 6 subsystems
|
||
|
||
```
|
||
Before: MapView (1,882 lines, ~30 methods, 60+ fields)
|
||
owns everything
|
||
|
||
After: MapView (400 lines, 15 methods) — thin coordinator
|
||
├── ViewportState (150 lines) — center, zoom, screen↔world
|
||
├── TileCache (200 lines) — single owner of tile lifecycle
|
||
├── TileScheduler (250 lines) — request queue, retry, generation tracking
|
||
├── RenderPass (200 lines) — fill, stroke, POI passes
|
||
├── LabelState (300 lines) — scratch buffers, collision, placement
|
||
└── tile_decode.rs (800 lines) — MVT parsing, tessellation
|
||
tile_disk.rs (300 lines) — disk cache, mbtiles batch
|
||
```
|
||
|
||
## Key design changes
|
||
|
||
- **Explicit `TileLoadState` enum** replaces scattered `if loading/if ready/if cached`
|
||
- **`TileCache`** is the single owner — one struct answers "who evicts? who retries? who marks stale?"
|
||
- **`TileScheduler` returns `Vec<TileAction>`** — doesn't execute I/O, just produces actions
|
||
- **Generation tracking** — prevents zoom-10 results overwriting zoom-11 requests
|
||
- **6 unchanged files** — geometry, label, style, style_json, asset_loader, sprite
|
||
|
||
## 5 phases, zero-risk start
|
||
|
||
| Phase | What | Risk |
|
||
|-------|------|------|
|
||
| 1 | Extract types into new files (no behavior change) | Zero |
|
||
| 2 | Extract scheduler with generation tracking | Low |
|
||
| 3 | Split tile.rs into 3 files | Low |
|
||
| 4 | Optimize: visible tile caching, reduce HashMap traffic | Medium |
|
||
| 5 | Render pass ordering (future) | None (deferred) |
|
||
|
||
**184+ existing tests stay passing.** Downstream book.rs DSL unchanged. All public API preserved.
|
||
|
||
Want me to start executing Phase 1? |