Open joewestcott opened 5 years ago
Wow -- good observation. This catches me entirely by surprise. You are correct about saving and resetting attributes as the following example shows:
package main
import (
"fmt"
"os"
"github.com/jung-kurt/gofpdf/v2"
)
func main() {
pdf := gofpdf.New("P", "mm", "A4", "")
st := gofpdf.StateGet(pdf)
pdf.AddPage()
pdf.TransformBegin()
pdf.TransformTranslateX(0)
pdf.SetFillColor(255, 127, 0)
pdf.Rect(0, 0, 10, 10, "F")
pdf.TransformEnd()
st.Put(pdf)
pdf.TransformBegin()
pdf.TransformTranslateX(10)
pdf.SetFillColor(255, 127, 0)
pdf.Rect(0, 0, 10, 10, "F")
pdf.TransformEnd()
fileStr := "transform.pdf"
err := pdf.OutputFileAndClose(fileStr)
if err == nil {
fmt.Printf("successfully generated %s\n", fileStr)
} else {
fmt.Fprintf(os.Stderr, "error %s: %s\n", fileStr, err)
}
}
I will look into this more and see what I can uncover. I am confused by section 8.4 in the reference: it seems that it is the reader's responsibility to stack and unstack the attributes before and after a transformation. How do you read that?
It seems that it is the reader's responsibility to stack and unstack the attributes before and after a transformation. How do you read that?
It seems like that's the case.
In terms of fixing it I can see two solutions, the first being to remove the existing 'is this value already set' checks before many of the setters (SetFontSize
, SetFillColor
, etc). The second being to implement our own graphics state in an array, and pop/push the values inside the TransformBegin
and TransformEnd
functions.
That could work like this:
We'll move all the relevant values (f.fontSize
, f.lineWidth
, f.color
, etc) out of the principal Fpdf struct, and into their own struct called GraphicsState, or something similar.
The principal fpdf struct will have a new graphicsState attribute added to it, of type []GraphicsState
.
All setters and getters operating on relevant values will now operate on the last GraphicsState.
TransformBegin
will initialise a new GraphicsState, with default values. TransformEnd
will remove the last.
Add additional documentation to TransformBegin
to explain how calling this method will start a new graphics state, resetting font size, fill color, etc to default values.
We could also add public methods to manually save/restore the graphics state, like pdfkit is doing here
None of these changes will affect the packages external API, but they should allow the internal state of gofpdf to be more accurate to the internal state of (properly conforming) readers.
These changes could cause PDFs to render differently after implementing, but those changes should only be more accurate to the programmer's original intention. (Setting a fill color will, indeed, change the fill color)
Thanks for the excellent summary, @joewestcott.
My inclination is to implement your first solution. These 'set only if the values are different' checks are fewer than the number of possible graphic attributes that would need to be bundled into a stackable GraphicsState. (Table 52 in the specification shows a lot of parameters.)
Regarding your second solution, when developing the charting functionality, I implemented the StateType structure so that the drawing of grids would not interfere with the current graphic state. The example shown above works because the fill color (among other graphic attributes) is being reset after the first transformation. This lets the fill color assignment work in the second transformation just as it did in the first. So a quick solution, slightly different than your solution, would be to simply push the return value of gofpdf.StateGet(pdf)
at the beginning of pdf.TransformBegin()
and pop it before returning from pdf.TransformEnd()
. (This is slightly different than your solution because StateGet()
simply takes a snapshot of the current graphic attributes and a state's Put()
method simply assigns these attributes back to their respective fields in the main pdf structure.) Had transformations been designed this way, there would be no need to use a stack. The return value pdf.TransformBegin()
would have and End()
method that would pop the graphics state.
I'll mull this over a little more, but my instinct favors the first approach.
I just commited 6b7c17b5b904039a423e8df6e51875702bae5763 that makes graphic property assignments unconditional. ExampleFpdf_SetFillColor shows transformations working as expected.
I just commited 6b7c17b that makes graphic property assignments unconditional. ExampleFpdf_SetFillColor shows transformations working as expected.
Amazing! Thanks for doing this. I've noticed a few more setters in the library that also exhibit the same issue, so I've submitted a PR (#235) removing state checks from the following methods. I think this is all of them!
SetFontSize
, SetFontUnitSize
, SetAlpha
, and SetDashPattern
.
Good catches, @joewestcott -- I completely missed those. And thanks for testing the changes.
I just factored the pattern drawing code in the transformation / graphic state test and noticed a bug. When pdf.SetXY()
is used to position the drawing cursor, the position is properly translated. But when pdf.CellFormat()
is used with text centering, the position appears to be translated by twice the amount it should. The problem seems to be that the page's margins are used in the positioning calculations and the translation methods do not account for this.
I'll reopen this issue while I look into this some more.
The following code should produce two orange rectangles at x:0 and x:10.
However, the second rectangle is black. It seems the second 'set' command is not writing the color change to the PDF, as gofpdf's internal state already has this color set. However, the call to
TransformBegin
has created a new graphics state stack in the PDF which has reset many font, line, and color parameters.To match the PDF spec It seems like we need to store our attributes in a stack, to be able to push them in
pdf.TransformBegin
, and pop the previous values back inpdf.TransformEnd
PDF spec, see 8.4 "Graphics State"