Optimize gradient rendering with precomputed lookups#21
Open
Optimize gradient rendering with precomputed lookups#21
Conversation
Contributor
Reviewer's GuideThis PR optimizes gradient rendering by precomputing linear-light color stops and building bounded per-span style lookup tables used for fast style retrieval in the common single- and double-width cluster paths, while preserving the existing interpolation logic as a fallback for edge cases. Sequence diagram for gradient rendering with precomputed style lookupsequenceDiagram
participant Console
participant Gradient
participant RenderLoop
Console->>Gradient: __rich_console__(console, options)
activate Gradient
Gradient->>Console: render_lines(content, options, pad, new_lines)
Console-->>Gradient: lines
Gradient->>Gradient: _build_style_lookup(width)
Gradient-->>Gradient: foreground_lut, background_lut
loop per line
Gradient->>RenderLoop: iterate segments
loop per cluster
RenderLoop->>Gradient: _get_style_from_lookup(position, cluster_width, width, foreground_lut, background_lut)
alt width is 1 or 2 and index in range and LUT available
Gradient-->>RenderLoop: precomputed Style
else fallback path
Gradient->>Gradient: _get_style_at_position(position, cluster_width, width)
Gradient-->>RenderLoop: computed Style
end
RenderLoop->>Gradient: _merge_styles(base_style, style)
RenderLoop->>Gradient: _apply_highlight_style(merged_style, cluster)
end
end
Gradient-->>Console: rendered segments
deactivate Gradient
Class diagram for updated Gradient gradient lookup and interpolationclassDiagram
class Gradient {
<<class>>
_GAMMA_CORRECTION : float
_MAX_PRECOMPUTED_WIDTH : int
_active_stops
_linear_foreground : list~tuple~float,float,float~~
_linear_background : list~tuple~float,float,float~~
_highlight_rules : list~_HighlightRule~
__init__(self, text, colors, bg_colors, style, justify, end, highlight_words, vertical)
colors(self) List~ColorTriplet~
colors(self, colors List~ColorType~) void
bg_colors(self) List~ColorTriplet~
bg_colors(self, colors Optional~List~ColorType~~) void
__rich_console__(self, console, options)
_get_style_at_position(self, position int, width int, span int) Style
_get_style_from_lookup(self, position int, width int, span int, foreground_lut list~Style~|None, background_lut list~Style~|None) Style
_compute_fraction(self, position int, width int, span float) float
_refresh_linear_stops(self) void
_to_linear_stops(self, stops list~ColorTriplet~) list~tuple~float,float,float~~
_build_style_lookup(self, span int) tuple~list~Style~|None, list~Style~|None~
_build_color_lut(self, span int, stops list~ColorTriplet~, linear_stops list~tuple~float,float,float~, is_background bool) list~Style~
_interpolate_linear_color(self, frac float, stops list~ColorTriplet~, linear_stops list~tuple~float,float,float~) tuple~float,float,float~~
_merge_styles(original Style, gradient_style Style) Style
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
_to_linear_stops/_interpolate_linear_colorpaths duplicate and slightly diverge from the existing_to_linear/to_srgbbehavior (different gamma modeling and no reuse of existing helpers), which risks subtle visual differences; consider refactoring to share the same conversion utilities so behavior stays identical while still benefiting from precomputation. - In
_get_style_from_lookup, the choice ofindex = position + (width // 2)for 2-cell clusters changes how the gradient is sampled compared to_get_style_at_position(which uses width in_compute_fraction); it may be worth confirming this matches the previous visual behavior or adjusting the LUT construction to explicitly handle double-width alignment. - When only one of
foreground_lut/background_lutis present,_get_style_from_lookupallocates a newStyle()on every call; you could avoid this per-cluster allocation by using a cached empty style or by merging directly into the non-empty style.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_to_linear_stops` / `_interpolate_linear_color` paths duplicate and slightly diverge from the existing `_to_linear`/`to_srgb` behavior (different gamma modeling and no reuse of existing helpers), which risks subtle visual differences; consider refactoring to share the same conversion utilities so behavior stays identical while still benefiting from precomputation.
- In `_get_style_from_lookup`, the choice of `index = position + (width // 2)` for 2-cell clusters changes how the gradient is sampled compared to `_get_style_at_position` (which uses width in `_compute_fraction`); it may be worth confirming this matches the previous visual behavior or adjusting the LUT construction to explicitly handle double-width alignment.
- When only one of `foreground_lut`/`background_lut` is present, `_get_style_from_lookup` allocates a new `Style()` on every call; you could avoid this per-cluster allocation by using a cached empty style or by merging directly into the non-empty style.
## Individual Comments
### Comment 1
<location> `src/rich_gradient/gradient.py:503-505` </location>
<code_context>
+ Returns:
+ Style with appropriate foreground and/or background colors.
+ """
+ if span <= 0 or position < 0:
+ return self._get_style_at_position(position, width, span)
+ if width in {1, 2}:
+ if background_lut is None and foreground_lut is None:
+ return self._get_style_at_position(position, width, span)
</code_context>
<issue_to_address>
**issue (bug_risk):** Precomputed lookup changes how width is used in the gradient calculation for 2-cell clusters.
The LUT path samples a single precomputed column using `index = position + (width // 2)` and a LUT built with `width=1` in `_compute_fraction`, while `_get_style_at_position(position, width, span)` incorporates `width` into the gradient fraction. For `width == 2`, this changes behavior from approximating over the cluster width to sampling only at the center, which can subtly change the gradient, especially with phase/justify logic or near edges. If you need to preserve the old behavior for `width == 2`, consider either continuing to use `_get_style_at_position` in that case or updating the LUT generation to account for `width` in the fraction computation.
</issue_to_address>
### Comment 2
<location> `src/rich_gradient/gradient.py:482` </location>
<code_context>
return Style(color=fg_style or None, bgcolor=bg_style or None)
+ def _get_style_from_lookup(
+ self,
+ position: int,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the LUT optimization by unifying the style-selection helpers and eliminating unnecessary cached linear-stop state.
The new LUT path adds a second style pipeline and extra mutable state that you don’t really need. You can keep the optimization while simplifying in two main places:
---
### 1. Unify `_get_style_from_lookup` and `_get_style_at_position`
Right now, you have two parallel helpers and the LUT one has width-specific branching and falls back in several places. You can collapse this into a single helper that always computes the style and *internally* uses LUTs when possible:
```python
def _style_at(
self,
position: int,
width: int,
span: int,
fg_lut: list[Style] | None,
bg_lut: list[Style] | None,
) -> Style:
# preserve all existing guards
if span <= 0 or position < 0 or (fg_lut is None and bg_lut is None):
return self._get_style_at_position(position, width, span)
# width‑agnostic cluster indexing; clamp into span
index = position + width // 2
if index < 0 or index >= span:
return self._get_style_at_position(position, width, span)
fg = fg_lut[index] if fg_lut is not None else Style()
bg = bg_lut[index] if bg_lut is not None else Style()
return fg + bg
```
And update the rendering loop to only use this helper:
```python
style = self._style_at(
column - cluster_width,
cluster_width,
width,
foreground_lut,
background_lut,
)
```
You can then drop `_get_style_from_lookup` entirely and keep `_get_style_at_position` as the single “slow path” implementation. This removes the width `{1, 2}` special case and the “which helper do I call?” mental branch, while preserving behavior where LUT cannot or should not be used.
---
### 2. Remove `_linear_foreground` / `_linear_background` state and simplify LUT building
The extra `_linear_*` attributes plus `_refresh_linear_stops` increase mutable state and sync points. Since LUT building is already O(span), you can derive linear stops on demand inside the LUT builder instead of maintaining them on the object.
You can replace `_refresh_linear_stops` and `_linear_*` with a small helper:
```python
def _to_linear_stops(
self, stops: list[ColorTriplet]
) -> list[tuple[float, float, float]]:
if not stops:
return []
def to_linear(c: float) -> float:
return (c / 255.0) ** self._GAMMA_CORRECTION
return [(to_linear(r), to_linear(g), to_linear(b)) for r, g, b in stops]
```
Then simplify your LUT API:
```python
def _build_fg_lut(self, span: int) -> list[Style] | None:
if not self.colors:
return None
linear = self._to_linear_stops(self.colors)
return self._build_color_lut(span, self.colors, linear, is_background=False)
def _build_bg_lut(self, span: int) -> list[Style] | None:
if not self.bg_colors:
return None
linear = self._to_linear_stops(self.bg_colors)
return self._build_color_lut(span, self.bg_colors, linear, is_background=True)
def _build_style_lookup(
self, span: int
) -> tuple[list[Style] | None, list[Style] | None]:
if span <= 0 or span > self._MAX_PRECOMPUTED_WIDTH:
return None, None
return self._build_fg_lut(span), self._build_bg_lut(span)
```
With this, you can remove:
- `self._linear_foreground`
- `self._linear_background`
- `_refresh_linear_stops`
- calls to `_refresh_linear_stops()` in `__init__`, `colors` setter, and `bg_colors` setter
This keeps all LUT benefits but reduces class-level state and makes LUT construction easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+503
to
+505
| if span <= 0 or position < 0: | ||
| return self._get_style_at_position(position, width, span) | ||
| if width in {1, 2}: |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Precomputed lookup changes how width is used in the gradient calculation for 2-cell clusters.
The LUT path samples a single precomputed column using index = position + (width // 2) and a LUT built with width=1 in _compute_fraction, while _get_style_at_position(position, width, span) incorporates width into the gradient fraction. For width == 2, this changes behavior from approximating over the cluster width to sampling only at the center, which can subtly change the gradient, especially with phase/justify logic or near edges. If you need to preserve the old behavior for width == 2, consider either continuing to use _get_style_at_position in that case or updating the LUT generation to account for width in the fraction computation.
|
|
||
| return Style(color=fg_style or None, bgcolor=bg_style or None) | ||
|
|
||
| def _get_style_from_lookup( |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the LUT optimization by unifying the style-selection helpers and eliminating unnecessary cached linear-stop state.
The new LUT path adds a second style pipeline and extra mutable state that you don’t really need. You can keep the optimization while simplifying in two main places:
1. Unify _get_style_from_lookup and _get_style_at_position
Right now, you have two parallel helpers and the LUT one has width-specific branching and falls back in several places. You can collapse this into a single helper that always computes the style and internally uses LUTs when possible:
def _style_at(
self,
position: int,
width: int,
span: int,
fg_lut: list[Style] | None,
bg_lut: list[Style] | None,
) -> Style:
# preserve all existing guards
if span <= 0 or position < 0 or (fg_lut is None and bg_lut is None):
return self._get_style_at_position(position, width, span)
# width‑agnostic cluster indexing; clamp into span
index = position + width // 2
if index < 0 or index >= span:
return self._get_style_at_position(position, width, span)
fg = fg_lut[index] if fg_lut is not None else Style()
bg = bg_lut[index] if bg_lut is not None else Style()
return fg + bgAnd update the rendering loop to only use this helper:
style = self._style_at(
column - cluster_width,
cluster_width,
width,
foreground_lut,
background_lut,
)You can then drop _get_style_from_lookup entirely and keep _get_style_at_position as the single “slow path” implementation. This removes the width {1, 2} special case and the “which helper do I call?” mental branch, while preserving behavior where LUT cannot or should not be used.
2. Remove _linear_foreground / _linear_background state and simplify LUT building
The extra _linear_* attributes plus _refresh_linear_stops increase mutable state and sync points. Since LUT building is already O(span), you can derive linear stops on demand inside the LUT builder instead of maintaining them on the object.
You can replace _refresh_linear_stops and _linear_* with a small helper:
def _to_linear_stops(
self, stops: list[ColorTriplet]
) -> list[tuple[float, float, float]]:
if not stops:
return []
def to_linear(c: float) -> float:
return (c / 255.0) ** self._GAMMA_CORRECTION
return [(to_linear(r), to_linear(g), to_linear(b)) for r, g, b in stops]Then simplify your LUT API:
def _build_fg_lut(self, span: int) -> list[Style] | None:
if not self.colors:
return None
linear = self._to_linear_stops(self.colors)
return self._build_color_lut(span, self.colors, linear, is_background=False)
def _build_bg_lut(self, span: int) -> list[Style] | None:
if not self.bg_colors:
return None
linear = self._to_linear_stops(self.bg_colors)
return self._build_color_lut(span, self.bg_colors, linear, is_background=True)
def _build_style_lookup(
self, span: int
) -> tuple[list[Style] | None, list[Style] | None]:
if span <= 0 or span > self._MAX_PRECOMPUTED_WIDTH:
return None, None
return self._build_fg_lut(span), self._build_bg_lut(span)With this, you can remove:
self._linear_foregroundself._linear_background_refresh_linear_stops- calls to
_refresh_linear_stops()in__init__,colorssetter, andbg_colorssetter
This keeps all LUT benefits but reduces class-level state and makes LUT construction easier to follow.
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.
Motivation
Description
colors/bg_colorsare set via_refresh_linear_stopsand_to_linear_stops._build_style_lookupwhich returns foreground/background style LUTs limited by_MAX_PRECOMPUTED_WIDTHto bound memory use._build_color_lutand interpolate using precomputed linear stops through_interpolate_linear_colorto avoid repeated conversion cost.__rich_console__) via_get_style_from_lookupfor width 1 and 2 clusters, while falling back to_get_style_at_positionfor other sizes or out-of-range spans.colors/bg_colorsremain consistent with lookups.Testing
ruff check src tests, which failed to run because the environment lacked the expectedruffbinary / pyenv pointed to a missing Python version and reportedruff: command not found.python3 -m pytest, which did not run becausepytestis not installed in the execution environment (No module named pytest).Codex Task
Summary by Sourcery
Optimize gradient style computation by precomputing linear-light color data and per-column style lookups for common cluster widths while preserving existing behavior via fallbacks.
Enhancements: