Track transformations and apply to gradients' matrices #894
Conversation
PDF gradients/patterns take coordinates in the coordinate space of the document, not the "user space", so if you perform a scale/rotate/translate and then paint a gradient inside, it doesn't render correctly. This commit tracks transformations applied to the document, and multiplies the gradient matrix with this tracked transformation matrix so that the gradient appears in the correct place in the document.
|
Rubocop wants .map(&:to_f) instead of .map { |n| n.to_f }, but I was just following the style of the surrounding code. Happy to change. |
|
@mogest Please do. Code seems clean enough but I haven't got to actually try it out yet. Hopefully will have some time this week. |
|
My main concern with this, beyond trying to find the time to truly understand this change, is any backwards compatibility implications. From my basic understanding, it looks like it a user could be relying on the broken behavior of their gradient's not being rotated, and this change would alter their behavior if they upgraded. Am I understanding correctly? |
If that's a major concern, why not just add an option at the top-level to use legacy or compliant gradient logic? One thing is for certain. We have to get past this or else gradients in SVGs embedded in PDFs (which is a major use case for a PDF generator) is never going to work correctly. |
|
I was thinking something similar. For that to work we would have to let users opt into the new correct behavior, deprecate the old broken behavior, then when we release prawn 3 at some point in the future we can remove the old code and the logic for handling it's optional use and just use the correct new behavior. I don't believe Prawn has done something like that before so I want to make sure that a breaking change is transitioned correctly, there are a lot of people possibly using the bad behavior and not knowing it. The other option is that merging this PR necessitates the next version of prawn released is 3.0, but I'm not personally comfortable releasing a breaking 3.0 right now so soon after 2.0. I would love ideas and any discussion on the ramifications of those choices. |
Precisely how I envisioned it.
Do you really think there are that many people using gradients directly? I've been using Prawn for almost 2 years and I never saw a need for it (the need comes in the form of the infrastructure necessary for libraries like prawn-svg to work). I tend to agree that it is too soon for Prawn 3, so the best path forward (if the agreement is that this is a critical breaking change) is to allow it to be opt-in. prawn-svg could even cause this switch to be enabled when activated (that's up to prawn-svg). |
|
I use both prawn-svg and gradients in my documents. But I don't use transformations. I don't think changing core behaviour in prawn-svg is a nice thing to do. Probably, we can create a new gem for this very specific thing. Warn in Prawn when gradients used with transformations that it's going to change in 3.0 and that there's this one gem you should use to get correct behaviour. prawn-svg can warn users too when incorrect behaviour is evoked (but prawn would probably detect it anyway). |
|
I feel like the external gem might be a bit fiddly given the change touches three separate classes. I'd suggest that we add an option to the fill_gradient/stroke_gradient call, something like apply_transformations: true. It'll initially default to false, and maybe log a warning if you don't explicitly set it false or true. As you suggest @packetmonkey, it'd make sense to change the default to true in prawn 3. Downside to the approach is the apply_transformations option hangs around afterwards. It's a pretty simple switch, so I don't think it'll negatively impact code complexity to keep it there. |
|
I like that proposal. (I'm not sure where the impression came from of
adding a separate gem. I wouldn't want it to be solved that way either.)
|
|
I think this sounds like the best plan, lets make the new behavior opt-in for now and when we cut prawn v3 we will make it the default behavior and remove the old. This also introduces a new question, how do we mark it as deprecated. I'm inclined to throw a Thoughts? |
|
+1 on |
As discussed on prawnpdf#894, we don't want to break existing code that uses Prawn where the coder has manually calculated and applied the transformation to the gradient co-ordinate arguments. We add an :apply_transformations option that defaults to false and a warning when the option is not supplied and a transformation has been carried out. It's expected this will default to true in Prawn v3.
As discussed on prawnpdf#894, we don't want to break existing code that uses Prawn where the coder has manually calculated and applied the transformation to the gradient co-ordinate arguments. We add an :apply_transformations option that defaults to false and a warning when the option is not supplied and a transformation has been carried out. It's expected this will default to true in Prawn v3.
mogest
force-pushed
the
transforming-gradients
branch
from
8f6a65d to
2028a2a
Compare
|
OK, :apply_transformations option added, warn displayed if option not explicitly supplied and a transformation was made (no point warning the user if turning on the option wouldn't result in different behaviour, IMO.) Added some info to the method comments, and a link to a non-existent wiki page! |
|
Alright I took some time to check out the branch and play with it. Everything is looking good in my tests when I used a scale block and a rotate block. I think this is working as good as it can be. I went ahead and set up the wiki page. I'm open to any feedback / edits on making that easier for developers to understand as this is the first time we are really executing a deprecation migration plan in Prawn, so if there is a precedent we want to set, now is the time. I'm looking for another 👍 or two but I think this is ready to go in, for sure before the next prawn release. @cheba Agree? @mogest Just throwing it out there that you rock :) Thanks a ton for pulling this together. |
|
@mogest Also please add an entry to the CHANGELOG for this. |
|
Great job on the wiki page @packetmonkey and another big thumbs up to @mogest for implementing the new behavior. Great teamwork! |
|
⭐ @mogest Well done! |
|
Thanks everyone! And thanks for the great contributing experience. Wiki page looks great, changelog committed, ready to roll hopefully! |
Track transformations and apply to gradients' matrices
|
Aaannddddd merged :) All glory to prawn-svg
Thanks again! |
As discussed on prawnpdf#894, we don't want to break existing code that uses Prawn where the coder has manually calculated and applied the transformation to the gradient co-ordinate arguments. We add an :apply_transformations option that defaults to false and a warning when the option is not supplied and a transformation has been carried out. It's expected this will default to true in Prawn v3.
PDF gradients/patterns take coordinates in the coordinate space of the
document, not the "user space", so if you perform a scale/rotate/translate
and then paint a gradient inside, it doesn't render correctly.
This commit tracks transformations applied to the document, and multiplies
the gradient matrix with this tracked transformation matrix so that the
gradient appears in the correct place in the document.
This has been created in response to issue #891.