stylesheet jsName vs svgName fix

[jhlggit]

Related issue: #XXXX

Description

In the SVG Loader, there's some code to read styles from embedded style sheets, which are held in a collection called stylesheetStyles. This collection was being indexed into using svgName, but the styles in an embedded style sheet are actually identified by the slightly different jsName so, where these two differed, the styles were not being picked up.

The fix is a one-line change, in which stylesheetStyles[ svgName ] is replaced with stylesheetStyles[ jsName ] (two occurrences).

For example, here is a style node from an SVG file:

<style>.highlightBox { fill: #ffa500; fill-opacity: .2; } .highlightBoxReferent { fill: #9ff3db; fill-opacity: .3; }</style>

Elsewhere in the file, there are elements with this attribute
class="highlightBox"

With the old code, these elements were rendered as fully opaque but, with this fix, they are now rendered with the specified opacity of .2.

The likely reason this wasn't noticed before is that not all styles are affected. In the example above, the fill colour was not affected because, for fill colour, the svgName and the jsName are the same (namely, "fill").

SVG file referenced in this example:
LogicEssence1_TimesNewRomanPSMT_TimesNewRomanPSMT.xml

screenshot with old code:

screenshot with fixed code:

[Mugen87]

Not sure this bit is correct. The test SVG style-css-inside-defs.svg in the webgl_loader_svg example now renders incorrectly with this change. The red border becomes too wide.

You can test by comparing Prod with this PR:

https://threejs.org/examples/#webgl_loader_svg
https://rawcdn.githack.com/jhlggit/three.js/StyleSheetNameFixBranch/examples/index.html#webgl_loader_svg

If you open the SVG directly in Chrome or via mac Preview, it renders like in the Prod version.

[jhlggit]

Thank you for the feedback. You're right - the change that fixes my issue does seem to cause a regression with stroke-width - sorry I didn't pick that up. It may take me some time to find a better place to make the change. In the meantime, please excuse my hesitation; this is my first foray into contributing to an open-source project: should I close this pull-request and open a new one when I have a better fix, or is there a way I can make a modification within this pull request - and, in any case, should I also formally log an issue to describe the original problem with fill-opacity?

[Mugen87]

please excuse my hesitation;

Don't worry about it. Let's keep this PR open and update it with your new solution. As long as the PR is open, we need no separate issue. If we don't manage to fix the issue now, we can close the PR and open an issue instead for tracking purposes.

[jhlggit]

OK; I've prepared a new solution. However, it does include a change to the test data file, models/svg/style-css-inside-defs.svg . Specifically, I've added a second star, which overlaps the first, and a fractional fill-opacity, so that you can see easily (from the overlap) whether the fractional opacity is being applied. I suspect that, if I include that change in the pull request, it will automatically fail testing, because, if I understand the doco, some of the testing includes pixel-to-pixel comparisons of the output. So, perhaps I ought not to upload the pull request until I've also updated the test case. Could you advise me on how to do that?

[Mugen87]

You must regenerate the E2E screenshot if you change the visual appearance of an example. However, since the mentioned test file is not loaded when opening webgl_loader_svg, everything should be fine. So you can just update the PR with your changes and the CI shouldn't be affected.

[jhlggit]

Having done some more research, I have discovered some more things about that border width:

First, if I revert to the production code, and then make a series of changes to the stroke-width attribute in the style element in the test data file style-css-inside-defs.svg, then the shape presents differently in the Chrome SVG viewer, but remains unchanged in three.js, which confirms my original observation that any style attribute whose svgName is different from its jsName is being ignored, and indicates that it's just fortuitous that, with stroke-width:3, the behaviour of three.js happens to match the behaviour of the chrome SVG viewer. Data file versions and screenshots can be found in the attached comparison.zip

Second, there's an existing issue in three.js, namely, #30268, in which "When a path is scaled down with a transform the SVGLoader doesnt also scale the styles such as stroke-width". So, what I suggest is, the behaviour observable with my pull request as it stands is just the existing production issue as described in #30268, and not a new issue introduced by my change. Or, to put it another way, the issue that I'm trying to fix was masking an instance of that other issue.

The existing production behaviour viz-a-viz stroke width could be maintained simply by commenting out line 1108 of SVGLoader.js; in the production code, this call does nothing anyway (as demonstrated above). However, I would recommend leaving it in, just so that it will get the benefit of a fix to #30268, if and when that arrives.

(I have not altered the pull request with the addition of an extra star to the test data, as I was considering above, because I would rather keep it as simple as possible for now).

comparisons.zip