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

Adding a ascent/descent setter in frame so that the original frame size is not lost in equation slackness adjustment #4318

Closed
wants to merge 3 commits into from

Conversation

LuxxxLucy
Copy link
Contributor

@LuxxxLucy LuxxxLucy commented Jun 2, 2024

In equations, there is an adjustment of the frame size, where the frame height (size().y) is changed so that a large equation will be adjusted to not effect the later line layout (as it effects the line height)

let MathParItem::Frame(frame) = item else { continue };
 
             let font_size = scaled_font_size(&ctx, styles);
             let top_edge = TextElem::top_edge_in(styles).resolve(font_size, &font, None);
             let bottom_edge =
                 -TextElem::bottom_edge_in(styles).resolve(font_size, &font, None);
 
            let ascent = top_edge.max(frame.ascent() - slack);
            let descent = bottom_edge.max(frame.descent() - slack);
             frame.translate(Point::with_y(ascent - frame.baseline()));
             frame.size_mut().y = ascent + descent;
         }

However, if the frame.size().y is modified here, we will not be able to do decoration of the frame (see #2200) , because the frame size is not correct anymore. This PR adds the interface to set ascent/descent of the frame, and so that the line height determination will rely to the value of ascent and descent.

Below shows the result after this PR's change, the red line is the baseline, and the blue line is the ascent, while the background is true frame size. The frame size is not modified.

image

Note 1:

The change of this PR should also be relevant to #1028 #4236

Note 2:

this PR does not break any other test, except for #2220, the reason being the descent being changed. Before:
Snipaste_2024-06-01_20-04-18
After
Snipaste_2024-06-01_20-02-51

Comment on lines +34 to 40
/// The ascent of the frame. If this is `None`, the
/// frame's implicit ascent is the baseline.
ascent: Option<Abs>,
/// The descent of the frame. If this is `None`, the
/// frame's implicit descent is size minus baseline.
descent: Option<Abs>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think size + baseline already determines ascent and descent. The box model should look like this

          ────┬──────────────────────────┬───    
            ▲ │                          │ ▲     
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
          asc │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ size.y
            │ │                          │ │     
            │ │                          │ │     
            │ │                          │ │     
baseline  ──┴─┼──────────────────────────┤ │     
            ▲ │                          │ │     
            │ │                          │ │     
       desc │ │                          │ │     
            ▼ │                          │ ▼     
          ────┴──────────────────────────┴────   

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that ascent+descent != size.y, as in the case of the equation slackness adjustment. In the slackness adjustment, the size (the original size of the frame) is larger than the ascent+descent.

The ascent and descent will be used by inline layout for determining the line height while the size still refers to the original size.

Copy link
Contributor

@Enter-tainer Enter-tainer Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that ascent+descent != size.y

I highly suspect this is not a good idea. asc + desc = size.y should be an invariant.

Copy link
Contributor Author

@LuxxxLucy LuxxxLucy Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what the slackness adjustment does

          ────┬──────────────────────────┬───    
              │                            ▲     
              │                            │
              │                            │     
              │                            │     
          ────┬─────────────┬              │  
            ▲ │                            │     
            │ │                            │     
          asc (adjusted by slackness)      │     
            │ │                            │     
            │ │                            │     
            │ │                            size.y
            │ │                            │     
            │ │                            │     
            │ │                            │     
baseline  ──┴─┼──────────────────────────┤ │     
            ▲ │                            │     
            │ │                            │     
       desc │ │                            │     
            ▼ │                            ▼     
          ────┴──────────────────────────┴────   

previously the ascent/descnet is not changed, but the size y is directly changed.

          ────┬──────────────────────────┬───────
              │                                 ▲
              │                                 │
              │                                 │
              │                                 │
          ────┬──────────────────────────┬───   │ 
            ▲ │                            ▲    true size.y
            │ │                            │    │ 
            │ │                            │    │ 
            │ │                            │    │ 
            │ │                            │    │ 
            │ │                            size.y (modified by the slackness adjustment)
            │ │                            │    │ 
            │ │                            │    │ 
            │ │                            │    │ 
baseline  ──┴─┼──────────────────────────┤ │    │ 
            ▲ │                            │    │ 
            │ │                            │    │ 
       desc │ │                            │    │ 
            ▼ │                            ▼    ▼ 
          ────┴──────────────────────────┴───────   

Copy link
Contributor

@Enter-tainer Enter-tainer Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slackness adjust for inline equations adjust is... a quick & dirty workaround in my opinion, and hopefully I want to remove it my PR #4236 (I'm still waiting @laurmaedje 's comment on the line height proposal #4224). There is no such adjust in proper typesetting systems like LaTeX.

Copy link
Contributor

@Enter-tainer Enter-tainer Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be impossible to make #4236 pass the test

Sorry, maybe there is a gap. I don't really get this point though. Currently there are about ~200 test doesn't pass. This is because I haven't fix line height for termlist/unordered list/ordered list yet. Another take is that you can always "pass" the test by updating the reference images..?

And slackness is properly handled in Latex and I think @laurmaedje has explained and provided examples.

believe me, tex doesn't have this adjust. The tex solution is described in #4224. Namely, tex determines line gap in 2 stages: in normal cases, it tries to make the distance between lines equals to \baselineskip. But if this makes lines overlap, it will ensure that the distance of the bottom edge of the previous line and the top edge of current line is 1pt.

Typst has a static gap for all cases. This makes it look bad when there are large items like inline equations. So we need extra "slackness adjust" to make it look right in most cases. But this also cause #1028.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think this height is the ascender (or for the math equations, the slackness adjusted height), instead of the actual size of this particular glyph.

This is unfortunately not true. slackness adjust only happens in equations that is to be inserted into normal text flows. They don't exist in other cases. For example:

has slacknessadjust: $a^2+b^2=c^2$

has slackness adjust as a whole, but no slackness adjust for each individual case arms. 

$cases(a^2+b^2=c^2, a=3, b=4, c=5)$

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I learn a lot. Thank you.

I am okay as long as the original size is not lost as I am interested in #2200; that being said, I still think it would be difficult to make #4236 pass the test (and I mean by preserving the existing result without breaking the tests, but I am not very familiar with it so I can be wrong). The line height adjustment seems to be complicated, if we are really going for it, a complete collision detection procedure needs to be implemented and some heuristic would be inevitable. I hope it can be done soon.

Do you think it is possible to get a predecessor PR to #4236 that can preserve the frame size?

Copy link
Contributor

@Enter-tainer Enter-tainer Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay as long as the original size

#4236 removes the slackness adjust, so inline math has correct bbox, ascent and descent.

I still think it would be difficult to make #4236 pass the test

Agree, the term list thing is not easy to fix. One thing to notice about typst's test system is that the reference image can be update with --update when running tests. This is especially useful when rendering result is expected to change.

The line height adjustment seems to be complicated, if we are really going for it, a complete collision detection procedure needs to be implemented and some heuristic would be inevitable.

To my surprise, it is not that hard(at least for most cases). Typst already maintains the exact bbox for each element, and the collision test code can be seen in https://github.com/typst/typst/pull/4236/files#diff-6c11c99cb19f6dd03f76013fec99eb36081d56355d9b5ed8546f1d785bd49af4L323-L325 .

You can see the inline equation overlap issue solved in this image: https://github.com/typst/typst/pull/4236/files#diff-d5c29466f38994d7e7c7337b0fee01231dc6a45d0a65b37807d38e3a2647cae1

I hope it can be done soon.

Me too! I didn't work on it last week because I'm (a) working on #4153 (b) waiting laurmaedje's comment on the proposal. I will continue to work on this pr.

Do you think it is possible to get a predecessor PR to #4236 that can preserve the frame size?

I'm not really sure. I haven't think of this in detail(because I bet on the line height solution!), there might be no "non-hacky" way to do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on what we settle on in #4224, but in principle I agree with @Enter-tainer that ascent + descent = size.y is a good invariant to have, and that the equation slack is more of a hack.

@LuxxxLucy
Copy link
Contributor Author

Closing in favour of waiting for #4224

@LuxxxLucy LuxxxLucy closed this Jun 7, 2024
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