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

Pad matrix cell to make matrix have a consistent height #4153

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

Conversation

Enter-tainer
Copy link
Contributor

fix #1617

@MDLC01
Copy link
Contributor

MDLC01 commented May 16, 2024

The content of 1-dimensional horizontal matrices seems to always be slightly higher than before (this is especially visible in test issue-852-mat-type). Is this intended?

@Enter-tainer
Copy link
Contributor Author

i'm not sure if it is intended or not. i can have a closer look in this weekend

@Enter-tainer
Copy link
Contributor Author

Enter-tainer commented May 16, 2024

i think it is intended. this pr set a minimal ascent and descent for each cell. and before that, the descent should be zero, and now it has a non zero descent. so things inside the matrix appears to be higher than before.

@Enter-tainer
Copy link
Contributor Author

i'm not 100% sure though. i will experiement with this tomorrow

@Enter-tainer
Copy link
Contributor Author

IMG_20240517_012728.jpg

It it pretty close to KaTeX's behavior. and i think real latex should look similar to this

@MDLC01
Copy link
Contributor

MDLC01 commented May 16, 2024

Great, then! Just wanted to make sure this wasn't an oversight.

@Enter-tainer
Copy link
Contributor Author

One thing looks weird is that typst doesn't scale the delimiter like KaTeX does.

image

@Enter-tainer Enter-tainer force-pushed the mgt/matrix-cell-padding branch 2 times, most recently from 1394827 to d480e05 Compare May 17, 2024 15:56
@Enter-tainer Enter-tainer marked this pull request as draft May 17, 2024 17:51
@Enter-tainer
Copy link
Contributor Author

I need time to understand what \matrix in plain tex means

@Enter-tainer Enter-tainer marked this pull request as ready for review May 18, 2024 06:13
@Enter-tainer
Copy link
Contributor Author

Enter-tainer commented May 18, 2024

In plain tex there is no such padding. But plaintex ensures that the distance between each row's baseline is at least \baselineskip. There is also a padding in amsmath's matrix environment. So I mark this PR ready.

@Enter-tainer Enter-tainer force-pushed the mgt/matrix-cell-padding branch 2 times, most recently from 89f2d0a to cd0da54 Compare May 23, 2024 06:47
Copy link
Member

@laurmaedje laurmaedje left a comment

Choose a reason for hiding this comment

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

We should definitely use the row gap in my opinion. Though it's hard to decide to what. I think the current value was just a random on the spot decision that I thought looks good.

tests/suite/math/mat.typ Outdated Show resolved Hide resolved
crates/typst/src/math/matrix.rs Outdated Show resolved Hide resolved
@Enter-tainer
Copy link
Contributor Author

We should definitely use the row gap in my opinion. Though it's hard to decide to what. I think the current value was just a random on the spot decision that I thought looks good.

maybe we can keep it untouched at this moment?

@laurmaedje
Copy link
Member

That was a bad typo from me: I meant "definitely reduce", not "definitely use".

@Enter-tainer
Copy link
Contributor Author

Enter-tainer commented May 28, 2024

I personally perfer something between 0.1em to 0.2em. Maybe 0.2em is the best one. 0.1em seems to narrow in the 3 row matrix

#set page(height: auto)
#let testgap(..content) = {
  let arr = ()
  for i in range(1,11) {
    let row-gap = i * 0.05em
    let gapmat = math.mat.with(row-gap: row-gap)
    let res = [ row-gap = #row-gap \ #block(inset: 5pt)[#gapmat(..content)]]
    arr.push(res)
  }

  table(columns: 5, ..arr)
}

= 2d Display
$ testgap(a,b;c,d) $
= 2d Inline
$testgap(a,b;c,d)$
= 3d Display
$ testgap(a,,;,b,;,,c) $
$ testgap(a,f,dots;dots.v,b,g;dots.down,c,t) $
= 3d Inline
$testgap(a,,;,b,;,,c)$
$testgap(a,f,dots;dots.v,b,g;dots.down,c,t)$

matrices

KaTeX

image
image
image

@laurmaedje
Copy link
Member

I and @reknih like 0.2em as well. Could you maybe make another comparison image with uppercase letters and numbers, just for confirmation that these look good, too?

@Enter-tainer
Copy link
Contributor Author

Here it is. I made a mistake in previous comparison image: seems that all of them are inline equations. New comparison code fix this.

matrices

#set page(height: auto)
#let testgap(isblock: true, ..content) = {
  let arr = ()
  for i in range(2, 7) {
    let row-gap = i * 0.05em
    let gapmat = math.mat.with(row-gap: row-gap)
    let res = [ row-gap = #row-gap \ #block(inset: 5pt)[#math.equation(block: isblock, gapmat(..content))]]
    arr.push(res)
  }

  table(columns: 5, ..arr)
}

= Display
$testgap(A,B;C,D;)$
$testgap(1,2;3,4)$
$testgap(a,b;c,d)$
$testgap(A,B,Z,X;W,Y,G,F;I,K,J,L;W,M,N,T)$
$testgap(a,,;,b,;,,c)$
$testgap(a,f,dots;dots.v,b,g;dots.down,c,t)$

= Inline
$testgap(A,B;C,D; isblock: #false)$
$testgap(1,2;3,4; isblock: #false)$
$testgap(a,b;c,d; isblock: #false)$
$testgap(A,B,Z,X;W,Y,G,F;I,K,J,L;W,M,N,T; isblock: #false)$
$testgap(a,,;,b,;,,c; isblock: #false)$
$testgap(a,f,dots;dots.v,b,g;dots.down,c,t; isblock: #false)$

@laurmaedje
Copy link
Member

Thank you! I think it all looks good, 0.2em it is!

@laurmaedje
Copy link
Member

I took a look at the changed test images and am a bit confused by how the row gap reflects there.

Why did the spacing in tests/ref/gradient-math-mat.png increase, but in tests/ref/gradient-math-misc.png it is suddenly super tight?

@Enter-tainer
Copy link
Contributor Author

I find that some images becomes taller but some becomes shorter. I think this is because current padding only applies to matrices, but the default row gap not only controls mat but also influence vec. And vec doesn't has such padding logic at this moment. Similar case is that math.cases testcase becomes smaller. Let me see how to add padding for vec

@laurmaedje
Copy link
Member

Ah, that makes sense. I think adding the same logic for vec and cases (basically anything with row gap) makes sense.

@Enter-tainer
Copy link
Contributor Author

Ah, that makes sense. I think adding the same logic for vec and cases (basically anything with row gap) makes sense.

Agree. I look at the code and I'm not pretty sure how to do it. cases and vec calls layout_vec_body, and it calls stack. It is stack that actually calculates the height, width and gap of boxes. I'm a little bit concerned that we may not be able to "hack it directly by using height.min()" because stack is also used by underover. Blindly do a min/max operation may have unexpected behavior for underover?

@Enter-tainer
Copy link
Contributor Author

Maybe adding a parameter to stack that controls whether to do padding or not? I'm not quite convinced because it doesn't sound good

@laurmaedje
Copy link
Member

The important part is that matrix and vector behave similarly. Depending on which one turns out better in the code, a parameter in stack or a new from-scratch layout_vec_body would both work, I think.

@Enter-tainer
Copy link
Contributor Author

Enter-tainer commented May 30, 2024

I think i've understand what's happening.

interestingly, cases are still shorter than before, while matrices generally becomes taller than before. I need figure it out tomorrow
note in this case, the brace becomes taller

This PR did 2 things:

  1. reduce row gap (-gap)
  2. pad matrix row height to be at least as high as ( (+height)

This result in 2 change in tests: some matrices become taller, some become shorter.

  • For those become taller: this is because the previous row height is smaller than (, and amount of +height is more than -gap
  • For those become shorter: this is because each row is already pretty high, so the -gap has more impact on the final height.

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.

Misaligned Matrix/Vector Brackets
3 participants