Skip to content

Improve SVG color map handling. - #119729

Open
bruvzg wants to merge 1 commit into
godotengine:masterfrom
bruvzg:svg_color_map
Open

bruvzg wants to merge 1 commit into
godotengine:masterfrom
bruvzg:svg_color_map

Conversation

@bruvzg

@bruvzg bruvzg commented May 25, 2026

Copy link
Copy Markdown
Member

Fixes godotengine/godot-proposals#14910

Adds handing for name:xxx; values in addition to name="xxx" as well as rgb()/rgba()/hsl()/hsla() CSS colors.

@bruvzg bruvzg added this to the 4.8 milestone May 25, 2026
@bruvzg
bruvzg requested a review from a team as a code owner May 25, 2026 06:45
@MewPurPur

MewPurPur commented May 25, 2026 •

Copy link
Copy Markdown
Contributor

Sounds good. I saw that the code expanded the list of attributes. While at it, we can also include the color and lightning-color attributes (e.g., https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Attribute/color) to complete the list.

Idk if ThorVG supports lightning-color, but it definitely supports color.

@bruvzg

bruvzg commented May 25, 2026

Copy link
Copy Markdown
Member Author

While at it, we can also include the color and lightning-color attributes

Updated. It's prefix, so simply using color should cover all *color cases.

@fire
fire requested a review from a team May 25, 2026 15:47
@MewPurPur

MewPurPur commented May 25, 2026 •

Copy link
Copy Markdown
Contributor

Hmm... I don't know about that, suffix matching seems dangerous and like it could risk mutating unrelated metadata (e.g. <metadata inkscape-canvas-color="#decade"/> or something). My intuition says you should only match these 6 exact attribute names.

@bruvzg

bruvzg commented May 25, 2026

Copy link
Copy Markdown
Member Author

e.g. or something

Does it matter, it's only applied before the rendering, and metadata like this should be ignored.

@MewPurPur

Copy link
Copy Markdown
Contributor

Oh, you're right, my bad, since this is only for rendering, I can't think of situations where it'd matter.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants