Skip to content

Commit

Permalink
SetSize should use 96 dpi for now
Browse files Browse the repository at this point in the history
I think it should probably use 96 dpi instead of 72.... In the next minor version of Lime I think we should change the function sig to allow a dpi argument.
  • Loading branch information
dimensionscape authored Oct 29, 2024
1 parent 85414fc commit 9cbdc83
Showing 1 changed file with 15 additions and 7 deletions.
22 changes: 15 additions & 7 deletions project/src/text/Font.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1040,14 +1040,22 @@ namespace lime {
}


void Font::SetSize (size_t size) {

size_t hdpi = 72;
size_t vdpi = 72;

FT_Set_Char_Size ((FT_Face)face, (int)(size*64), (int)(size*64), hdpi, vdpi);
void Font::SetSize(size_t size)
{
//We changed this from 72? I think in the next version of lime we should probably
//change the function signature from both the native and haxe side to expect an optional
//dpi argument
size_t hdpi = 96;
size_t vdpi = 96;

FT_Set_Char_Size(
(FT_Face)face, //Handle to the target face object
0, //Char width in 1/64th of points (0 means same as height)
static_cast<int>(size * 64), //Char height in 1/64th of points
hdpi, //Horizontal DPI
vdpi //Vertical DPI
);
mSize = size;

}


Expand Down

11 comments on commit 9cbdc83

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

@dimensionscape I'm seeing that this commit is causing text in OpenFL to render with large extra spacing between letters on native targets. I think that the dpi values probably need to stay at 72.

@dimensionscape
Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I didnt think there was anything else that called this other than renderGlyph, which isnt used by textfield at all, but after searching for the private method individually I did notice https://github.com/openfl/openfl/blob/b2a5aa17422962a206f1110f7df26b883e244e9f/src/openfl/text/_internal/TextLayout.hx#L123

I will go ahead and change the internal signature of the function to accept an argument to set the dpi and then make a change in TextLayout to specify 72.

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

It should still default to 72, though, because existing released versions of OpenFL can't be modified to pass an argument.

@dimensionscape
Copy link
Member Author

Choose a reason for hiding this comment

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

It should still default to 72, though, because existing released versions of OpenFL can't be modified to pass an argument.

Yes, we'll just pass 72 in Textlayout

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe you misunderstood me. We can't modify TextLayout in OpenFL 9.2 or 9.3 to pass 72, so you are proposing a breaking change.

@dimensionscape
Copy link
Member Author

Choose a reason for hiding this comment

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

9.3 is already broken with the latest version of Lime because of filesystem and BackgroundWorker changes, for the record.

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

Oh great. More things to fix!

@dimensionscape
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh great. More things to fix!

Well, when we reverted BackgroundWorker from the typedef to ThreadPool, I suppose ThreadPool wasn't entirely backwards compatible. I forget exactly why but I couldn't compile with the latest version of Lime.

In any case. Any problems with this: 2f99776 ?

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

In any case. Any problems with this: 2f99776 ?

That's probably fine.

@joshtynjala
Copy link
Member

Choose a reason for hiding this comment

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

Well, when we reverted BackgroundWorker from the typedef to ThreadPool, I suppose ThreadPool wasn't entirely backwards compatible. I forget exactly why but I couldn't compile with the latest version of Lime.

Just for completeness, in case anyone references this conversation in the future, I'm copying the error message from using OpenFL 9.3 with Lime 8.2 here:

openfl/src/openfl/filesystem/FileStream.hx:227: characters 58-66 : lime.system.ThreadPool has no field canceled (Suggestion: cancel)

We had preemptively used #if (lime >= "8.2.0") to use ThreadPool instead of BackgroundWorker. Probably to avoid the deprecated compiler warning. As you said, when we reverted BackgroundWorker to its original state, the released versions of OpenFL were still expecting the new version of BackgroundWorker, and that's why there is now an error.

(I see that I was the one who made that change: openfl/openfl@3191f4f)

In this case, we can't do much to fix it. Unless we want to release an OpenFL 9.3.5 update without that conditional compilation. I'd rather not spend time on that, though, unless someone really needs it, for some reason.

Post-mortem: In the future, I guess we should try to avoid using conditional compilation that references unreleased versions of Lime in released versions of OpenFL, so that this doesn't happen again.

@player-03
Copy link
Contributor

Choose a reason for hiding this comment

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

Post-mortem: In the future, I guess we should try to avoid using conditional compilation that references unreleased versions of Lime in released versions of OpenFL, so that this doesn't happen again.

Or at the very least, keep a list of them so we know what to check when reverting. Mentioning the PR in the commit will automatically add such a note.

And as for me personally, I'll have to be a bit less aggressive with the deprecation warnings. Start with a simple comment, then move on to @:deprecated in a later version, once the new feature is locked in.

Please sign in to comment.