Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVGLoader Example: Use srgb output in the svg example #23280

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 19, 2022

Related issue: #23272

Description

Converts the SVGLoader example page to use sRGB output rather than Linear. I'll note that given that MeshBasicMaterials and therefore no lighting is being used it could be considered more optimal to leave the demo as-is but for the sake of encouraging users to generally use the correct workflow this seems like a good change?

https://raw.githack.com/gkjohnson/three.js/linear-svg-colors/examples/webgl_loader_svg.html

I was taking a look at ColladaLoader and OBJLoader, as well. It doesn't look like there's very explicit documentation on color spaces for these formats and they're more specification by convention. Given that our demos don't adjust the default output color space for the renderer it seems like the assumption is that the textures and colors are sRGB. Is there any evidence or example models that show this isn't always the right thing to do?

cc @donmccurdy @WestLangley @Mugen87

@gkjohnson gkjohnson requested a review from yomboprime January 19, 2022 22:18
@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

As you proposed in #23272, I think it makes more sense to add the color conversion in the loader directly (inside SVGLoader instead of in the example).

@mrdoob mrdoob added this to the r137 milestone Jan 20, 2022
@yomboprime
Copy link
Collaborator

Is there any evidence or example models that show this isn't always the right thing to do?

It seems it is indeed sRGB:

The specification we've been following is SVG 2:
https://www.w3.org/TR/SVG2/painting.html#ColorProperty

Which points to the CSS3 specification of color:
https://www.w3.org/TR/css-color-3/#numerical

"All RGB colors are specified in the sRGB color space"

So I agree it must be done in the loader. My only reason for not doing that is SVG can be used for not rendering purposes. For example it is not uncommon to code the strength of a laser cut path into the color, when using the SVG in a CNC laser cutting machine.

@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

For example it is not uncommon to code the strength of a laser cut path into the color, when using the SVG in a CNC laser cutting machine.

What could go wrong... 😁

@yomboprime
Copy link
Collaborator

What could go wrong...

I guess it can be re-converted to sRGB in that case.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Jan 20, 2022

Is it possible to do this generally? It looks like the fill and stroke properties are on a generic list of styles parsed from CSS:

const fillColor = path.userData.style.fill;

// ...

const strokeColor = path.userData.style.stroke;

Certainly we can convert fill and stroke internally from sRGB to Linear but if there are other CSS styles that are colors provided then they won't be converted and the result will be inconsistent. Part of me also feels like if the shape of the parse result indicates that it contains a list of CSS styles then it should return the provided raw value from the file / CSS colors which are in sRGB.

@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

Part of me also feels like if the shape of the parse result indicates that it contains a list of CSS styles then it should return the provided raw value from the file / CSS colors which are in sRGB.

Yeah, I can see that. I guess the SVGLoader it's a bit of an exception yes...

Okay, I'll merge as it is.

@mrdoob mrdoob merged commit 0321aa7 into mrdoob:dev Jan 20, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

Thanks!

@gkjohnson gkjohnson deleted the linear-svg-colors branch January 21, 2022 18:47
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.

3 participants