Skip to content

fix(pptx_to_svg): decode <a:hslClr> hue with the correct unit scale#102

Merged
hugohe3 merged 1 commit into
hugohe3:mainfrom
voidborne-d:fix/color-resolver-hslClr-hue-units
May 14, 2026
Merged

fix(pptx_to_svg): decode <a:hslClr> hue with the correct unit scale#102
hugohe3 merged 1 commit into
hugohe3:mainfrom
voidborne-d:fix/color-resolver-hslClr-hue-units

Conversation

@voidborne-d
Copy link
Copy Markdown
Contributor

Problem

resolve_color() in skills/ppt-master/scripts/pptx_to_svg/color_resolver.py:217-221 decodes a DrawingML <a:hslClr> element by dividing the hue attribute by 60000.0. The DrawingML attribute is in 1/60000-degree units, so the result is hue in degrees (0..360).

The downstream helper _hsl_to_hex(h, s, l) (same file, lines ~340-345) however expects hue as a fraction in [0, 1) — it does h = h % 1.0 before handing off to colorsys.hls_to_rgb. So any non-zero hue collapses to 0.0 and every <a:hslClr> color decodes as pure red regardless of its actual hue.

Repro (on main @ 2026-05-14, no fix applied)

from xml.etree import ElementTree as ET
import sys; sys.path.insert(0, "skills/ppt-master/scripts")
from pptx_to_svg.color_resolver import resolve_color

ns = 'xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main"'
for hue, label, want in [
    (0,         "red    0°",  "#FF0000"),
    (3600000,   "yellow 60°", "#FFFF00"),
    (7200000,   "green 120°", "#00FF00"),
    (10800000,  "cyan 180°",  "#00FFFF"),
    (14400000,  "blue 240°",  "#0000FF"),
    (18000000,  "magenta 300°", "#FF00FF"),
]:
    elem = ET.fromstring(f'<a:hslClr {ns} hue=\"{hue}\" sat=\"100000\" lum=\"50000\"/>')
    got, _ = resolve_color(elem, None)
    print(f\"{label:14s} got={got}  want={want}\")

Before the fix:

red    0°      got=#FF0000  want=#FF0000
yellow 60°     got=#FF0000  want=#FFFF00   ← wrong
green 120°     got=#FF0000  want=#00FF00   ← wrong
cyan 180°      got=#FF0000  want=#00FFFF   ← wrong
blue 240°      got=#FF0000  want=#0000FF   ← wrong
magenta 300°   got=#FF0000  want=#FF00FF   ← wrong

Fix

Divide by 21_600_000 (= 60000 × 360) so the hue is already normalized to a [0, 1) fraction before reaching _hsl_to_hex. Comment updated alongside.

     elif tag == "hslClr":
-        h = float(color_elem.attrib.get(\"hue\", \"0\")) / 60000.0  # 1/60000 deg
+        # DrawingML hue is in 1/60000 deg ([0, 21_600_000) maps to [0°, 360°));
+        # _hsl_to_hex expects a fraction in [0, 1), so divide by 60000 * 360.
+        h = float(color_elem.attrib.get(\"hue\", \"0\")) / 21_600_000.0

After the fix the same repro snippet returns the expected hex values for the full 0–360° wheel.

Local verification

Beyond the 6 primary/secondary hues above, I also walked the boundary cases:

Case Input Result
360° wraps hue=21600000 sat=100000 lum=50000 #FF0000
720° (2 rotations) hue=43200000 sat=100000 lum=50000 #FF0000
sat=0 (grey) hue=14400000 sat=0 lum=50000 #808080
lum=0 (black) hue=7200000 sat=100000 lum=0 #000000
lum=100% (white) hue=7200000 sat=100000 lum=100000 #FFFFFF

I also reran the other 5 colour branches (srgbClr, schemeClr, sysClr, prstClr, scrgbClr) plus an srgbClr + <a:alpha> modifier — none of them touch _hsl_to_hex and all still resolve to the same values as before.

Notes

  • The _hsl_to_hex helper itself is left untouched — its h = h % 1.0 if h != 1.0 else 1.0 contract makes sense for a fraction-input helper, and changing it would silently affect any future caller that already passes a fraction.
  • Per CONTRIBUTING.md (Not a fit: Adding CI, test frameworks ...), no test file is included. The verification snippet above is fully copy-pasteable for hand-running if you'd like.
  • <a:hslClr> is rare in PowerPoint-authored decks (most use srgbClr / schemeClr), but it does appear in some third-party DrawingML output and in certain theme variants — and when it does appear today, every such fill silently turns pure red.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

…) scale

`resolve_color` for the `hslClr` branch divided the `hue` attribute by
60000.0, leaving the value in plain degrees (e.g. 240 for blue). The
downstream `_hsl_to_hex` helper, however, expects a fraction in [0, 1)
and only does `h = h % 1.0`, so a 240° input collapses to `0.0` and
every `<a:hslClr>` color resolves as pure red regardless of the actual
hue.

Divide by `21_600_000` (= 60000 × 360) instead so the result is already
normalized to [0, 1) before reaching `_hsl_to_hex`, matching the unit
that the helper documents.

Repro (before the fix):

    >>> from xml.etree import ElementTree as ET
    >>> from pptx_to_svg.color_resolver import resolve_color
    >>> ns = 'xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main"'
    >>> for hue, label in [(0,'red'),(7200000,'green 120°'),
    ...                    (14400000,'blue 240°'),(10800000,'cyan 180°')]:
    ...     elem = ET.fromstring(f'<a:hslClr {ns} hue="{hue}" sat="100000" lum="50000"/>')
    ...     print(label, resolve_color(elem, None)[0])
    red          #FF0000
    green 120°   #FF0000   ← expected #00FF00
    blue 240°    #FF0000   ← expected #0000FF
    cyan 180°    #FF0000   ← expected #00FFFF

After the fix all four cases round-trip the full 0–360° colour wheel
(verified locally for red / yellow / green / cyan / blue / magenta /
360° wrap / 720° wrap, plus the sat=0 grey and lum=0/100% black/white
edges).

Other branches (`srgbClr`, `schemeClr`, `sysClr`, `prstClr`,
`scrgbClr`) are untouched, and the `_hsl_to_hex` helper is unchanged so
no other callers are affected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Repository owner deleted a comment from chatgpt-codex-connector Bot May 14, 2026
@hugohe3
Copy link
Copy Markdown
Owner

hugohe3 commented May 14, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@hugohe3 hugohe3 merged commit 4fd3b39 into hugohe3:main May 14, 2026
@hugohe3
Copy link
Copy Markdown
Owner

hugohe3 commented May 14, 2026

Thanks @voidborne-d for the surgical fix! 🙏

The diagnosis is spot-on — the <a:hslClr> hue unit (1/60000 deg) and the [0, 1) fraction that _hsl_to_hex expects differ by a factor of 360, so any non-zero hue collapsed to 0 after the % 1.0 and the whole HSL branch effectively hard-coded red. One-line fix, with the unit-conversion rationale captured in the comment; the sibling hueOff branch in the same file (/60000 → /360) is exactly the same conversion in its decomposed form, which is a nice cross-check that the 21_600_000 constant is right.

I re-ran your repro locally — the 6 primary/secondary hues plus the 360°/720° wrap and the sat=0 / lum=0 / lum=100% edge cases all match expectations. Squash-merged into main. This kind of bug on the reverse-parsing path is genuinely hard to notice in day-to-day use — thanks for catching it and for the unit-comment cleanup along the way 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants