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

Char spacing #4642

Merged
merged 26 commits into from May 25, 2018
Merged

Char spacing #4642

merged 26 commits into from May 25, 2018

Conversation

ceco-fmedia
Copy link
Contributor

Added additional property letterSpacing in BitmapText to set extra space between each character.
Set textWidth/lineWidth to include the additional space at the end of the last character as it includes the space before the first one.


set letterSpacing(value) // eslint-disable-line require-jsdoc
{
if (this._maxWidth !== value)
Copy link
Member

Choose a reason for hiding this comment

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

should be this._letterSpacing

/**
* Letter spacing. This is useful for setting the space between characters.
* @member {number}
* @public
Copy link
Member

Choose a reason for hiding this comment

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

this isn't public. The getter is what's public

@themoonrat
Copy link
Member

themoonrat commented Jan 28, 2018

Testing it with the following code, and comparing current and your version of bitmap text... there appear to be issues with center and right aligning when letter spacing is used in conjunction with maxWidth.

const config = {
  width: 960,
  height: 540,
  resolution: 1,
  autoResize: true,
  transparent: true
};

const app = new PIXI.Application( config );
document.body.appendChild(app.view);

const graphics = new PIXI.Graphics();
graphics.beginFill(0xFF700B, 1);
graphics.drawRect(0, 0, 300, 300);
graphics.position.set(100, 100);
app.stage.addChild(graphics);
window.graphics = graphics;

PIXI.loader
	.add('desyrel', 'img/desyrel.xml')
	.load(onAssetsLoaded);

function onAssetsLoaded() {
	var text = new PIXI.extras.BitmapText("Now this is a story all about how my life got flipped-turned upside down.", { font: '35px Desyrel', align: 'right' });

	
	text.position.set(100, 100);
	text.letterSpacing = 40;
	text.maxWidth = 300;

	app.stage.addChild(text);
	window.text = text;
}

Pure pixi, so letterSpacing now applied: right aligns correctly within maxWidth
pure

PR version of pixi: letterSpacing in effect; right aligns incorrectly.
pr

@cursedcoder
Copy link
Member

Please consider to add some unit tests for this feature.

@ceco-fmedia
Copy link
Contributor Author

Can anyone help me find out why the TravisCI is failing?

@ivanpopelyshev
Copy link
Collaborator

There's something wrong with Travis

@ceco-fmedia
Copy link
Contributor Author

@themoonrat the problem with the center and right aligning was because the BitmapText supported only space as line breaking character and the line flipped-turned was too big. All lines are currently aligned to the biggest line, not the maxWidth property.

@bigtimebuddy
Copy link
Member

@themoonrat could you review this again?

@bigtimebuddy bigtimebuddy added this to the v4.8.0 milestone Mar 8, 2018
@themoonrat
Copy link
Member

@bigtimebuddy will do. Code lgtm ... just wanna have a little play to try and see if I can break things :)

@bigtimebuddy
Copy link
Member

Sure, no problem. Fiddles welcomed

@themoonrat
Copy link
Member

themoonrat commented Mar 9, 2018

So, the letter spacing itself I'm happy with.
A couple of other things.
The first is the change in how you treat hyphens. Previously they would be kept together as a word, but now they are allowed as a break for word wrapping. Are there any other expected changes to how bitmap text will look with this PR even with letterSpacing 0? I'm not sure if this is important or not @bigtimebuddy ?

For example

Current Pixi:
hyphen-before
PR Pixi:
hyphen-after

Second: And this bug isn't unique to your code, but also existed in the existing code.... it doesn't take into account line breaks added into strings already.

const config = {
  width: 960,
  height: 540,
  resolution: 1,
  autoResize: true,
  transparent: true
};

const app = new PIXI.Application( config );
document.body.appendChild(app.view);

const graphics = new PIXI.Graphics();
graphics.beginFill(0xFF700B, 1);
graphics.drawRect(0, 0, 300, 300);
graphics.position.set(100, 100);
app.stage.addChild(graphics);
window.graphics = graphics;

PIXI.loader
	.add('desyrel', 'img/desyrel.xml')
	.load(onAssetsLoaded);

function onAssetsLoaded() {
	var text = new PIXI.extras.BitmapText("One\ntwo\nthree\nfour five six seven eight nine ten", { font: '35px Desyrel', align: 'right' });
	
	text.position.set(100, 100);
	text.letterSpacing = 0;
	text.maxWidth = 300;

	app.stage.addChild(text);
	window.text = text;
}

Produces
line-breaks

Notice how it repeats the 'ei' on line 4... the first 2 letters of eight. It doesn't distinguish between line breaks already within the text, and line breaks created internally due to word wrap.

I don't know whether it's worth fixing in this PR, or put this PR through as it matches old behaviour and ask for a new PR later on to fix this? Thoughts again @bigtimebuddy ?

@bigtimebuddy
Copy link
Member

  1. We should match existing behavior with regards to hyphens.
  2. Line-breaks can be another PR, I'd rather not pile on this PR

@bigtimebuddy
Copy link
Member

@ceco-fmedia and @themoonrat where are we with this PR? Would like to get into 4.8 if we can. Please let me know. Also @ceco-fmedia could you resolve conflicts?

@bigtimebuddy bigtimebuddy removed this from the v4.8.0 milestone May 24, 2018
@bigtimebuddy bigtimebuddy dismissed themoonrat’s stale review May 25, 2018 13:49

Issues have been addressed above

@bigtimebuddy bigtimebuddy merged commit 3ad703a into pixijs:dev May 25, 2018
@ceco-fmedia ceco-fmedia deleted the char_spacing branch October 18, 2018 11:02
@lock
Copy link

lock bot commented Oct 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💾 v4.x (Legacy) Legacy version 4 support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants