add text component and a-text primitive to core #2200

Merged
merged 5 commits into from Jan 27, 2017

Conversation

Projects
None yet
6 participants
@machenmusik
Collaborator

machenmusik commented Dec 21, 2016

add bmfont-text component and a-text primitive to core from https://github.com/chenzlabs/aframe-bmfont-component

@dmarcos

This comment has been minimized.

Show comment
Hide comment
@dmarcos

dmarcos Dec 21, 2016

Collaborator

This is a modification of @fernandojsg component and there's a PR opened there(https://github.com/fernandojsg/aframe-bmfont-component/pull/1). I'll wait for fernando's review first.

Collaborator

dmarcos commented Dec 21, 2016

This is a modification of @fernandojsg component and there's a PR opened there(https://github.com/fernandojsg/aframe-bmfont-component/pull/1). I'll wait for fernando's review first.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Dec 21, 2016

Collaborator

yep, makes sense, the PR to @fernandojsg 's version is mine and incorporated here

Collaborator

machenmusik commented Dec 21, 2016

yep, makes sense, the PR to @fernandojsg 's version is mine and incorporated here

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Dec 22, 2016

Collaborator

I have merged the output from @fernandojsg component PR review into this PR.

Collaborator

machenmusik commented Dec 22, 2016

I have merged the output from @fernandojsg component PR review into this PR.

src/components/bmfont-text.js
+
+ init: function () {
+ this.texture = new THREE.Texture();
+ this.texture.anisotropy = 16; // ??

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

What are the question marks?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

What are the question marks?

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we add a better comment?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we add a better comment?

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

I don't know, those were in the original. i can remove them, but i don't have a better comment -- @fernandojsg any ideas?

@machenmusik

machenmusik Dec 22, 2016

Collaborator

I don't know, those were in the original. i can remove them, but i don't have a better comment -- @fernandojsg any ideas?

This comment has been minimized.

@bryik

bryik Dec 23, 2016

Member

If it helps, I set anisotropy to 16 in my component because I think it improves the look of large amounts of text particularly when viewed from an angle. The question marks probably relate to a comment I had about setting anisotropy with renderer.getMaxAnisotropy(); I didn't know how to do this in A-Frame so I just left it at 16 (the maximum level for my GPU)

@bryik

bryik Dec 23, 2016

Member

If it helps, I set anisotropy to 16 in my component because I think it improves the look of large amounts of text particularly when viewed from an angle. The question marks probably relate to a comment I had about setting anisotropy with renderer.getMaxAnisotropy(); I didn't know how to do this in A-Frame so I just left it at 16 (the maximum level for my GPU)

This comment has been minimized.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

Can we create a constant at the top and explain why 16?

@dmarcos

dmarcos Dec 23, 2016

Collaborator

Can we create a constant at the top and explain why 16?

src/components/bmfont-text.js
+ },
+
+ coerceData: function (data) {
+ // We have to coerce some data to numbers/booleans

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we explain why?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we explain why?

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

I don't know, those were in the original, hadn't looked

@machenmusik

machenmusik Dec 22, 2016

Collaborator

I don't know, those were in the original, hadn't looked

This comment has been minimized.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

@fernandojsg Why do we need to coerce?

@dmarcos

dmarcos Dec 23, 2016

Collaborator

@fernandojsg Why do we need to coerce?

This comment has been minimized.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

looking at this slightly more, I think the data needs to be coerced because it goes right into the text creation in the options merge.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

looking at this slightly more, I think the data needs to be coerced because it goes right into the text creation in the options merge.

This comment has been minimized.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

(added a comment but that didn't make this one outdated for some reason)

@machenmusik

machenmusik Dec 24, 2016

Collaborator

(added a comment but that didn't make this one outdated for some reason)

src/components/bmfont-text.js
+ coerceData: function (data) {
+ // We have to coerce some data to numbers/booleans
+ data = assign({}, data);
+ if (typeof data.lineHeight !== 'undefined') {

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I prefer to do data.lineHeigt === undefined

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I prefer to do data.lineHeigt === undefined

src/components/bmfont-text.js
+ data.lineHeight = parseFloat(data.lineHeight);
+ if (!isFinite(data.lineHeight)) { data.lineHeight = undefined; }
+ }
+ if (typeof data.width !== 'undefined') {

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I prefer to do data.width === undefined it's more compact and consistent with the rest of the code base.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I prefer to do data.width === undefined it's more compact and consistent with the rest of the code base.

src/components/bmfont-text.js
+ this.material.uniforms.map.value = this.texture;
+ }
+
+ if (this.mesh) {

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

one liner

@dmarcos

dmarcos Dec 22, 2016

Collaborator

one liner

src/components/bmfont-text.js
+ case 'front': return THREE.FrontSide;
+ case 'back': return THREE.BackSide;
+ default:
+ throw new TypeError('unknown side string ' + str);

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Capitalize first letter of the error message

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Capitalize first letter of the error message

src/components/bmfont-text.js
+ mode: {default: 'normal', oneOf: ['normal', 'pre', 'nowrap']},
+ color: {type: 'color', default: '#000'},
+ opacity: {type: 'number', default: '1.0'},
+ type: {default: 'SDF', oneOf: ['SDF', 'basic', 'MSDF']},

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I would rename this to shader

@dmarcos

dmarcos Dec 22, 2016

Collaborator

I would rename this to shader

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure
but here I think the intent was to note the type of font (basic, signed distance field, multichannel signed distance field) so actually type is more appropriate than directly referencing shader IMO

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure
but here I think the intent was to note the type of font (basic, signed distance field, multichannel signed distance field) so actually type is more appropriate than directly referencing shader IMO

This comment has been minimized.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

We don't care about compatibility with prior components. type is not specific enough besides it's also used in the schemas with a different meaning.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

We don't care about compatibility with prior components. type is not specific enough besides it's also used in the schemas with a different meaning.

src/components/bmfont-text.js
+ align: {type: 'string', default: 'left', oneOf: alignments},
+ letterSpacing: {type: 'number', default: 0},
+ lineHeight: {type: 'number'}, // default to font's lineHeight value
+ fnt: {type: 'string', default: 'default'},

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we call it font?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Can we call it font?

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure

This comment has been minimized.

@machenmusik

machenmusik Dec 23, 2016

Collaborator

hmm, there is already a field font...

@machenmusik

machenmusik Dec 23, 2016

Collaborator

hmm, there is already a field font...

This comment has been minimized.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

I don't see the field data.font used anywhere.

@dmarcos

dmarcos Dec 23, 2016

Collaborator

I don't see the field data.font used anywhere.

This comment has been minimized.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

ok, let's do it :-)

@machenmusik

machenmusik Dec 24, 2016

Collaborator

ok, let's do it :-)

src/components/bmfont-text.js
+ letterSpacing: {type: 'number', default: 0},
+ lineHeight: {type: 'number'}, // default to font's lineHeight value
+ fnt: {type: 'string', default: 'default'},
+ fntImage: {type: 'string'}, // default to fnt but with .fnt replaced by .png

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Cane we call it fontImage?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

Cane we call it fontImage?

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure

This comment has been minimized.

@machenmusik

machenmusik Dec 23, 2016

Collaborator

shouldn't rename until we figure out fnt vs. font

@machenmusik

machenmusik Dec 23, 2016

Collaborator

shouldn't rename until we figure out fnt vs. font

src/components/bmfont-text.js
+ lineHeight: {type: 'number'}, // default to font's lineHeight value
+ fnt: {type: 'string', default: 'default'},
+ fntImage: {type: 'string'}, // default to fnt but with .fnt replaced by .png
+ mode: {default: 'normal', oneOf: ['normal', 'pre', 'nowrap']},

This comment has been minimized.

@dmarcos

dmarcos Dec 22, 2016

Collaborator

mode is a bit ambiguous. Can we come up with a better name?

@dmarcos

dmarcos Dec 22, 2016

Collaborator

mode is a bit ambiguous. Can we come up with a better name?

This comment has been minimized.

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure
open to suggestions, i don't have a better one at the moment

@machenmusik

machenmusik Dec 22, 2016

Collaborator

if we are not worried about compatibility with the prior component, sure
open to suggestions, i don't have a better one at the moment

This comment has been minimized.

@donmccurdy

donmccurdy Dec 23, 2016

Member

IMO, compatibility with prior component is of no importance. Consistency with CSS conventions is better. I'd call this property whiteSpace, from css spec.

@donmccurdy

donmccurdy Dec 23, 2016

Member

IMO, compatibility with prior component is of no importance. Consistency with CSS conventions is better. I'd call this property whiteSpace, from css spec.

This comment has been minimized.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

great suggestion, thanks!

@machenmusik

machenmusik Dec 24, 2016

Collaborator

great suggestion, thanks!

@fernandojsg

This comment has been minimized.

Show comment
Hide comment
@fernandojsg

fernandojsg Dec 22, 2016

Member

Maybe we could move the discussion to my repo PR and once we got that fixed and working update it? Or just the other way but to avoid commenting in both places

Member

fernandojsg commented Dec 22, 2016

Maybe we could move the discussion to my repo PR and once we got that fixed and working update it? Or just the other way but to avoid commenting in both places

@fernandojsg

This comment has been minimized.

Show comment
Hide comment
@fernandojsg

fernandojsg Dec 22, 2016

Member

In fact the component wasnt yet ready to ship it on aframe.
First we need to fix the functionality for example msdf doesnt work yet but its exposed anyway. Then we should refactor and lint the code.
And finally add some cool examples.

Member

fernandojsg commented Dec 22, 2016

In fact the component wasnt yet ready to ship it on aframe.
First we need to fix the functionality for example msdf doesnt work yet but its exposed anyway. Then we should refactor and lint the code.
And finally add some cool examples.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Dec 23, 2016

Collaborator

IMO, it's better to work issues here where we don't necessarily have to concern ourselves with compatibility with prior uses of the actual component.

Sidebar, it is a nuisance that you can't use the same source between standalone and proposed core... which would make this sort of thing much easier if working components could literally be pulled into core

Collaborator

machenmusik commented Dec 23, 2016

IMO, it's better to work issues here where we don't necessarily have to concern ourselves with compatibility with prior uses of the actual component.

Sidebar, it is a nuisance that you can't use the same source between standalone and proposed core... which would make this sort of thing much easier if working components could literally be pulled into core

@@ -0,0 +1,31 @@
+/* Experimental text primitive.
+ * Issues: color not changing, removeAttribute() not working, mixing primitive with regular entities fails
+ * Color issue relates to: https://github.com/donmccurdy/aframe-extras/blob/master/src/primitives/a-ocean.js#L44

This comment has been minimized.

@donmccurdy

donmccurdy Dec 23, 2016

Member

Do we know what is meant by "mixing primitive with regular entities"?

@donmccurdy

donmccurdy Dec 23, 2016

Member

Do we know what is meant by "mixing primitive with regular entities"?

This comment has been minimized.

@donmccurdy

donmccurdy Dec 23, 2016

Member

And, the issue mentioned in <a-ocean /> should be fixed as of A-Frame v0.4.0.

@donmccurdy

donmccurdy Dec 23, 2016

Member

And, the issue mentioned in <a-ocean /> should be fixed as of A-Frame v0.4.0.

This comment has been minimized.

@machenmusik

machenmusik Dec 23, 2016

Collaborator

not sure, but i would assume that means something about problems using a-text primitive with other components in same entity.

@machenmusik

machenmusik Dec 23, 2016

Collaborator

not sure, but i would assume that means something about problems using a-text primitive with other components in same entity.

This comment has been minimized.

@bryik

bryik Dec 23, 2016

Member

The problems with the text primitive do seem to have been fixed with A-Frame v0.4.0.

@donmccurdy with 0.3.0 adding a text primitive to a scene would sometimes break text added the regular way (e.g. <a-entity bmfont-text="text: Hello World;"></a-entity> would not show up if there was a text primitive in the scene)

@bryik

bryik Dec 23, 2016

Member

The problems with the text primitive do seem to have been fixed with A-Frame v0.4.0.

@donmccurdy with 0.3.0 adding a text primitive to a scene would sometimes break text added the regular way (e.g. <a-entity bmfont-text="text: Hello World;"></a-entity> would not show up if there was a text primitive in the scene)

This comment has been minimized.

@machenmusik

machenmusik Dec 24, 2016

Collaborator

so are any of these issues still valid, or should the comment be removed?

@machenmusik

machenmusik Dec 24, 2016

Collaborator

so are any of these issues still valid, or should the comment be removed?

This comment has been minimized.

@bryik

bryik Dec 28, 2016

Member

I wrote those comments for my variant of bmfont-text with A-Frame v0.3.0, so I can't say for sure if they're valid for Fernando's bmfont-text with A-Frame v0.4.0. My guess would be no.

@bryik

bryik Dec 28, 2016

Member

I wrote those comments for my variant of bmfont-text with A-Frame v0.3.0, so I can't say for sure if they're valid for Fernando's bmfont-text with A-Frame v0.4.0. My guess would be no.

This comment has been minimized.

@dmarcos

dmarcos Dec 31, 2016

Collaborator

Can we check if these comments are still relevant?

@dmarcos

dmarcos Dec 31, 2016

Collaborator

Can we check if these comments are still relevant?

This comment has been minimized.

@machenmusik

machenmusik Dec 31, 2016

Collaborator

(removed)

@machenmusik

machenmusik Dec 31, 2016

Collaborator

(removed)

@dmarcos

This comment has been minimized.

Show comment
Hide comment
@dmarcos

dmarcos Dec 23, 2016

Collaborator

Sidebar, it is a nuisance that you can't use the same source between standalone and proposed core... which would make this sort of thing much easier if working components could literally be pulled into core
@machenmusik What does it mean working components? @fernandojsg won't maintain his component anymore and will be now part of the core. The registry paired with the inspector is the way to easily consume / discover components that are not part of the core while separating concerns and liabilities. I want to reduce what gets into core to what we consider is minimal functionality and has to be consistent across a-frame scenes. If functionality keeps growing we won't be able to keep good quality.

Collaborator

dmarcos commented Dec 23, 2016

Sidebar, it is a nuisance that you can't use the same source between standalone and proposed core... which would make this sort of thing much easier if working components could literally be pulled into core
@machenmusik What does it mean working components? @fernandojsg won't maintain his component anymore and will be now part of the core. The registry paired with the inspector is the way to easily consume / discover components that are not part of the core while separating concerns and liabilities. I want to reduce what gets into core to what we consider is minimal functionality and has to be consistent across a-frame scenes. If functionality keeps growing we won't be able to keep good quality.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Dec 24, 2016

Collaborator

@machenmusik What does it mean working components?

when you're developing a component, you typically start with it being standalone, and only later does it potentially merit proposal for inclusion into core. when such proposals are done, it is a shame that there is a bunch of manual editing required to go from standalone component to core PR... e.g. rename component index.js to component.js, relative require statements as opposed to global AFRAME, to THREE or not to THREE :-), component boilerplate doesn't understand docs or tests, etc.

Collaborator

machenmusik commented Dec 24, 2016

@machenmusik What does it mean working components?

when you're developing a component, you typically start with it being standalone, and only later does it potentially merit proposal for inclusion into core. when such proposals are done, it is a shame that there is a bunch of manual editing required to go from standalone component to core PR... e.g. rename component index.js to component.js, relative require statements as opposed to global AFRAME, to THREE or not to THREE :-), component boilerplate doesn't understand docs or tests, etc.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Dec 31, 2016

Collaborator

squashed the PR edits here.
what else is needed?

Collaborator

machenmusik commented Dec 31, 2016

squashed the PR edits here.
what else is needed?

src/components/bmfont-text.js
+// of large amounts of text particularly when viewed from an angle.
+var MAX_ANISOTROPY = 16;
+
+var FONT_BASE_URL = 'https://cdn.rawgit.com/fernandojsg/aframe-bmfont-component/5c88edf40bbc88b336d7db6f040c3ae0d2f65aff/fonts/';

This comment has been minimized.

@dmarcos

dmarcos Jan 18, 2017

Collaborator

Should we move the fonts to the assets repo? https://github.com/aframevr/assets

@dmarcos

dmarcos Jan 18, 2017

Collaborator

Should we move the fonts to the assets repo? https://github.com/aframevr/assets

This comment has been minimized.

@machenmusik

machenmusik Jan 18, 2017

Collaborator

probably, yes

@machenmusik

machenmusik Jan 18, 2017

Collaborator

probably, yes

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 18, 2017

Collaborator

i made a crude attempt to cache fnt/png requests in a separate branch, is it better to get baseline in then do later, or try to do all at once?

Collaborator

machenmusik commented Jan 18, 2017

i made a crude attempt to cache fnt/png requests in a separate branch, is it better to get baseline in then do later, or try to do all at once?

@dmarcos

This comment has been minimized.

Show comment
Hide comment
@dmarcos

dmarcos Jan 19, 2017

Collaborator

@ngokevin @fernandojsg Should we pull the trigger and merge this?

Collaborator

dmarcos commented Jan 19, 2017

@ngokevin @fernandojsg Should we pull the trigger and merge this?

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 19, 2017

Member

Can the component name be shortened? text might be too prescriptive, what about bmtext or sdftext?

Member

ngokevin commented Jan 19, 2017

Can the component name be shortened? text might be too prescriptive, what about bmtext or sdftext?

@fernandojsg

This comment has been minimized.

Show comment
Hide comment
@fernandojsg

fernandojsg Jan 19, 2017

Member

@dmarcos let me have a look at it, I didn't check the last updates for a while, sorry :)
@ngokevin umm as we could have several techniques I wouldn't choose sdftext, being a part of the core maybe text could be enough? or text2d or something non related to the technique used to render it

Member

fernandojsg commented Jan 19, 2017

@dmarcos let me have a look at it, I didn't check the last updates for a while, sorry :)
@ngokevin umm as we could have several techniques I wouldn't choose sdftext, being a part of the core maybe text could be enough? or text2d or something non related to the technique used to render it

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 19, 2017

Member

I wouldn't mind text too much, we'd document that there are other methods. And it seems cleaner.

I haven't had time to take a look either. Will get around, but if I don't, I can always post-review.

Member

ngokevin commented Jan 19, 2017

I wouldn't mind text too much, we'd document that there are other methods. And it seems cleaner.

I haven't had time to take a look either. Will get around, but if I don't, I can always post-review.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

could use some more eyes on the new custom shader (avoid horrible aliasing when text gets too small)

Collaborator

machenmusik commented Jan 19, 2017

could use some more eyes on the new custom shader (avoid horrible aliasing when text gets too small)

@ngokevin

Needs some baseline unit tests

examples/primitives/text/index.html
@@ -0,0 +1,100 @@
+<html>

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

This could be a examples/test/text/ example.

@ngokevin

ngokevin Jan 19, 2017

Member

This could be a examples/test/text/ example.

src/components/bmfont-text.js
+
+var FONT_BASE_URL = 'https://cdn.aframe.io/fonts/';
+var fontMap = {
+ 'default': FONT_BASE_URL + 'DejaVu-sdf.fnt',

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

Could make these font names all lower case with no dashes to make it less error prone. Then could alphabetize them here

@ngokevin

ngokevin Jan 19, 2017

Member

Could make these font names all lower case with no dashes to make it less error prone. Then could alphabetize them here

This comment has been minimized.

@machenmusik

machenmusik Jan 19, 2017

Collaborator

(addressed, but review didn't pick up)

@machenmusik

machenmusik Jan 19, 2017

Collaborator

(addressed, but review didn't pick up)

src/components/bmfont-text.js
+var loadedFontPromises = {};
+var loadedTexturePromises = {};
+
+coreShader.registerShader('modified-SDF', {

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

nit: could be modified-sdf to not be case sensitive, could add whitelines between the properties of the shader

@ngokevin

ngokevin Jan 19, 2017

Member

nit: could be modified-sdf to not be case sensitive, could add whitelines between the properties of the shader

This comment has been minimized.

@dmarcos

dmarcos Jan 19, 2017

Collaborator

can we name if just SDF? What is the modification?

@dmarcos

dmarcos Jan 19, 2017

Collaborator

can we name if just SDF? What is the modification?

src/components/bmfont-text.js
+
+coreShader.registerShader('modified-SDF', {
+ schema: {
+ map: { type: 'map', is: 'uniform' },

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

no spaces inside object braces

@ngokevin

ngokevin Jan 19, 2017

Member

no spaces inside object braces

src/components/bmfont-text.js
+module.exports.Component = registerComponent('bmfont-text', {
+ schema: {
+ // scale is now determined by width and wrappixels/wrapcount... scale: {default: 0.003},
+ tabSize: {default: 4},

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

can we alphabetize this prop list

@ngokevin

ngokevin Jan 19, 2017

Member

can we alphabetize this prop list

src/components/bmfont-text.js
+ },
+
+ updateLayout: function (data) {
+ var el = this.el;

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

Lots of variables initialized up-front, possible to declare them first and initialize them closer to where they're used?

Alternative initialize + declare them where they're used and use const

@ngokevin

ngokevin Jan 19, 2017

Member

Lots of variables initialized up-front, possible to declare them first and initialize them closer to where they're used?

Alternative initialize + declare them where they're used and use const

This comment has been minimized.

@machenmusik

machenmusik Jan 19, 2017

Collaborator

I think we don't want to use const because we aren't assuming ES6, will see what I can do

@machenmusik

machenmusik Jan 19, 2017

Collaborator

I think we don't want to use const because we aren't assuming ES6, will see what I can do

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

Yeah maybe not for this case for core http://caniuse.com/#search=const

I usually use it in components that are VR-specific though since we have the benefit of targeting experimental browsers

@ngokevin

ngokevin Jan 19, 2017

Member

Yeah maybe not for this case for core http://caniuse.com/#search=const

I usually use it in components that are VR-specific though since we have the benefit of targeting experimental browsers

This comment has been minimized.

@machenmusik

machenmusik Jan 19, 2017

Collaborator

moved stuff around, should be better...

@machenmusik

machenmusik Jan 19, 2017

Collaborator

moved stuff around, should be better...

src/components/bmfont-text.js
+
+function threeSideFromString (str) {
+ switch (str) {
+ case 'double': return THREE.DoubleSide;

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

can you brace the cases?

case 'double': { return THREE.DoubleSide; }

@ngokevin

ngokevin Jan 19, 2017

Member

can you brace the cases?

case 'double': { return THREE.DoubleSide; }

+ 'bmfont-text': {anchor: 'align', width: 5}
+ },
+ mappings: {
+ text: 'bmfont-text.text',

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

can alphabetize

@ngokevin

ngokevin Jan 19, 2017

Member

can alphabetize

@@ -0,0 +1,29 @@
+/* Experimental text primitive.
+ */
+

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

empty line

@ngokevin

ngokevin Jan 19, 2017

Member

empty line

+/* Experimental text primitive.
+ */
+
+var getMeshMixin = require('../getMeshMixin');

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

is the mesh mixin needed?

@ngokevin

ngokevin Jan 19, 2017

Member

is the mesh mixin needed?

This comment has been minimized.

@machenmusik

machenmusik Jan 19, 2017

Collaborator

good question. was in inherited code, and I believe it's in the A-Frame registerPrimitive sample...

@machenmusik

machenmusik Jan 19, 2017

Collaborator

good question. was in inherited code, and I believe it's in the A-Frame registerPrimitive sample...

This comment has been minimized.

@ngokevin

ngokevin Jan 19, 2017

Member

Yeah, the box example uses the material component so it benefits from the mixin, but we don't need it here.

@ngokevin

ngokevin Jan 19, 2017

Member

Yeah, the box example uses the material component so it benefits from the mixin, but we don't need it here.

src/components/bmfont-text.js
+ 'uniform vec3 color;',
+ 'uniform float opacity;',
+ 'varying vec2 vUV;',
+ '#define ALL_SMOOTH 0.5',

This comment has been minimized.

@dmarcos

dmarcos Jan 19, 2017

Collaborator

can we put defines at the top?

@dmarcos

dmarcos Jan 19, 2017

Collaborator

can we put defines at the top?

src/components/bmfont-text.js
+ if (this.mesh) { this.mesh.material = this.material; }
+ return;
+ }
+ if (this.data.shader === 'SDF') {

This comment has been minimized.

@dmarcos

dmarcos Jan 19, 2017

Collaborator

should not be a case for modified-sdf here?

@dmarcos

dmarcos Jan 19, 2017

Collaborator

should not be a case for modified-sdf here?

This comment has been minimized.

@machenmusik

machenmusik Jan 19, 2017

Collaborator

modified-sdf is customShader. I couldn't figure out how to do the validation oneOf with a dynamic shader list and pre-existing values at the same time, or else I would have done as shader

@machenmusik

machenmusik Jan 19, 2017

Collaborator

modified-sdf is customShader. I couldn't figure out how to do the validation oneOf with a dynamic shader list and pre-existing values at the same time, or else I would have done as shader

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

would text be too prone to namespace collisions?

Collaborator

machenmusik commented Jan 19, 2017

would text be too prone to namespace collisions?

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

Could definitely use help on tests, another case of inheriting something with none... and often visual verification is important...

Collaborator

machenmusik commented Jan 19, 2017

Could definitely use help on tests, another case of inheriting something with none... and often visual verification is important...

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 19, 2017

Member

Namespace collisions not too much of an issue, Registry components aren't allowed to register names used by core components. I'll just rename mine to text-geometry (or make it part of the geometry component).

I can help with tests in a bit. Thanks!

Member

ngokevin commented Jan 19, 2017

Namespace collisions not too much of an issue, Registry components aren't allowed to register names used by core components. I'll just rename mine to text-geometry (or make it part of the geometry component).

I can help with tests in a bit. Thanks!

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

if text won't cause collision issues great. it does have a property called text though, so that might be a little strange. did you want me to change the component name, or were you going to take care of that?

Collaborator

machenmusik commented Jan 19, 2017

if text won't cause collision issues great. it does have a property called text though, so that might be a little strange. did you want me to change the component name, or were you going to take care of that?

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 19, 2017

Member

I think text.text is fine. You can change, or I can get to it later.

Member

ngokevin commented Jan 19, 2017

I think text.text is fine. You can change, or I can get to it later.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

gonna be a few hours at minimum on my end, if you have the cycles go ahead

Collaborator

machenmusik commented Jan 19, 2017

gonna be a few hours at minimum on my end, if you have the cycles go ahead

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 19, 2017

Collaborator

if you folks have better suggestions than using gl_FragCoord.w as a guess of when to drop the alpha test (which makes the distant appearance even worse) and blend, let me know. there are still some cases like extreme off-axis viewing that the custom shader doesn't do much for...

Collaborator

machenmusik commented Jan 19, 2017

if you folks have better suggestions than using gl_FragCoord.w as a guess of when to drop the alpha test (which makes the distant appearance even worse) and blend, let me know. there are still some cases like extreme off-axis viewing that the custom shader doesn't do much for...

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 20, 2017

Collaborator

so I changed the name to text, and switched aframe-arcade over to it, if you'd like to have a look / test drive

Collaborator

machenmusik commented Jan 20, 2017

so I changed the name to text, and switched aframe-arcade over to it, if you'd like to have a look / test drive

src/components/text.js
+var loadedFontPromises = {};
+var loadedTexturePromises = {};
+
+coreShader.registerShader('modified-sdf', {

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

It could be better to take it out to another file like the rest of the shaders and just require it here.

@fernandojsg

fernandojsg Jan 20, 2017

Member

It could be better to take it out to another file like the rest of the shaders and just require it here.

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

yep

@dmarcos

dmarcos Jan 20, 2017

Collaborator

yep

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

we really need to call it modified-sdf?

@dmarcos

dmarcos Jan 20, 2017

Collaborator

we really need to call it modified-sdf?

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

that was literal naming scheme (it is actually modified sdf shader) feel free to suggest another name. none of the other shader choices (sdf, basic, msdf) are exposed as registered shaders, but now that we have the recent PRs, agree this one can be added

@machenmusik

machenmusik Jan 20, 2017

Collaborator

that was literal naming scheme (it is actually modified sdf shader) feel free to suggest another name. none of the other shader choices (sdf, basic, msdf) are exposed as registered shaders, but now that we have the recent PRs, agree this one can be added

src/components/text.js
+ // default to font's lineHeight value
+ lineHeight: {type: 'number'},
+ opacity: {type: 'number', default: '1.0'},
+ shader: {default: 'custom', oneOf: ['custom', 'sdf', 'basic', 'msdf']},

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

Why not including the shader to the list instead of including the new custom value and the customShader parameter? I believe user won't know the difference and probably it's not going to be that common that users creates custom shaders, and in that case they could always add them to this list.

@fernandojsg

fernandojsg Jan 20, 2017

Member

Why not including the shader to the list instead of including the new custom value and the customShader parameter? I believe user won't know the difference and probably it's not going to be that common that users creates custom shaders, and in that case they could always add them to this list.

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

Should we expose the shader in the schema at all? It looks one more knob to tweak that introduce complexity and potential confusion.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

Should we expose the shader in the schema at all? It looks one more knob to tweak that introduce complexity and potential confusion.

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

I do think there are use cases for custom shaders (e.g. this hybrid modification, or outlines, or interesting text effects), in which case they should be from the registered shader list, which is why I originally chose an explicitly verifiable "custom" setting for shader (essentially a promise you know what you are doing) and then customShader uses the existing registered shader name list

@machenmusik

machenmusik Jan 20, 2017

Collaborator

I do think there are use cases for custom shaders (e.g. this hybrid modification, or outlines, or interesting text effects), in which case they should be from the registered shader list, which is why I originally chose an explicitly verifiable "custom" setting for shader (essentially a promise you know what you are doing) and then customShader uses the existing registered shader name list

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(we could add the modified-sdf shader to the standard list, but I don't see much benefit in doing that at the moment...)

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(we could add the modified-sdf shader to the standard list, but I don't see much benefit in doing that at the moment...)

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

I would keep it simple and forgo the custom
option to starty width

@dmarcos

dmarcos Jan 20, 2017

Collaborator

I would keep it simple and forgo the custom
option to starty width

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

so then let's make modified-sdf in standard list and make default... did anyone ever figure out if basic or msdf work? if not, can strike entirely, as all the stock fonts are sdf

@machenmusik

machenmusik Jan 20, 2017

Collaborator

so then let's make modified-sdf in standard list and make default... did anyone ever figure out if basic or msdf work? if not, can strike entirely, as all the stock fonts are sdf

src/components/text.js
+ whiteSpace: {default: 'normal', oneOf: ['normal', 'pre', 'nowrap']},
+ // default to geometry width, or if not present then DEFAULT_WIDTH
+ width: {type: 'number'},
+ // units are 0.6035 * font size e.g. about one default font character (monospace DejaVu size 32)

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

That 0.6035 is an arbitrary value based on the DejaVu font? If so, we should modify that value for each font loaded isn't it?

@fernandojsg

fernandojsg Jan 20, 2017

Member

That 0.6035 is an arbitrary value based on the DejaVu font? If so, we should modify that value for each font loaded isn't it?

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

probably but not sure what the "right" value to use would be for each; as we are using variable width fonts, there isn't really an exactly correct value. was hoping that something from font metrics would be usable, but so far no dice. suggestions?

@machenmusik

machenmusik Jan 20, 2017

Collaborator

probably but not sure what the "right" value to use would be for each; as we are using variable width fonts, there isn't really an exactly correct value. was hoping that something from font metrics would be usable, but so far no dice. suggestions?

src/components/text.js
+ transparent: {default: true},
+ whiteSpace: {default: 'normal', oneOf: ['normal', 'pre', 'nowrap']},
+ // default to geometry width, or if not present then DEFAULT_WIDTH
+ width: {type: 'number'},

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

I'm not sure about having a value that could specified by the user, could have a default value (DEFAULT_WIDTH) or could be based on the geometry's width. Maybe leaving this one as:

width: {type: 'number', default: DEFAULT_WIDTH},

And create another boolean parameter use_geometry_width or so?

@fernandojsg

fernandojsg Jan 20, 2017

Member

I'm not sure about having a value that could specified by the user, could have a default value (DEFAULT_WIDTH) or could be based on the geometry's width. Maybe leaving this one as:

width: {type: 'number', default: DEFAULT_WIDTH},

And create another boolean parameter use_geometry_width or so?

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

We need docs badly for this component. I would also try to reduce the moving parts as much as possible

@dmarcos

dmarcos Jan 20, 2017

Collaborator

We need docs badly for this component. I would also try to reduce the moving parts as much as possible

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

it's the order of precedence that makes things tricky. if you use a default value in schema, you don't know when that value was explicitly specified, vs. when default was used... was bitten by that in auto-enter-vr.

cases in point:

in the case where you are adding text to a plane or other entity with geometry already, the usual thing is to default to same width (and height) but you also need to be able to specify (e.g. label a huge ground plane centered on origin without using the huge width) -- for the first case not specifying anything but text seems natural, and in second case specifying override width seems natural. thus the current implementation...

@machenmusik

machenmusik Jan 20, 2017

Collaborator

it's the order of precedence that makes things tricky. if you use a default value in schema, you don't know when that value was explicitly specified, vs. when default was used... was bitten by that in auto-enter-vr.

cases in point:

in the case where you are adding text to a plane or other entity with geometry already, the usual thing is to default to same width (and height) but you also need to be able to specify (e.g. label a huge ground plane centered on origin without using the huge width) -- for the first case not specifying anything but text seems natural, and in second case specifying override width seems natural. thus the current implementation...

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(you would want useGeometryWidth to default to true in first case, but to false in second case, which isn't possible)

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(you would want useGeometryWidth to default to true in first case, but to false in second case, which isn't possible)

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

sidebar: is there any point in allowing separate fontImage property really?

@machenmusik

machenmusik Jan 20, 2017

Collaborator

sidebar: is there any point in allowing separate fontImage property really?

src/components/text.js
+ } else if (this.currentFont) {
+ // new data like change of text string
+ var font = this.currentFont;
+ var textRenderWidth = data.wrappixels || (data.wrapcount * 0.6035 * font.info.size);

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

The same as before, this 0.6035 is arbitrary right?

@fernandojsg

fernandojsg Jan 20, 2017

Member

The same as before, this 0.6035 is arbitrary right?

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

"experimentally determined" to work well with Dejavu default font

@machenmusik

machenmusik Jan 20, 2017

Collaborator

"experimentally determined" to work well with Dejavu default font

src/components/text.js
+
+ // update geometry dimensions to match layout, if not specified
+ if (elGeo) {
+ if (!elGeo.width) { el.setAttribute('geometry', 'width', width); }

This comment has been minimized.

@fernandojsg

fernandojsg Jan 20, 2017

Member

If you have a circle geometry, it won't have a width value but this code will set that value and it won't affect that geometry.
And also this is assuming that the user always wants to modify the geometry (box, plane) to fit the text, but it can't be always true.

@fernandojsg

fernandojsg Jan 20, 2017

Member

If you have a circle geometry, it won't have a width value but this code will set that value and it won't affect that geometry.
And also this is assuming that the user always wants to modify the geometry (box, plane) to fit the text, but it can't be always true.

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

this override only happens if there isn't already a geometry width -- your point about circle is well taken, but seems harmless in that case -- so if you have a box or plane that shouldn't fit the text, it probably has a specified width (and height)

@machenmusik

machenmusik Jan 20, 2017

Collaborator

this override only happens if there isn't already a geometry width -- your point about circle is well taken, but seems harmless in that case -- so if you have a box or plane that shouldn't fit the text, it probably has a specified width (and height)

src/components/text.js
+ // default to geometry width, or if not present then DEFAULT_WIDTH
+ width: {type: 'number'},
+ // units are 0.6035 * font size e.g. about one default font character (monospace DejaVu size 32)
+ wrapcount: {type: 'number', default: 40},

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

camel case wrapCount

@dmarcos

dmarcos Jan 20, 2017

Collaborator

camel case wrapCount

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

this propery is defined twice

@dmarcos

dmarcos Jan 20, 2017

Collaborator

this propery is defined twice

This comment has been minimized.

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(ran into some issue with the primitive when it was camelcased before? will give it another try)

@machenmusik

machenmusik Jan 20, 2017

Collaborator

(ran into some issue with the primitive when it was camelcased before? will give it another try)

src/components/text.js
+ // units are 0.6035 * font size e.g. about one default font character (monospace DejaVu size 32)
+ wrapcount: {type: 'number', default: 40},
+ // if specified, units are bmfont pixels (e.g. DejaVu default is size 32)
+ wrappixels: {type: 'number'}

This comment has been minimized.

@dmarcos

dmarcos Jan 20, 2017

Collaborator

camel case: wrapPixels

@dmarcos

dmarcos Jan 20, 2017

Collaborator

camel case: wrapPixels

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 20, 2017

Collaborator

remember the concern about namespace collisions?
aframe-master.js:71618 The primitive a-text has a mapping collision. The attribute text has the same name as a registered component and has been renamed to text-text
so renaming in the primitive at least...

Collaborator

machenmusik commented Jan 20, 2017

remember the concern about namespace collisions?
aframe-master.js:71618 The primitive a-text has a mapping collision. The attribute text has the same name as a registered component and has been renamed to text-text
so renaming in the primitive at least...

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 20, 2017

Member

Maybe <a-text content="Hello"> or <a-text value="Hello">? Also, we can talk more about future of primitives in Slack later on.

Member

ngokevin commented Jan 20, 2017

Maybe <a-text content="Hello"> or <a-text value="Hello">? Also, we can talk more about future of primitives in Slack later on.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 20, 2017

Collaborator

already did <a-text value="hi there"> so we are good there, question is whether to change for text as well to make it consistent

Collaborator

machenmusik commented Jan 20, 2017

already did <a-text value="hi there"> so we are good there, question is whether to change for text as well to make it consistent

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 21, 2017

Member

Makes sense to me. text.value.

Member

ngokevin commented Jan 21, 2017

Makes sense to me. text.value.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 21, 2017

Collaborator

text.value is in there. we have candidate docs now, functionality LGTM at the moment, any headway on tests? not sure if there were any from prior incarnations to leverage

Collaborator

machenmusik commented Jan 21, 2017

text.value is in there. we have candidate docs now, functionality LGTM at the moment, any headway on tests? not sure if there were any from prior incarnations to leverage

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 21, 2017

Member

I can help with tests right after I finish my current set of work. I'll put it up next.

Member

ngokevin commented Jan 21, 2017

I can help with tests right after I finish my current set of work. I'll put it up next.

docs/components/text.md
+
+The text component renders bitmap and signed distance field font text.
+
+From Fernando Serrano's aframe-bmfont-component with further modifications by Michael Chen,

This comment has been minimized.

@fernandojsg

fernandojsg Jan 23, 2017

Member

I believe we should remove our names from here, in a readme as it was originally written makes sense to credit the sources and so on but here we're writing end-user documentation and I don't think it's useful anymore that info.

@fernandojsg

fernandojsg Jan 23, 2017

Member

I believe we should remove our names from here, in a readme as it was originally written makes sense to credit the sources and so on but here we're writing end-user documentation and I don't think it's useful anymore that info.

src/components/text.js
+ whiteSpace: {default: 'normal', oneOf: ['normal', 'pre', 'nowrap']},
+ // default to geometry width, or if not present then DEFAULT_WIDTH
+ width: {type: 'number'},
+ // units are 0.6035 * font size e.g. about one default font character (monospace DejaVu size 32)

This comment has been minimized.

@fernandojsg

fernandojsg Jan 23, 2017

Member

Following the discussion about the arbitrary 0.6035 number. I would remove it from the code as it's just an specific case hand-made computed value used to match the DejaVu size, so if you change the font it doesn't make any sense to keep using it and the values for width will be also arbitrary.
So better to skip using it and make the code less cryptic.

@fernandojsg

fernandojsg Jan 23, 2017

Member

Following the discussion about the arbitrary 0.6035 number. I would remove it from the code as it's just an specific case hand-made computed value used to match the DejaVu size, so if you change the font it doesn't make any sense to keep using it and the values for width will be also arbitrary.
So better to skip using it and make the code less cryptic.

This comment has been minimized.

@machenmusik

machenmusik Jan 23, 2017

Collaborator

as it performs a useful function, i think it will be better to try and find a derivation applicable to other fonts, will investigate some more

@machenmusik

machenmusik Jan 23, 2017

Collaborator

as it performs a useful function, i think it will be better to try and find a derivation applicable to other fonts, will investigate some more

This comment has been minimized.

@machenmusik

machenmusik Jan 23, 2017

Collaborator

no longer arbitrary, added some code to do quick computation on font load...

@machenmusik

machenmusik Jan 23, 2017

Collaborator

no longer arbitrary, added some code to do quick computation on font load...

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 24, 2017

Member

OK, helping on tests and clean up now

Member

ngokevin commented Jan 24, 2017

OK, helping on tests and clean up now

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 25, 2017

Member

Would be great to have a section in the doc about other possible text implementatons (geometry, DOM-to-canvas). Usages, advantages, disadvantages, links.

Member

ngokevin commented Jan 25, 2017

Would be great to have a section in the doc about other possible text implementatons (geometry, DOM-to-canvas). Usages, advantages, disadvantages, links.

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 25, 2017

Member

I've filed mrdoob/three.js#10644 to improve the three.js FileLoader so we don't have to do the whole get or create Promise dance. Else I'm gonna work on decoupling that stuff we have now out of the component to make it easier to read and test.

Member

ngokevin commented Jan 25, 2017

I've filed mrdoob/three.js#10644 to improve the three.js FileLoader so we don't have to do the whole get or create Promise dance. Else I'm gonna work on decoupling that stuff we have now out of the component to make it easier to read and test.

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 25, 2017

Collaborator

re: other implementations - yeah that would probably be useful, although it could probably use a broader perspective if someone has tried them all

Collaborator

machenmusik commented Jan 25, 2017

re: other implementations - yeah that would probably be useful, although it could probably use a broader perspective if someone has tried them all

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 25, 2017

Collaborator

re: moving promise caching to FileLoader - one danger there is loss of granularity (e.g.caching small things like fonts but not always large things like videos that are being progressively streamed) - aframe texture promise cache is separate from this font promise cache, etc.

Collaborator

machenmusik commented Jan 25, 2017

re: moving promise caching to FileLoader - one danger there is loss of granularity (e.g.caching small things like fonts but not always large things like videos that are being progressively streamed) - aframe texture promise cache is separate from this font promise cache, etc.

+ 'text': {anchor: 'align', width: 5}
+ },
+ mappings: {
+ align: 'text.align',

This comment has been minimized.

@ngokevin

ngokevin Jan 25, 2017

Member

These could be automatically populated given the schema. I'd also add a hyphen whenever there's a camelCase (e.g., font-image).

@ngokevin

ngokevin Jan 25, 2017

Member

These could be automatically populated given the schema. I'd also add a hyphen whenever there's a camelCase (e.g., font-image).

This comment has been minimized.

@machenmusik

machenmusik Jan 25, 2017

Collaborator

sorry, parsing - are you asking to remove the defaults from being specified in the primitive because you want to make them rely on defaults from the mapped components themselves?

@machenmusik

machenmusik Jan 25, 2017

Collaborator

sorry, parsing - are you asking to remove the defaults from being specified in the primitive because you want to make them rely on defaults from the mapped components themselves?

This comment has been minimized.

@ngokevin

ngokevin Jan 25, 2017

Member

Sorry. I meant two things:

  1. Populate the mappings given the text component schema. Check the mesh mixin how we do it for the material properties. This allows us to not have to update the primitive every time we update a property name.

  2. Use hyphens instead of all lowercase. font-image vs fontimage

@ngokevin

ngokevin Jan 25, 2017

Member

Sorry. I meant two things:

  1. Populate the mappings given the text component schema. Check the mesh mixin how we do it for the material properties. This allows us to not have to update the primitive every time we update a property name.

  2. Use hyphens instead of all lowercase. font-image vs fontimage

machenmusik and others added some commits Dec 21, 2016

add bmfont-text component and a-text primitive to core from https://g…
…ithub.com/chenzlabs/aframe-bmfont-component

add text example

merge changes from component PR

fix size animation in text example

changes requested per discussion on PR

various edits per discussion on PR

comment on coerceData

remove obsolete comments

try to make github outdated detection happy

first pass of crude caching

change base URL to fonts pending aframevr/assets#13

better implementation of font and texture caching using Promise

remap font before checking cached promises

allow bmfont-text to use custom shaders

better custom shader support

make sure transparent is set if custom shader

default to modified SDF shader that looks much better at a distance

tweak custom shader to avoid flickering

move example to test
various edits and fixes as per discussion on PR
more edits per discussion on PR

better shader, reinstate alphaTest

renamed bmfont-text to just text per discussion on PR

shaders don't have a dispose

use kelsonsans too

to my eyes at least, further improved custom shader

camelCase fixes per discussion on PR

remove customShader property and add modified-sdf as default shader property, per discussion on PR

fix missed property name changes

allow multiple instances of text component

to avoid namespace collision, a-text value property is now what receives the text string to display

add another text example

first pass at docs based on @fernandojsg and @bryik, with some additions

change text property to value to match primitive

removed historical information per discussion on PR

compute width factor for font, rather than using magic constant

try tweaking shader transition

Revert "try tweaking shader transition"

This reverts commit 7594b6e.

add text sizes example

add descender into total height

add fonts example

tests and cleanup
better shader; FIXME: change shader processing to keep initial prepro…
…cessor extensions stuff at the top of the file to avoid browser warning

fix intermittent test "updates geometry with value"

// NOTE: there are two paths by which geometry update can happen;
// first, as after-effect of font change;
// second, as direct effect when no font change.
// To make this test work reliably,
// modified text component so order of arguments is same in both cases.
// If the first case is happening, the call may not be instantaneous,
// and we would need to wait until the font is loaded.

revamp a-text primitive to extend automagically from text

tweak shader to perform modified alpha test
(makes examples/test/text/scenarios.html look significantly better)

expose zOffset

more resilient to off-axis viewing angles

fix missed modified-sdf --> modifiedsdf

add documentation for a-text primitive

finish unit tests

clean up examples

newlines in text value can be expressed as \n

fix multiple instancing bug seen with examples/test/text/scenarios.html
rename modifiedsdf shader to sdf, and allow registered shaders;
remove old Jam3 shaders since they weren't being used

fix geometryComponent use before assignment from cleanup

update documentation

add unit tests for multiple and text autoscale

a little more on text geometry interaction
@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin Jan 27, 2017

Member

Alright! Spent pretty much the entire week looking at this and helping with tests, clean up, docs, and refactors. It looks great. Super thanks @machenmusik! And @fernandojsg @bryik for the long lineage of the text component. 🥇

If we catch anything else, we can address in a smaller follow up PRs.

Member

ngokevin commented Jan 27, 2017

Alright! Spent pretty much the entire week looking at this and helping with tests, clean up, docs, and refactors. It looks great. Super thanks @machenmusik! And @fernandojsg @bryik for the long lineage of the text component. 🥇

If we catch anything else, we can address in a smaller follow up PRs.

@ngokevin ngokevin merged commit 5d87ccc into aframevr:master Jan 27, 2017

3 checks passed

codecov/patch 95.34% of diff hit (target 1.00%)
Details
codecov/project 85.97% (target 70.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ngokevin ngokevin changed the title from add bmfont-text component and a-text primitive to core to add text component and a-text primitive to core Jan 27, 2017

@machenmusik

This comment has been minimized.

Show comment
Hide comment
@machenmusik

machenmusik Jan 27, 2017

Collaborator

awesome! thanks to everyone for sticking with it!

maybe we should add showing "Hello World!" to the A-Frame hello world example...

Collaborator

machenmusik commented Jan 27, 2017

awesome! thanks to everyone for sticking with it!

maybe we should add showing "Hello World!" to the A-Frame hello world example...

@ngokevin ngokevin referenced this pull request Jan 31, 2017

Closed

[feature request] text #1966

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment