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

Images dont respect margins? #1156

Open
Lucas-C opened this issue May 5, 2024 Discussed in #1155 · 7 comments
Open

Images dont respect margins? #1156

Lucas-C opened this issue May 5, 2024 Discussed in #1155 · 7 comments

Comments

@Lucas-C
Copy link
Member

Lucas-C commented May 5, 2024

Discussed in #1155

Originally posted by joren-dev May 4, 2024
So I am using the self.image on a page, and while doing so I ran into the following problem
image

It clearly does not respect the bottom margin that I've said like so in the ctor:

margin_footer: int = 35
self.set_auto_page_break(True, margin_footer)

now the images are rendered as follows:

def print_images(self, images, fixed_width, max_per_row=0, vertical_spacing=5):
    
    image_info = None
    images_in_row = 0
    
    # Iterate through images
    for image_path in images:
        # If maximum images per row is set and reached, move to the next row
        if max_per_row != 0 and images_in_row == max_per_row:
            self.set_x(self.l_margin)  # Reset x position
            self.set_y(self.get_y() + image_info.rendered_height + vertical_spacing)  # Move to next row
            images_in_row = 0  # Reset images in row counter

        # If adding this image would exceed the page width, move to the next row
        if self.get_x() + fixed_width > self.epw:
            self.set_x(self.l_margin)  # Reset x position
            self.set_y(self.get_y() + image_info.rendered_height + vertical_spacing)  # Move to next row
            images_in_row = 0  # Reset images in row counter

        # Display image
        image_info = self.image(image_path, x=self.get_x(), y=self.get_y(), w=fixed_width)

        # Update current_x and images_in_row for next iteration
        self.set_x(self.get_x() + fixed_width + 10)  # Add 10 units spacing between images
        images_in_row += 1

    # Set y to below images, reset x to left margin
    self.set_xy(
        self.l_margin,
        self.get_y() + (image_info.rendered_height if image_info else 0) + 5,
    )

Now for both the width and the height I make sure that itll fit within the margins, is this intended? I would think that for the bottom margin it would auto break like with self.cell, self.tables, etc.

I solved the issue myself by adding:

# TODO: Figure out why images dont respect bottom margins
computed_image_height = calculate_image_height(image_path, fixed_width)

# Check if the image exceeds the page height
if self.get_y() + computed_image_height > self.page_break_trigger:
    self.add_page()  # Trigger page break
    self.set_xy(self.l_margin, self.t_margin)  # Reset x and y position

Prior to calling image_info = self.image(image_path, x=current_x, y=current_y, w=fixed_width) which solved the issue.

image

However having to do something alike for each image would be pretty inefficient and I have a feeling I am doing something wrong which causes my behavior, could someone help me solve this mystery?


I've tried to make a minimal working example:

from fpdf import FPDF

class PDF(FPDF):
    def __init__(self, orientation="P", unit="mm", format="A4"):
        super().__init__(orientation=orientation, unit=unit, format=format)
        self.set_auto_page_break(auto=True, margin=15)
        

# Create a new PDF object
pdf = PDF()

# Set up the document
pdf.set_title('My PDF Document')
pdf.set_author('John Doe')

# Add a chapter
pdf.add_page()
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)
pdf.ln(2)
pdf.image('settings/meter_reading_example.png', w=100)


# Output the PDF
pdf.output('test.pdf')

But it seems to respect the margins just fine:
image

So I am doing something wrong, just not sure what.

@joren-dev
Copy link

joren-dev commented May 5, 2024

Hi, I successfully created a minimal reproduction of the error

class PDF(FPDF):
    def __init__(self, orientation="P", unit="mm", format="A4"):
        super().__init__(orientation=orientation, unit=unit, format=format)
        self.set_auto_page_break(auto=True, margin=15)
        
    def print_images(self, images, fixed_width):
        image_info = None
        
        # Iterate through images
        for image_path in images: 
            # Display image
            image_info = self.image(image_path, x=self.get_x(), y=self.get_y(), w=fixed_width)
            
            # Update y position to below the image + margin
            self.set_y(self.get_y() + image_info.rendered_height + 5)

# Create a new PDF object
pdf = PDF()

# Set up the document
pdf.set_title('My PDF Document')
pdf.set_author('John Doe')
pdf.add_page()

image_path = 'settings/meter_reading_example.png'
images_list = [image_path] * 30

pdf.print_images(images_list, fixed_width=100)

# Output the PDF
pdf.output('test.pdf')

image

@joren-dev
Copy link

joren-dev commented May 5, 2024

Seems like I've pinpointed the issue a bit further down:

    def print_images(self, images, fixed_width):
        image_info = None

        # Iterate through images
        for image_path in images:
            # Display image
            self.set_x(self.l_margin)
            self.image(
                image_path, w=fixed_width
            )
            self.ln(5)

Works just fine
image

Seems like providing custom x and y to self.image creates the issue: x=self.get_x(), y=self.get_y()

in _raster_image on line: 4023

        # Flowing mode
        if y is None:
            self._perform_page_break_if_need_be(h)
            y = self.y
            self.y += h
        if x is None:
            x = self.x

It seems like when y is passed, it will not check for _perform_page_break_if_need_be and that its intended that way. However in my case I have to pass y otherwise I cannot have my images in rows horizontally.

for image_path in images:
        if (max_per_row != 0 and images_in_row == max_per_row) or (
            self.get_x() + fixed_width > self.epw
        ):
            self.set_x(self.l_margin)  # Reset x position
            self.ln(vertical_spacing)
            images_in_row = 0  # Reset images in row counter

        # Display image
        image_info = self.image(
            image_path, w=fixed_width
        )

        # Update current_x and images_in_row for next iteration
        self.set_x(
            self.get_x() + fixed_width + 10
        )  # Add 10 units spacing between images
        images_in_row += 1

Respects the margin, but without specifying the y= in the image call I cannot get them horizontally aligned.

image

But if I were to pass the y= and get them horizontally aligned, it will stop respecting the border. I would say that it should always check if it should auto_break, or have another member passed to image like: force_auto_break or smth alike to make it possible to pass a custom y and still auto_break if the image is not possible within a margin.

@Lucas-C
Copy link
Member Author

Lucas-C commented May 5, 2024

Well done in finding the root cause of your problem!

But if I were to pass the y= and get them horizontally aligned, it will stop respecting the border. I would say that it should always check if it should auto_break, or have another member passed to image like: force_auto_break or smth alike to make it possible to pass a custom y and still auto_break if the image is not possible within a margin.

I see...
Have you tried setting pdf.y BEFORE calling pdf.image(), without passing y= to .image()?

from fpdf import FPDF

pdf = FPDF()
pdf.add_page()
pdf.set_y(pdf.h - 50)
pdf.image('test/image/image_types/insert_images_insert_jpg.jpg')
pdf.output('issue_1156.pdf')

@joren-dev
Copy link

joren-dev commented May 5, 2024

Well done in finding the root cause of your problem!

But if I were to pass the y= and get them horizontally aligned, it will stop respecting the border. I would say that it should always check if it should auto_break, or have another member passed to image like: force_auto_break or smth alike to make it possible to pass a custom y and still auto_break if the image is not possible within a margin.

I see... Have you tried setting pdf.y BEFORE calling pdf.image(), without passing y= to .image()?

from fpdf import FPDF

pdf = FPDF()
pdf.add_page()
pdf.set_y(pdf.h - 50)
pdf.image('test/image/image_types/insert_images_insert_jpg.jpg')
pdf.output('issue_1156.pdf')

So this is what I had before, with my print_images(), which gave:
image

Now I did what you suggested, with some small tweaks to work with my "max_per_row" system, here
The result is:
image

I did some debugging, now look at this:
image

This is the first iteration, the y is at the top margin as it should, seems fine and itll do line 27 so its setting y to 10.

image

Then second iteration, the y is 47.5, which is basically 10 + image height, okay so it moved the y down with the rendered height of my image, seems fine. However I correct this by doing the: self.set_y(self.get_y() - image_info.rendered_height). This line yields: 10 again, okay so far so good, except the image is printed on top of the other now.

The set_x() that adjusts the x value has not been changed, and if I were to pass x and y as done in here, it does put the image right next to it just fine.

Which seems to be caused by the following line: self.set_y(self.get_y() - image_info.rendered_height)

# self.get_x() gives correct value: 70.0
if images_in_row > 0:
    self.set_y(self.get_y() - image_info.rendered_height)  # Adjust y-coordinate
    # self.get_x() gives incorrect value after this call above: back to 10.0
else:
    self.set_y(self.get_y())  # Set y-coordinate as it is for the first image

image

Apparently setting y resets the x to l_margin, somehow I didnt know that.

So preserving the x prior to set_y solves the issue:
image

This is the final working code: https://pastebin.com/mHfaBD36

@joren-dev
Copy link

joren-dev commented May 5, 2024

So in short:

  • x resets to l_margin when adjusting y, that seems, incorrect but might be intended that way.
  • When passing x and y to image the bottom margins are not respected and _perform_page_break_if_need_be (auto page break) will not trigger
  • passing x to image has no effect to top/bottom margins, it wont respect r_margin but that seems logical

In order to have custom x/y one should set y and x prior to image call

correct_x = self.get_x()
if images_in_row > 0:
    self.set_xy(correct_x, self.get_y() - image_info.rendered_height)

correct_x ensures the x isnt reset to l_margin after adjusting y

I am fairly sure we would image to always respect the bottom margin, despite passing a custom y, so that should probably be fixed.

@Lucas-C
Copy link
Member Author

Lucas-C commented May 6, 2024

Other options that you have:

  • use FPDF.set_xy() to set both X & Y positions at the same time
  • directly set pdf.x = and pdf.y =

x resets to l_margin when adjusting y, that seems, incorrect but might be intended that way.

I agree that it can be unexpected, but this is the behaviour since fpdf2 exists I think.
That doesn't mean that this is good design, just that it would be delicate to change without breaking existing user code that uses this method.

Which seems to be caused by the following line: self.set_y(self.get_y() - image_info.rendered_height)

Thank you for the detailed analysis, but most of the debugging analysis you shared is about your own code (the print_image() method), and that does not really has much to do with the fpdf2 library itself... 😅

I am fairly sure we would image to always respect the bottom margin, despite passing a custom y, so that should probably be fixed.

I am not so sure.
Consider for example image insertion performed inside FPDF.footer(): in that case we may want to avoid page breaks in order to insert images in margins.

However, I agree that this behaviour should be better documented.
Maybe the docstring of FPDF.image() could include a sentence like:
"If y is provided, this method will not trigger any page break; otherwise, auto page break detection will be performed"

What do you think about this @joren-dev?
Would you like to contribute a PR regarding this?
Else I can handle this myself, no problem.

@Lucas-C
Copy link
Member Author

Lucas-C commented May 23, 2024

I made the minor docstring improvement in PR #1179

Unless you have other comments or questions @joren-dev, we will probably close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants