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

Relative calculations #357

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Relative calculations #357

wants to merge 5 commits into from

Conversation

MarcinZiabek
Copy link
Member

@MarcinZiabek MarcinZiabek commented Oct 4, 2022

Relative position

Description and API

The RelativePosition element is a generalization of the Alignment element. Instead of using predefined positions (left, center, right, top, middle, bottom), it uses relative sizes of parent and its child.

This control is available in FluentAPI via two methods:

public static IContainer RelativePositionVertical(this IContainer element, float parentOffset, float childOffset);
public static IContainer RelativePositionHorizontal(this IContainer element, float parentOffset, float childOffset);

Fun fact: this is a classic example of branch-less programming. The code, instead of using conditions, uses proper multiplication. However, I am not expecting any significant performance improvements.

Replacement of Alignment element

Using all 4 parameters, it is possible to achieve any relative positioning. Including already existing, basic cases:

.AligntLeft()
.RelativePositionHorizontal(0, 0)

.AligntCenter()
.RelativePositionHorizontal(0.5f, -0.5f)

.AligntRight()
.RelativePositionHorizontal(1f, -1f)

.AligntTop()
.RelativePositionVertical(0, 0)

.AligntMiddle()
.RelativePositionVertical(0.5f, -0.5f)

.AligntBottom()
.RelativePositionVertical(1f, -1f)

More advanced usecase

However, it is also possible to cover more advanced positioning, for example:

.Width(500)
.Height(500)

.Padding(100)
.Background(Colors.Grey.Lighten1)

.RelativePositionVertical(0.5f, -0.5f)
.RelativePositionHorizontal(1f, -0.5f)

.Width(120)
.Height(160)

.Background(Colors.Grey.Darken2);

Gives the following result:

Relative size

Description and API

The RelativeSize element provides the ability to proportionally fill certain area.

Important: I am not sure yet about usefulness of this element, or even about exact behaviour and implementation. This should be considered as experimental. I am happy to start a discussion.

This control is available in FluentAPI via two methods:

.RelativeWidth(0.4f)   // take 40% of parent's width
.RelativeHeight(0.6f)  // take 60% of parent's height

Replacement of Extend element

The current implementation, can be used to replace the Extend element, like so:

.ExtendHorizontal()
.RelativeWidth(1f)

.ExtendVertical()
.RelativeHeight(1f)

Example

Let's analyse a simple examples:

.Width(500)
.Height(500)
    
.Padding(100)
.Background(Colors.Grey.Lighten2)

.AlignCenter()
.AlignMiddle()

.RelativeWidth(0.4f)
.RelativeHeight(0.8f)
.Background(Colors.Grey.Darken2);

That gives the following result:

Rendering stability problem

Even with theoretically correct implementation, this element is not stable and can create a feedback loop. Many elements in QuestPDF perform additional measurement operations of their children. There are many reasons of this design, for example, the Alignment element needs to know exact size of its child, in order to correctly place it on the canvas.

To better understand the problem of the current implementation (commit fa3d912), let's take a look at the following example:

.Width(500)
.Height(500)
    
.Padding(100)
.Background(Colors.Grey.Lighten2)

// notice repeated alignment operations
.AlignCenter() 
.AlignMiddle()

// container makes sure that multiple Alignment elements are created, 
and therefore multiple Measure operations are executed
.Container() 

.AlignCenter()
.AlignMiddle()

.Container()

.AlignCenter()
.AlignMiddle()

.RelativeWidth(0.4f)
.RelativeHeight(0.8f)
.Background(Colors.Grey.Darken2);

The code suggests, that the result should be identical to the previous example. But it isn't:

Relative padding

Description and API

The RelativePadding element could be used as an alternative to the RelativeSize element.

There are two significant differences:

  1. It takes all provided space.
  2. It gives control over child placement.

Important: is there any better name? (flutter uses the Align name)

This control is available in FluentAPI via the following methods:

public static IContainer RelativePaddingLeft(this IContainer element, float value);
public static IContainer RelativePaddingRight(this IContainer element, float value);

public static IContainer RelativePaddingTop(this IContainer element, float value);
public static IContainer RelativePaddingBottom(this IContainer element, float value);

Example

Let's analyse a simple examples:

.Width(250)
.Height(250)
    
.Padding(50)
.Background(Colors.Grey.Lighten2)

.RelativePaddingLeft(0.1f)
.RelativePaddingTop(0.2f)
.RelativePaddingRight(0.3f)
.RelativePaddingBottom(0.4f)

.Background(Colors.Grey.Darken2);

That gives the following result:

Why replacement for Relative Size

RelativeWidth = 1 - RelativePaddingLeft - RelativePaddingRight
RelativeHeight = 1 - RelativePaddingTop - RelativePaddingBottom

Shrink API

Description and API

In many situations, the parent container may provide more space that is necessary for its child to render. In such a case, a bigger area is being enforced. However, sometimes we want to cover only the minimal area necessary, and therefore loosen size constraints provided by the parent.

The Shrink element is a generalized and renamed version of the MinimalBox element.

// it allows to minimize takes space vertically or horizontally:
.ShrinkVertical()
.ShrinkHorizontal()

// or both (that is the same as deprecated MinimalBox)
.Shrink()
.MinimalBox()

Examples

RenderingTest
    .Create()
    .PageSize(300, 200)
    .ProduceImages()
    .ShowResults()
    .Render(container =>
    {
        container
            .Padding(20)
            .Border(2)
            .Shrink() // without / ShrinkHorizontal / ShrinkVertical / Shrink
            .Background(Colors.Grey.Lighten2)
            .Padding(20)
            .Text("This is test.")
            .FontSize(20);
    });

Without:

With ShrinkHorizontal:

With ShrinkVertical:

With Shrink (both horizontal and vertical):

@MarcinZiabek
Copy link
Member Author

@Bebo-Maker I would love to know your thoughts about features in this PR. I consider it as a simple experiment that may potentially solve some specific corner cases.

@bennetbo
Copy link
Contributor

bennetbo commented Oct 6, 2022

Just played around with this for a while. Looking great so far!
I really think this can help to create some complex layouts.

One thing i noticed during testing:
If the specified relative height/width (.RelativeHeight(..)) is too small for the child (e.g. text) its not visible. But if i use a fixed height (.Height(..)), a layout exception gets thrown, so there seems to be an inconsistency there.

Code:

page.Content()
    .Column(c =>
    {
        c.Item()
         .AlignCenter()
    
         .RelativeHeight(0.2f) // works fine
         //.RelativeHeight(0.1f) // item is not visible anymore
         //.Height(20) // throws layout exception
    
         .Background(Colors.Yellow.Accent4)
         .Text("123");
    });

Not sure if this is related to the 'rendering stability problem' you mentioned. I have not spent any time debugging this, will take a look at it later.

@MarcinZiabek
Copy link
Member Author

Thank you for taking a look 😁 Having somebody well-skilled in the library, and with a fresh mind - this helps so much!

Not sure if this is related to the 'rendering stability problem' you mentioned. I have not spent any time debugging this, will take a look at it later.

Yes, I think that your experience can be connected to that measurement issue. And I do not see yet any obvious workarounds, without changing entire architecture concept.

I love this size-related API (RelativeWidth and RelativeHeight), just not sure how to fix it.

@bennetbo
Copy link
Contributor

bennetbo commented Oct 6, 2022

I get it know, its exactly like you described. Removing the alignment (.AlignCenter()) call fixed the problem - Code throws a layout exception. Just needed some time to get back into debugging the layout algorithm.

I dont see any option to fix this with the current layout algorithm - like you already mentioned.
However the new API creates a lot of possibilities for complex layouts. Maybe you could release it as an 'Experimental' API, to gather some more feedback?

@MarcinZiabek MarcinZiabek added this to the 2022.12 milestone Oct 19, 2022
@zlatanov
Copy link

zlatanov commented Nov 13, 2022

@MarcinZiabek I am wondering why you chose to have RelativePadding, RelativePosition when these can all be covered if you introduce a new Unit - Percent.

Relative position could be renamed to Margin and be used either with absolute values or percentage as in your examples?

This could have much broader impact on the API capabilities, as one would be able to use percentages everywhere where you're supporting a unit value, except may be the page size.

@MarcinZiabek
Copy link
Member Author

@MarcinZiabek I am wondering why you chose to have RelativePadding, RelativePosition when these can all be covered if you introduce a new Unit - Percent.

Relative position could be renamed to Margin and be used either with absolute values or percentage as in your examples?

This could have much broader impact on the API capabilities, as one would be able to use percentages everywhere where you're supporting a unit value, except may be the page size.

This a fair question with a rather simple answer: complexity.

Right now, the library uses points to measure size, where point is defined as 1inch = 72 points by PDF standards. Other units are converted to points inside Fluent API, so internal implementation is not impacted (and still uses points).

Adding a new Unit.Pecent option changes the character of calculation. It can no longer be performed in FluentAPI as it may require knowledge of the current canvas size. This moves entire calculation logic from the FluentAPI to the drawing layer. It is not bad, just impacts many places and requires careful changes.

I am still considering the approach you have described. It just requires significantly more work. If I decide to do so, this PR will be moved to one of the future releases. I love the Unit-based approach but this also delays everything.

Let me also thank you for sharing your thoughts ❤️ If you see any alternatives or other approaches, please share 😁

@zlatanov
Copy link

Adding a new Unit.Pecent option changes the character of calculation. It can no longer be performed in FluentAPI as it may require knowledge of the current canvas size.

It would if you stored the configuration/definition in points as you currently do, but if instead you stored them as a UnitValue, then you wouldn't have this problem?

Anywhere later in the rendering phase, when you actually need these values, you would do something like:

var points = Left.ToPoints(availableSize.Width);

I may be missing something, as I am unfamiliar with the actual details of the renderer, but at first glance this seems doable.

@MarcinZiabek
Copy link
Member Author

MarcinZiabek commented Nov 13, 2022

Yes, this is basically what needs to happen, the conversion inside rendering code. I think that it is going to be quite straightforward in most cases. Of course it may require some caching in more heavy scenarios. As will increase memory usage (float vs float+enum).

There are also corner cases:

.Padding(1, Unit.Inch)
.Padding(15, Unit.Point)

The code above should produce a padding with total size 1 inch + 15 points. The current code generates only one padding element with added padding values, instead of one element by method invocation. Similar optimization is applied in several places.

However, when the summing operation needs to happen during rendering logic, this optimization cannot happen anymore. So this may also require some measurements to see how significant is the performance regression.

@zlatanov
Copy link

zlatanov commented Nov 14, 2022

There are also corner cases:

.Padding(1, Unit.Inch)
.Padding(15, Unit.Point)

The code above should produce a padding with total size 1 inch + 15 points. The current code generates only one padding element with added padding values, instead of one element by method invocation. Similar optimization is applied in several places.

This should be fine, because underneath you could still keep converting all units but percentages to points and the code above would still work. However, if we wrote instead:

.Padding(1, Unit.Inch)
.Padding(10, Unit.Percent)

Then we would be unable to fold both of the constructs in the same container, which is fine in my opinion :).

However, when the summing operation needs to happen during rendering logic, this optimization cannot happen anymore. So this may also require some measurements to see how significant is the performance regression.

I will gladly help, if you want. I am confident that such a change should not introduce any performance regressions. The only possible issue would be memory usage as you pointed out, but I doubt it would be measurable compared to the memory needed for the entire document model.

@MarcinZiabek
Copy link
Member Author

I will gladly help, if you want.

Feel free to prepare a PR 😁 I does not need to cover entire feature, just a prototype is already great. However, I cannot commit to any time frame regarding the release process. I have limited amount of time and need to be extra careful when publishing changes. I am hoping to focus more on the project in 2023.

I am confident that such a change should not introduce any performance regressions. The only possible issue would be memory usage as you pointed out, but I doubt it would be measurable compared to the memory needed for the entire document model.

I am already investigating huge memory optimizations that I would like to introduce mid-2023. Essentially, reusing objects and significantly reducing GA overhead. But before I commit to such changes, I need to improve first the test coverage. And in this domain I also have some ideas on how to improve testing framework 😁 So, let's not worry much about memory as for now.

@zlatanov
Copy link

Feel free to prepare a PR

Nerd sniping at its finest. I definitely will. 💯

@MarcinZiabek
Copy link
Member Author

The Shrink API has been released to main in a3ae684

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

Successfully merging this pull request may close these issues.

None yet

3 participants