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

wip: Subset fonts by fontstyle #1248

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShupingHe
Copy link
Contributor

@ShupingHe ShupingHe commented Apr 26, 2023

#1107

image

this pr helps reduce more font size of diagram

@ShupingHe
Copy link
Contributor Author

ShupingHe commented Apr 26, 2023

below is the overall idea of this pr:
well in the previous optimization #1089 , we collect all the characters, called corpus, inside a diagram, and then subset our font by cutting the unnecssary fonts. what remains after this optimization is, say,if we only have a bold a character, we'd still include both regular or other fonts for this a character.
so, what we need to do is when collecting characters, we also need their style info, then when we subset our fonts, we can get rid of all unnecessary fonts.

@ShupingHe ShupingHe force-pushed the subset-fonts-by-fontstyle branch from 26b9cd2 to 98a7b5e Compare April 26, 2023 09:52
@ShupingHe
Copy link
Contributor Author

@alixander i asked you some questions in the related issue but got no responses... anyway, no worries

i draft this pr, can you help to take a glimpse, so i can know if i'm on the right direction

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

this looks like the right approach!

the e2e test deltas have some errors though

(btw to get these, i just run
git checkout master e2etests/testdata
and then
./ci/e2ereport.sh --delta
)

Screen Shot 2023-04-26 at 4 58 29 PM

Screen Shot 2023-04-26 at 4 58 48 PM

@ShupingHe
Copy link
Contributor Author

ShupingHe commented Apr 28, 2023

@alixander for this script:

costumes: {
  shape: sql_table
  id: int {constraint: primary_key}
}

i notice that for the generated diagram, text consumes is bold. the text style is added here

isBold := !obj.IsContainer() && obj.Shape.Value != "text"

image

but inside the output svg, class text-bold can't be found, is that expected ? is it a bug?
image

@alixander
Copy link
Collaborator

@donglixiaoche ah, yeah i think that is a bug. it just didn't affect anything because Class has its own renderer that ignored the value set on d2target:

func classHeader(diagramHash string, shape d2target.Shape, box *geo.Box, text string, textWidth, textHeight, fontSize float64) string {

@ShupingHe
Copy link
Contributor Author

ShupingHe commented May 4, 2023

@alixander but basically we add bold font according to text-bold class, and after this optimization, if no text-bold is found, bold font won't be included. so the bug you mentioned above is caused by this bug.

do you think i need fix this bug here?

@alixander
Copy link
Collaborator

yes, i think it was just an innocuous bug until now, but this method will require that bugfix

@ShupingHe ShupingHe changed the title Subset fonts by fontstyle wip: Subset fonts by fontstyle May 16, 2023
@ShupingHe ShupingHe marked this pull request as draft May 16, 2023 18:54
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.

2 participants