search

openfl / svg

Provides SVG parsing and rendering for OpenFL and Haxe
MIT License
68 stars 30 forks source link

Code-review fixes for rotation (#41 and #42) #46

Closed ashes999 closed 8 years ago

ashes999 commented 8 years ago

(x, y) coordinates are optional in rotate Verified translation for Matrix is negated correctly

Please review this @ibilon . The translation looks negated, I think that's because the Haxe matrix class inverts it. It is actually the correct formula (negating it gives the wrong rotation).

ashes999 commented 8 years ago

Just a reminder that I'm still waiting for this @ibilon

ibilon commented 8 years ago

Oh sorry, was on holiday when you posted and I missed it, will review soon.

ibilon commented 8 years ago

I'd change the regex to ~/rotate\(([0-9\.]+)(\s+([0-9\.]+)\s+([0-9\.]+))?\)/ according to the doc there's no comma, you can't just pass "angle x", it's either "angle" or "angle x y", also you need at least a space between the numbers.

ashes999 commented 8 years ago

Here's my dilemma. TLDR: compatibility or strict adherence to specifications?

You're right that the SVG docs don't specify a comma here. However, I generated my test SVGs with svg-edit, which used commas in the generated SVG.

I would prefer an SVG library that "just works" with SVGs from anywhere, even if they're not (strictly) adherent to specifications. This will close the door on a whole class of bugs people may open in the future about "well I used X program for my SVG and it doesn't show up properly."

On the other hand, maybe looser validation will bite us. We can't possibly support every (broken?) SVG tool out there. So we should stick to the specs.

What are your opinions on this @ibilon and @jgranick ? This is a decision for the library at a whole.

jgranick commented 8 years ago

I think the motivation is to load our SVG files, not to get tripped up on specifics :wink:

ashes999 commented 8 years ago

So can I merge?

ibilon commented 8 years ago

Yeah I guess since the input is user provided svg it makes sense to be lenient about standards.

@ashes999 still need a couple of changes to the regex, the first \s* should be \s+, maybe add a \s* before the comma, and change the ,? with a [, ]. So: ~/rotate\(([0-9\.]+)(\s+([0-9\.]+)\s*[, ]\s*([0-9\.]+))?\)/

ashes999 commented 8 years ago

This should fix all the remaining issues. The tests also pass now.

ibilon commented 8 years ago

:+1: