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

Added letterSpacing variable to FlxText. #3027

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Added letterSpacing variable to FlxText. #3027

merged 5 commits into from
Feb 9, 2024

Conversation

MAJigsaw77
Copy link
Contributor

@MAJigsaw77 MAJigsaw77 commented Feb 8, 2024

Adds access to openfl's letterSpacing in FlxText instances.

related issue, letterSpacing doesn't work on html5 - openfl/openfl#1700

@MAJigsaw77 MAJigsaw77 changed the title Added spacing variable to FlxText. Added letterSpacing variable to FlxText. Feb 8, 2024
@moxie-coder
Copy link
Contributor

nice

Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

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

See below for requested changes and provide a small use case

*
* The default value is `null`, which means that 0 pixels of letter spacing is used.
*/
public var letterSpacing(default, set):Null<Float> = null;
Copy link
Member

@Geokureli Geokureli Feb 9, 2024

Choose a reason for hiding this comment

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

  1. (default, set) will cause issues, since set_ letterSpacing only sets _defaultFormat.letterSpacing. meaning the following will happen:
text.letterSpacing = 1;
trace(text.letterSpacing); // null

We should add a getter that returns _defaultFormat.letterSpacing

  1. looks like the other fields avoid using Null and just use primitive types, I think Null should be avoided if possible. let's use Float and set the default value to 0, and also set the _defaultFormats initial letterSpacing to 0

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 letterSpace is null by default in openfl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That value cannot be getted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@Geokureli Geokureli Feb 9, 2024

Choose a reason for hiding this comment

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

That value cannot be getted

Sure it can

The letterSpace is null by default in openfl

I understand, that's why i said "set the _defaultFormats initial letterSpacing to 0", specifically, here

@Geokureli Geokureli added this to the 5.7.0 milestone Feb 9, 2024
@Geokureli
Copy link
Member

Geokureli commented Feb 9, 2024

also this is gonna wait until after the 5.6.1 release, which should happen soon

Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

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

To reiterate:

set the _defaultFormats initial letterSpacing to 0
specifically here

Also:

provide a small use case

@MAJigsaw77
Copy link
Contributor Author

Usage example:

import flixel.text.FlxText;

var text:FlxText = new FlxText(0, 0, 0, "Hello World", 64);
text.letterSpacing = 2.5;
text.screenCenter();
add(text);

@Geokureli
Copy link
Member

All good, I'll merge this after I get 5.6.1 released

@Geokureli
Copy link
Member

Geokureli commented Feb 9, 2024

When i test the following code:

var text1 = new FlxText(0, 0, 0, "Hello World", 64);
text1.screenCenter();
text1.y -= text1.height / 2;
add(text1);

var text2 = new FlxText(0, 0, 0, "Hello World", 64);
text2.letterSpacing = 10;
text2.screenCenter();
text2.y += text2.height / 2;
add(text2);

I get this result, that seems like 2 identical results, despite different values for letterSpacing
Screenshot 2024-02-09 at 1 53 18 PM

This is an openfl issue on html5, I linked the issue in the description

@Geokureli Geokureli merged commit 399a9a6 into HaxeFlixel:dev Feb 9, 2024
16 checks passed
Geokureli added a commit that referenced this pull request Feb 10, 2024
* Added `letterSpacing` variable to `FlxText`. (#3027)

* Update FlxText.hx

* `spacing` -> `letterSpacing`.

* Oops

* Update FlxText.hx

* Update FlxText.hx

* Fix negative animation frame number (#3028)

* fix negative anim frame number sorting

* use int abs

* warn when sorting frames with invalid names

* remove typo

---------

Co-authored-by: George FunBook <gkurelic@gmail.com>

* downgrade warnings and prevent more crashes

* fix error in xml attribute check

* pretty up unit test

---------

Co-authored-by: Mihai Alexandru <77043862+MAJigsaw77@users.noreply.github.com>
Co-authored-by: Timur <T1mL3arn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants