Refactor render pipeline interfaces to decouple colorization#170
Merged
MatthewPeterKelly merged 15 commits intoMay 28, 2026
Merged
Conversation
docs/phase-2-detailed-plan.md (new, ~440 lines) — implementer-facing plan covering the 4-phase pipeline, trait shapes (ColorMapKind, revised Renderable, RenderingPipeline, colorize_collapse), per-variant Cell/Cache types, the sampling_level: i32 model, allocation strategy, the three-commit split (2.1 / 2.2 / 2.3), JSON migration script outline, and an expected-pixel-hash-delta table per fixture per commit. docs/gui-unification-roadmap.md updates: §3 phase summary table updated. §5 (Data Model) rewritten: Vec<Cell> framing, ColorMapKind with Cell/Cache/create_cache/refresh_cache/colorize_cell, RenderingPipeline<F> orchestrator, allocation-once strategy, the unified sampling_level model, and updated rationale section. §6 Phase 2 redirected to the new detailed-plan doc, with the three-commit verification summary. §6 Phase 6 simplified to "add recolorize_only + dirty flags" since the pipeline already owns the buffers. §9.2 render-trigger matrix updated to sampling_level semantics. §9.4 static-dispatch invariant tightened, plus per-frame/per-pixel zero-allocation guarantee. §11 risks: replaced the "pure refactor" Phase 2 risk with one tracking the bit-equivalence gate in 2.1 and the histogram-source hash bump in 2.3. §13 Q1 swapped from the obsolete UnifiedColorMap question to the QuadraticMap<T> wrapper-drop question.
Phase 2 commit 2.1: introduce the new compute / color split machinery end-to-end without disturbing the production runtime. - Add `ColorMapKind` trait with statically-typed `Cell` and `Cache` associated types. Per-variant impls on `ForegroundBackground`, `BackgroundWithColorMap`, and `MultiColorMap`. Cache is rebuilt in place from keyframes via `refresh_cache` (allocation-free), and `colorize_cell` is a no-`dyn` per-(sub)pixel hot-path lookup. - Extend `Renderable` with `type ColorMap`, `compute_raw_field`, `populate_histogram`, `normalize_field`, and `color_map`. The histogram and normalize methods default to no-ops (DDP path). - Add `src/core/render_pipeline.rs` with `RenderingPipeline<F>` orchestrating the four phases (compute_raw_field → populate_histogram → normalize_field → refresh_cache + colorize_collapse) against preallocated buffers. The generic free function `colorize_collapse` walks the row-major output `egui::ColorImage`, averaging `(n+1)²` subpixel `[u8;3]` results per output pixel; fully monomorphized over the `ColorMapKind`. - Implement the new methods on `QuadraticMap<T>`, `DrivenDampedPendulumParams`, and `NewtonsMethodRenderable<F>`. Histogram source for the gradient fractals continues to use the legacy sub-sample grid in 2.1; 2.3 switches to a full-field walk. - Rename `QuadraticMapParams::color_map` -> `color_map_params` so the renderer-level `color_map()` does not shadow the params accessor; touches mandelbrot.rs, julia.rs, benches/benchmark.rs. - Add unit tests on each `ColorMapKind` impl (None / endpoint / mid / out-of-range root index / refresh-on-edit) and on `colorize_collapse` (synthetic 4x4 AA averaging, no-AA single-cell, gradient endpoints). - Add `tests/phase_2_pixel_equivalence_tests.rs` exercising the new pipeline against the legacy `Renderable::render_to_buffer` path on small Mandelbrot/Julia/Newton/DDP fixtures. DDP asserts strict equality (no lookup table); the gradient fractals assert per-channel diffs within `MAX_GRADIENT_DIFF=32`. The looser tolerance reflects an intentional architectural change: the legacy LUT is built over `[cdf.min_data, cdf.max_data]` with the CDF baked into each entry, while the new cache is built over `[0, 1]` with the CDF applied per cell in `normalize_field`. The latter shape is a hard requirement for Phase 6 re-colorize-only (cache stays valid when the CDF doesn't change), but the two quantization schemes can disagree by a small amount near steep gradient transitions. Phase 2.2 will regenerate pixel-hash regression fixtures accordingly. - New machinery is `#[allow(dead_code)]` until 2.2 wires it into the production runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… sampling_level
Phase 2 commit 2.2: production runtime now goes through
`RenderingPipeline`. The legacy compute/colorize path is removed.
- `RenderOptions` collapses `subpixel_antialiasing: u32` and
`downsample_stride: usize` into a single signed `sampling_level: i32`.
Positive = AA, 0 = baseline, negative = block-fill (nearest-neighbor;
the legacy `KeyframeLinearPixelInerpolation` interpolation is dropped).
`SpeedOptimizer for RenderOptions` mutates the unified axis.
- `image_utils::render` (offline render) and `PixelGrid` (interactive)
both go through `RenderingPipeline::render`. `Renderable::render_point`
and the default `render_to_buffer` are deleted; each fractal's impl
now exposes `histogram_bin_count`, `histogram_max_value`,
`lookup_table_count`, plus the `compute_raw_field` /
`populate_histogram` / `normalize_field` / `color_map` methods added in
2.1. `compute_raw_field` (and the colorize/normalize walks) handles
positive AA, baseline, and negative block-fill against a field sized
for the user's max sampling level.
- `QuadraticMap<T>` slims down to just `{ fractal_params }`. Histogram,
CDF, and color cache live in the pipeline.
`NewtonsMethodRenderable<F>` slims down to `{ params, system }`.
- Delete `generate_scalar_image*`, `wrap_renderer_with_antialiasing`,
`KeyframeLinearPixelInerpolation`, `render_image_internal`,
`fill_skipped_entries`, `render_single_row_within_image`,
`PixelRenderLambda`, `subpixel_offset_vector`, `create_buffer`,
`create_empty_histogram`, and `src/fractals/utilities.rs`. The
`display_buffer_to_color_image` transposition is gone too — the
pipeline writes directly into the row-major `egui::ColorImage`.
`SubpixelGridMask` / `UpsampledPixelMapper` / `SubpixelIndex` are
retained for `chaos_game` (out of scope per roadmap §9.5).
- Benchmark migrated to drive `RenderingPipeline::render` end-to-end.
- All 39 example / regression / bench JSON files migrated to
`sampling_level` via `scripts/migrate_phase_2_render_options.py`
(committed alongside).
- Pixel-hash regression fixtures regenerated for Mandelbrot, Julia,
and the downsample fixture. Two distinct sources of pixel drift:
(1) The new color cache is indexed over `[0, 1]` with the CDF applied
per cell in `normalize_field`, while the legacy lookup was over
`[cdf.min_data, cdf.max_data]` with the CDF baked into each table
entry. The two quantization schemes round to different bins near
steep gradient transitions.
(2) Block-fill is nearest-neighbor; the legacy path used bilinear
interpolation between sparse samples.
DDP, Barnsley, and Sierpinski hashes are unchanged. The
`phase_2_pixel_equivalence_tests` suite (introduced in 2.1) is
removed alongside the legacy path it was comparing against.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2 commit 2.3. The histogram that drives gradient normalization for Mandelbrot, Julia, and Newton now reflects the same population the colorize pass reads, instead of an independent sub-sample grid. - `Renderable::populate_histogram` for `QuadraticMap` and `NewtonsMethodRenderable` switched to full-field walks via shared `walk_populated_*_cells` helpers (read-only siblings of the compute/normalize walks introduced in 2.2). Newton's histogram now uses smooth iteration counts instead of the integer iteration count. - `histogram_sample_count` is removed from `ColorMapParams` and `CommonParams`, and from the QuadraticMap/Newton speed-optimizer reference caches. The field is now silently ignored by serde so the existing JSONs (which still carry it) continue to deserialize. A follow-up cleanup commit can strip the dead field. - Drop `ImageSpecification::scale_to_total_pixel_count` (sub-sample grid was its only caller). Tag `NewtonRhapsonResult.iteration_count` and the `QuadraticMapParams::color_map_params_mut` trait method as `#[allow(dead_code)]` since they're public surface kept for Phase 6 / future diagnostics. DDP / Barnsley / Sierpinski paths unaffected. - Pixel-hash regression fixtures regenerated for Mandelbrot, Julia, and the downsample fixture. The hash diff vs. 2.2 reflects the histogram source change (not a structural rendering change); spot checking the rendered PNGs confirms the output looks visually consistent with the 2.2 outputs at the same parameters. DDP / Barnsley / Sierpinski hashes unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o-old) Introduce src/core/field_iteration.rs with the new `FieldKernel` trait and the three core helpers — `compute_raw_field`, `populate_histograms` (plural), and `colorize_collapse` — that Phase 3.2 will route the `RenderingPipeline` through. The helpers operate on the unified `Vec<Vec<Option<(f32, u32)>>>` field shape so a single AA / block-fill traversal serves every fractal once Phase 3.2 lands; per-fractal code will collapse to a one-line `evaluate(point)` impl. `colorize_collapse` moves verbatim from `render_pipeline.rs`; its tests move with it and the existing `BackgroundWithColorMap` / `ForegroundBackground` covers the gradient and basin shapes. New unit tests against synthetic kernels exercise: - AA traversal (sampling_level=1) writes only the first n² cells per block - baseline (sampling_level=0) writes one top-left cell per block - block-fill (sampling_level=-1) honors the stride - recorded-points kernel verifies coordinates match `PixelMapper` - gradient-index routing into the correct `histograms[k]` slot - `None` cells are skipped during histogram population - empty histograms slice is rejected at the assert boundary The new items are gated by `#[allow(dead_code)]` because Phase 3.1 is intentionally parallel-to-old: no fractal calls them yet. Pixel hashes invariant for every regression fixture (Mandelbrot, Julia, Barnsley, Serpinsky). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…grams (Phase 3.2/3.3)
Folds Phases 3.2 and 3.3 from the detailed plan into a single commit
because the trait surgery is deeply intertwined: per-fractal
`compute_raw_field` / `populate_histogram` / `normalize_field` impls
can only be deleted once every fractal uses the same `Option<(f32, u32)>`
cell shape, which is exactly what unifying the three `ColorMapKind`
variants provides. Splitting would have required a temporary lossy
bridge.
Replaces:
- `ColorMapKind` trait + `ForegroundBackground` / `BackgroundWithColorMap`
/ `MultiColorMap` impls → unified `ColorMap { flat_color, gradients }`
+ `ColorMapCache { cdfs, luts, flat }` + `colorize_cell` free function
in `src/core/color_map.rs`. Empty `gradients` rejected at serde-time.
- The `Renderable` trait’s `compute_raw_field` / `populate_histogram` /
`normalize_field` methods → gone. `Renderable` is now
`Renderable: FieldKernel + SpeedOptimizer` plus housekeeping. Adds
`color_map_mut` (used in Phase 7).
- `RenderingPipeline`’s single `Histogram` + `CumulativeDistributionFunction`
→ `Vec<Histogram>` (one per gradient); CDFs live inside `ColorMapCache`
and are rebuilt from the fresh per-gradient histograms each frame.
- The `normalize_field` pipeline phase → gone. The field stays raw
end-to-end; CDF percentile lookup happens inside `colorize_cell` at
colorize time. Five-phase pipeline now: fill / bin / cdf / lut / colorize.
- `QuadraticMap<T>` wrapper struct → gone (open question §13.1).
Mandelbrot and Julia get blanket `Renderable` / `FieldKernel` /
`SpeedOptimizer` impls over `T: QuadraticMapParams`. The existing
`KeyframeColorMap<F>` keyframe-interpolator is renamed (was
`ColorMap<F>`) to free up the name for the new struct.
- Per-fractal AA / block-fill loops → gone. Each fractal exposes a
one-line `FieldKernel::evaluate` that calls its existing scalar
computation. Newton emits `(smooth_iter, root_index)`; Mandelbrot/Julia
emit `(smooth_iter, 0)`; DDP emits `Some((1.0, 0))` for the zeroth
basin and `None` otherwise.
JSON migration via `scripts/migrate_phase_3_color_maps.py` rewrites every
fixture under examples/, benches/, and tests/param_files/:
- Mandelbrot/Julia: `{background, color_map}` → `{flat_color, gradients}`
(single-element `gradients`); legacy `histogram_sample_count` dropped.
- DDP: `{foreground, background}` → degenerate single-keyframe gradient
(foreground repeated at queries 0.0 and 1.0) over a `flat_color =
background`.
- Newton: `{cyclic_attractor, color_maps}` → `{flat_color, gradients}`.
Legacy `histogram_sample_count` dropped.
Pixel-hash impact (regression suite gated):
- Mandelbrot (3 fixtures), Julia, Barnsley, Sierpinski: invariant. With
`gradients.len() == 1` the per-gradient histogram reduces to today’s
single histogram, and moving CDF lookup from a normalize-pass to
colorize-time is mathematically a no-op for N=1.
- DDP: regression fixture restored (was disabled — see issue #90 for the
CI flake). Output is visually identical to the legacy white-on-black
render, but the bit-level encoding shifts because the cell type is
now `Option<(f32, u32)>`.
- Newton: two new regression fixtures added (roots-of-unity-4 and
cosh-minus-one). Per-root CDFs are an algorithmic improvement;
eyeball-verified the full-resolution renders for clean per-basin
contrast before locking the hashes.
Roadmap doc updated to mark Phase 3 shipped and document the merged-commit
deviation from the planned 3.1/3.2/3.3 split.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements Phase 3 of the GUI unification roadmap, introducing a unified ChangesCore Rendering & Color Architecture
Fractal Type Updates
CLI & Migration
Configuration & Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
MatthewPeterKelly
left a comment
MatthewPeterKelly
left a comment
Owner
Author
There was a problem hiding this comment.
Looks good overall! I added several suggestions to make the code more readable.
Comment on lines
+31
to
+33
| /// Color used for cells whose `evaluate` returned `None` (Mandelbrot | ||
| /// in-set, DDP out-of-basin, Newton non-converging). | ||
| pub flat_color: [u8; 3], |
Owner
Author
There was a problem hiding this comment.
rename to background and clean up comment.
Suggested change
| /// Color used for cells whose `evaluate` returned `None` (Mandelbrot | |
| /// in-set, DDP out-of-basin, Newton non-converging). | |
| pub flat_color: [u8; 3], | |
| pub background_color: [u8; 3], |
Comment on lines
+34
to
+35
| /// One gradient per "channel". Length must be ≥ 1; rejected at | ||
| /// deserialization otherwise. Each gradient is a `Vec<ColorMapKeyFrame>`. |
Owner
Author
There was a problem hiding this comment.
Improve comment. A "colormap" is represented by a vec of keyframes. So the double vec here is really just a vector of colormaps, which is useful for fractals that contain multiple color maps.
| /// `ColorMap::refresh_cache` from the current keyframes. | ||
| pub luts: Vec<ColorMapLookUpTable>, | ||
| /// `flat_color` pre-converted to `Color32`. | ||
| pub flat: Color32, |
Owner
Author
There was a problem hiding this comment.
Suggested change
| pub flat: Color32, | |
| pub background: Color32, |
Comment on lines
+107
to
+111
| /// Refresh `flat` and the per-gradient `luts` from the current keyframes | ||
| /// and `flat_color`. Allocation-free; does NOT touch `cdfs` (those are | ||
| /// owned by the pipeline and refreshed from histograms after each | ||
| /// compute pass). | ||
| pub fn refresh_cache(&self, cache: &mut ColorMapCache) { |
Owner
Author
There was a problem hiding this comment.
This is a bit of a foot-gun. Is it possible to tweak things a bit so that the lookup table and background color cache are updated immediately wheneveer the CDF and histogram are updated? Basically, restructure the algorithm so that it is not possible to update the CDF without also updating the lookup table and background color.
Comment on lines
+133
to
+136
| let n = cache.luts.len(); | ||
| let g = (gradient_index as usize) % n.max(1); | ||
| let pct = cache.cdfs[g].percentile(value); | ||
| let rgb: Rgb<u8> = cache.luts[g].compute_pixel(pct); |
Owner
Author
There was a problem hiding this comment.
Do not use single letter variables here. For example, you have g for both green and gradient. Just write out the variable name to be readable.
It is fine to leave rgb as that is a standard acronym.
Consider using lookup_table instead of lut as well. Throughout.
| continue; | ||
| } | ||
| let py = (outer_y / n_max_plus_1) as u32; | ||
| let im = pixel_map.height.map(py) + (j as f64) * step * pixel_height; |
Owner
Author
There was a problem hiding this comment.
This line (and the one for re as well, seem overly complicated. They're using the pixel mapper, but then adding a correction factor to it. I think the problem here is that the pixel mapper input is in "simple pixel space", but here we are using a partial map.
I wonder if we can simplify this. This simplest thing I can think of is to do the correction in pixel space on the input. of the mapper. I also wonder if there is a way to restructure which elements in the larger buffer are populated, but I'm not convinced there.
Comment on lines
+64
to
+78
| let i = outer_x % n_max_plus_1; | ||
| if i >= n { | ||
| return; | ||
| } | ||
| let px = (outer_x / n_max_plus_1) as u32; | ||
| let re = pixel_map.width.map(px) + (i as f64) * step * pixel_width; | ||
| for (outer_y, cell) in col.iter_mut().enumerate() { | ||
| let j = outer_y % n_max_plus_1; | ||
| if j >= n { | ||
| continue; | ||
| } | ||
| let py = (outer_y / n_max_plus_1) as u32; | ||
| let im = pixel_map.height.map(py) + (j as f64) * step * pixel_height; | ||
| *cell = kernel.evaluate([re, im]); | ||
| } |
Owner
Author
There was a problem hiding this comment.
This index mapping logic is complicated. Please extract it out into a simple function and then add a unit test. Ensure that this function is highly optimized, easy to understand, and has minimal (or no) abstraction overhead.
|
|
||
| /// Walk every populated cell of `field` and insert each `Some((value, k))` | ||
| /// into `histograms[k % histograms.len()]`. The pipeline calls | ||
| /// `Histogram::reset` on each entry first; this function never resets. |
Owner
Author
There was a problem hiding this comment.
What is the "this function never resets" part?
Owner
Author
There was a problem hiding this comment.
Improve comment.
Comment on lines
+108
to
+118
| /// call only needs `&Histogram`. The `&mut [Histogram]` signature is | ||
| /// here to make the per-render reset / fill semantics explicit. | ||
| pub fn populate_histograms( | ||
| n_max_plus_1: usize, | ||
| sampling_level: i32, | ||
| field: &[Vec<Option<(f32, u32)>>], | ||
| histograms: &mut [Histogram], | ||
| ) { | ||
| let n_hists = histograms.len(); | ||
| assert!(n_hists > 0, "histograms slice must not be empty"); | ||
| let histograms: &[Histogram] = histograms; |
Owner
Author
There was a problem hiding this comment.
This is really confusing. Why should it be mutable? Just have it be mutable where reset is called? But... then we call insert on histogram, so it seems like it does actually need to be mutable? I'm confused. Can we make the code better here somehow?
Comment on lines
+119
to
+155
| if sampling_level >= 0 { | ||
| let n = sampling_level as usize + 1; | ||
| field.par_iter().enumerate().for_each(|(outer_x, col)| { | ||
| let i = outer_x % n_max_plus_1; | ||
| if i >= n { | ||
| return; | ||
| } | ||
| for (outer_y, cell) in col.iter().enumerate() { | ||
| let j = outer_y % n_max_plus_1; | ||
| if j >= n { | ||
| continue; | ||
| } | ||
| if let Some((v, k)) = cell { | ||
| let idx = (*k as usize) % n_hists; | ||
| histograms[idx].insert(*v); | ||
| } | ||
| } | ||
| }); | ||
| } else { | ||
| let block_size = (-sampling_level) as usize + 1; | ||
| let stride = n_max_plus_1 * block_size; | ||
| field.par_iter().enumerate().for_each(|(outer_x, col)| { | ||
| if outer_x % stride != 0 { | ||
| return; | ||
| } | ||
| for (outer_y, cell) in col.iter().enumerate() { | ||
| if outer_y % stride != 0 { | ||
| continue; | ||
| } | ||
| if let Some((v, k)) = cell { | ||
| let idx = (*k as usize) % n_hists; | ||
| histograms[idx].insert(*v); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } |
Owner
Author
There was a problem hiding this comment.
This sampling logic is complicated, and looks somewhat similar to the logic in compute_raw_field(). Maybe you could make some class or function that helps to split out the complicated indexing into a simple and testable interface, making the implementation of compute_raw_field() and populate_histograms() trivial.
…` alias (PR #170 review) Addresses review comment #3240588689 (the outer `Vec<Vec<ColorMapKeyFrame>>` isn't "gradients" — a single color map is itself the inner Vec, so the outer container is "a palette of color maps plus a background"). Type changes: - New `pub type ColorMap = Vec<ColorMapKeyFrame>;` — a single color map. - `ColorMap` struct → `ColorPalette { flat_color, color_maps }`. - `ColorMapCache` → `ColorPaletteCache`. - `Renderable::color_map()` / `color_map_mut()` → `color_palette()` / `color_palette_mut()`. - `colorize_cell` renames its inner `g` (which collided with green) to `index`, `pct` to `percentile`, `n` to `count` (drift toward comment #3285054675 — full single-letter-var sweep lands in commit 4). JSON keys: `gradients` → `color_maps`. The migration script gains a normalization pass that handles both the legacy `{background, color_map}` shape and the post-Phase-3 `{flat_color, gradients}` interim shape, so re-running it is idempotent against either input. `KeyframeColorMap<F>` (the keyframe interpolator) keeps its name — it now wraps a `&ColorMap` (alias) rather than a `&[ColorMapKeyFrame]`, but the two are the same type, so no source changes outside the docstring. Pixel hashes invariant for every fixture (Mandelbrot/Julia/Barnsley/ Sierpinski/DDP/Newton); pure rename. Roadmap doc §5.1–§5.6 updated to use the post-rename names; the historical §6 Phase 1/2/3 descriptions retain their original `ColorMapKind` wording (correct for those periods). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comments #3240576940 and #3240592590 — the reviewer prefers `background_color` for the in-set / out-of-basin / non-converged solid color, and `background` for the pre-converted `Color32` on the cache. The previous "flat" naming didn't match the rest of the UI vocabulary, where "background" is already the canonical term for this slot. - `ColorPalette.flat_color` → `ColorPalette.background_color`. - `ColorPaletteCache.flat` → `ColorPaletteCache.background`. - `colorize_cell` `None` branch updated; `create_cache` / `refresh_cache` updated; tests / fixtures / docs swept. JSON keys: `flat_color` → `background_color`. Migration script gains a normalization step so re-running is idempotent against either spelling. Pixel hashes invariant for every fixture (Mandelbrot / Julia / Barnsley / Sierpinski / DDP / Newton); pure rename. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er_compute_pass (PR #170 review) Addresses review comments #3285048664 ("This is a bit of a foot-gun. Is it possible to tweak things a bit so that the lookup table and background color cache are updated immediately whenever the CDF and histogram are updated?") and #3285082702 ("It seems like it would be more clear to have the histogram stored alongside of the CDF inside of the color map cache"). The cache is now the single source of truth for everything the colorize step reads: - `ColorPaletteCache.histograms: Vec<Histogram>` — moved over from `RenderingPipeline`. Public because the pipeline still fills them between the reset and the refresh. - `ColorPaletteCache::reset_histograms(&mut self)` — replaces the pipeline's open-coded loop. - `ColorPaletteCache::refresh_after_compute_pass(&palette)` — atomically rebuilds CDFs (from `self.histograms`), LUTs (from the palette's keyframes), and the cached background `Color32`. One call updates every downstream-visible piece of state, so the colorize step can never see a half-updated cache. - `ColorPaletteCache::cdfs()` (read-only accessor) — `cdfs`, `luts`, and `background` are now private; mutation flows only through the atomic refresh. The pipeline `render()` collapses from five steps to four: 1. `compute_raw_field` 2. `cache.reset_histograms()` + `populate_histograms(.., &mut cache.histograms)` 3. `cache.refresh_after_compute_pass(palette)` 4. `colorize_collapse_unified` The old `ColorPalette::refresh_cache` method is gone — Phase 7's keyframe-edit fast path will call `refresh_after_compute_pass` against a cache whose histograms haven't been re-filled, which is a no-op for the CDFs (same input) and a real refresh for the LUTs and background. The atomic invariant is the same either way. Pixel hashes invariant for every fixture (Mandelbrot / Julia / Barnsley / Sierpinski / DDP / Newton); same operations in the same order, just behind a tighter API. Roadmap §5.1 / §5.3 updated to describe the new cache shape and the four-phase pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…per (PR #170 review) Addresses the AA-indexing readability cluster (review comments #3285054675, #3285088971, #3285125803, #3285126548, #3285129902, #3285131840, #3285174101, #3285180192, #3285185006, #3285185575, #3290634844): - New `SamplePlanner` enum encodes the AA / block-fill decomposition rule in one place: given an outer field index it returns `Some((pixel_index, subpixel_index))` for populated positions or `None` for the traversal to skip. Five new unit tests cover the decomposition directly so it can be tweaked confidently. - New `par_for_each_populated_cell_mut` / `par_for_each_populated_cell` traversal helpers thread the planner through rayon parallel-by-column loops. `compute_raw_field` and `populate_histograms` now collapse to closures over those helpers — no more duplicated `if sampling_level >= 0` / else skeleton, no more `outer_x % n_max_plus_1` arithmetic inline. - Per review comment #3285174101: the AA sub-pixel coordinate math no longer mixes `width/(W-1)` (from `PixelMapper::map`) with `width/W` (from a hand-computed `pixel_width / n`). It now uses `PixelMapper::new(&spec.upsample(subpixel_count)).width.map(px*n + i)` end-to-end, which is what `PixelMapper` was designed for. Sub-pixel spacing is consistent: every sub-pixel slot is exactly `width/(W·n − 1)` apart. At `sampling_level == 0` and at block-fill levels, `subpixel_count = 1` so `spec.upsample(1) == spec` and the mapper degenerates to the base map — pixel hashes invariant for those fixtures. - Variable / API renames (review comments #3285054675, #3285088971, #3285125803, #3285126548): single-letter `i`/`j`/`n`/`px`/`py`/`g` expanded to `subpixel_index_{x,y}` / `subpixel_count` / `pixel_index_{x,y}` / `color_map_index`. `cache.luts` → `cache.lookup_tables`. "AA" → "anti-aliasing" in docs. - Comment / docstring fixes (comments #3285129902, #3285131840, #3285185006, #3285185575): the inline early-abort and stride logic is now inside `SamplePlanner::decompose` with its own explanatory comments; `populate_histograms` doc explains the `&mut [Histogram]` signature in terms of per-render ergonomics rather than the cryptic "this function never resets". Pixel-hash impact: - Mandelbrot baseline (sampling_level=0), Mandelbrot downsample (sampling_level=-3), Julia baseline (sampling_level=0), Barnsley, Sierpinski: invariant. The new formula reduces to the old one at `subpixel_count = 1`. - Mandelbrot AA (sampling_level=3), DDP (sampling_level=2), both Newton fixtures (sampling_level=1): hashes regenerated. ~1-pixel boundary shifts due to the consistent sub-pixel spacing. Full-resolution Newton renders eyeball-verified to show identical basin structure; DDP shows the same swirl pattern (white-on-black gradient is constant-color so only boundary classification shifts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-> make_red_to_blue_color_map (PR #170 review) Closes review comments #3285075945 (rename `outer`/`inner` locals in `RenderingPipeline::new` and explain the dimension math) and #3285057976 (rename the `red_to_blue` test helper to `make_red_to_blue_color_map`). - `RenderingPipeline::new` locals: `outer` → `outer_dim_x`, `inner` → `inner_dim_y`. Added a doc comment above explaining that the field is `(n_max_plus_1·W) × (n_max_plus_1·H)` cells and runtime sampling levels populate a sub-rectangle. - `red_to_blue` → `make_red_to_blue_color_map` in the `color_map.rs` tests. Field-iteration tests already used `red_to_blue_palette` per earlier commit. Pixel hashes invariant — pure renames. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Code finally LGTM! |
MatthewPeterKelly
deleted the
decouple-scalar-field-and-color-mapping-common-aa
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Replaces #169 - same goal, but with a different approach to the interfaces that significantly reduces complexity and duplication in the fractal code.
The full details are in the
docs/gui-unification-roadmap.md(in context), along with details for this PR indocs/phase-3-detailed-plan.md.