Webgl text #3110

Spongman · 2018-08-03T09:36:18Z

implements text() for the webgl renderer. most of the code is in src/webgl/text.js, however some other changes were required:

moving most of the p5.Renderer2D.prototype.text() and textAlign() code into shared methods in p5.Renderer, this necessitated:
changing the textAlign and textBaseline properties so they are backed directly by variables in the renderer instead of being calculated based on canvas properties. this also simplifies the logic for those properties.
changes to push/pop, _applyTextProperties methods to accommodate the above.
changes to various p5.Font methods to make them renderer-independent.
optimization in p5.RendererGL.prototype.getTexture which gets called a ton.
add support for creating p5.Texture from an ImageData object.
a new private p5.Shader.prototype.updateTextures() method that ensures all dirty textures are sent to the GPU.

Spongman · 2018-08-03T21:48:41Z

src/core/p5.Renderer.js

+  }
+};
+
+p5.Renderer.prototype.text = function(str, x, y, maxWidth, maxHeight) {


this code came from p5.Renderer2D.prototype.text

Spongman · 2018-08-03T21:50:47Z

src/webgl/p5.RendererGL.js

+  var textures = this.textures;
+  for (var it = 0; it < textures.length; ++it) {
+    var texture = textures[it];
+    if (texture.src === img) return texture;


using a for loop instead of Array.find() which is really slow..

Maybe indexOf would be faster? http://www.andygup.net/fastest-way-to-find-an-item-in-a-javascript-array/

i don't think indexOf would work in this case since we're looking for the element that has a particular (src) property. this is what find() is intended for - apply a predicate to each element and return the first that matches. but the function call is really slow.

Spongman · 2018-08-03T21:51:43Z

src/webgl/p5.Texture.js


  var gl = this._renderer.GL;
  // pull texture from data, make sure width & height are appropriate
  if (textureData.width !== this.width || textureData.height !== this.height) {
+    updated = true;


using this flag to de-duplicate the texImage2D code

Spongman · 2018-08-03T21:52:28Z

src/webgl/shaders/font.frag

@@ -0,0 +1,189 @@
+#extension GL_OES_standard_derivatives : enable


i'll comment this when we have glsl minification in the build

Yes, this file needs comments. 👍

i added issue #3114

... and PR #3116

Spongman · 2018-08-03T21:58:05Z

src/core/p5.Renderer.js

@@ -44,6 +44,8 @@ p5.Renderer = function(elt, pInst, isMainCanvas) {
  this._textStyle = constants.NORMAL;
  this._textAscent = null;
  this._textDescent = null;
+  this._textAlign = constants.LEFT;
+  this._textBaseline = constants.BOTTOM;


these are stored here now, instead of as native 2d canvas properties.

AdilRabbani · 2018-08-04T11:06:54Z

Oh boy, this is my first time reviewing a PR....please bear with me 😬

AdilRabbani

I'm getting this output on https://p5js.org/reference/#/p5/textLeading example in 2D on your build :

textAscent() also seems to give unexpected results in 2D mode :

Original	Now

AdilRabbani · 2018-08-04T11:21:43Z

src/webgl/text.js

+ *
+ * the ImageInfos class holds a list of ImageDatas of a given size.
+ */
+function ImageInfos(width, height) {


Considering this is private, wouldn't it be better to make it _ImageInfos? Just like other private functions in p5.js? Also there are other functions like this one in this file (setPixel, FontInfo...).

uh-oh, good catch on the 2d bugs, that definitely needs to be fixed.

as for the '_'s, i don't think this is necessary. those functions are all private to the 'text' module whose only export is the p5 object.

ok, 1543600 should fix the alignment/metrics issues.

AdilRabbani · 2018-08-06T10:11:54Z

Can there be a default font for WebGL as well? It feels odd that nothing is displayed if the user doesn't load a font. Getting 'WEBGL: only opentype fonts are supported' log in the console.
I think there should be an option for z coordinates as well? (str, x, y, z, maxWidth, maxHeight)
We would need a WebGL doc example also. Above p5.prototype.text().

@kjhollen can you take a final look at this as well? 😀

Spongman · 2018-08-06T20:50:41Z

i thought about adding a z parameter, but decided against it since other primitives like arc, rect, ellipse & triangle only take 2d coordinates. i figure if you're going real 3d positioning of stuff in webgl-mode then you probably should be familiar with translate(). oh, and there's no way to distinguish between text(str,x,y,width) and text(str,x,y,z).

i added an example for text().

as for adding a default font: it's a good idea, but i'm not sure how easy this would be to implement. do we always preload it before setup()? or do we add some async on-demand loading the first time it's required? do we load it from a fixed location on the internet (what if the machine is disconnected?), do we require it to be located in a specific location relative to the sketch? do we embed it as a base64-encoded binary (~35K for latin script) in the p5.js javascript?

AdilRabbani · 2018-08-07T16:37:30Z

This seems a bit tricky. How is 2D mode handling this? I think it seems to be using a default web safe font?

Spongman · 2018-08-07T16:52:09Z

2d mode is using <canvas> so, as well as opentype fonts, it can use the fonts built into the browser (or OS). the default font is set here:

p5.js/src/core/p5.Renderer2D.js

Line 31 in 0956fc9

this.drawingContext.font = 'normal 12px sans-serif';

AdilRabbani · 2018-08-07T17:16:30Z

@kjhollen @mlarghydracept Overall the implementation is working great! There is a problem of adding a default font. What do you suggest?

dhowe · 2018-08-08T03:40:42Z

I'm getting this output on https://p5js.org/reference/#/p5/textLeading example in 2D on your build
textAscent() also seems to give unexpected results in 2D mode

if you need more checks for alignment, leading etc, some of the sketches in 'manual-test-examples' might be of use in verifying that 2d, 3d, and original processing are all producing the same results (test/manual-test-examples/p5.Font/custom/index.html for example)...

kjhollen · 2018-08-08T22:09:32Z

thank you for the initial review, @AdilRabbani! I'm happy to take a deeper look at this—I'll do it next week once I settle in from moving a little more, if that works for you @Spongman?

stalgiag · 2018-08-08T22:10:00Z

I just had an opportunity to briefly look over this and everything looked fantastic. I can properly review it on Friday.

Edit: We have comical timing @kjhollen . Seems like @Spongman closing and reopening summoned both of us.

Spongman · 2018-08-08T22:10:47Z

thanks everyone, no rush ;-)

kjhollen · 2018-08-08T22:10:55Z

or @mlarghydracept can look sooner :)

Spongman · 2018-08-08T22:12:43Z

sorry about that - didn't mean to spam - travis got stuck for some reason on the last commit and i don't know any other way to re-trigger it.

stalgiag · 2018-08-11T01:31:24Z

Okay I have been taking some more time with this and realize there are huge number of changes to look over. I will try to do a line-by-line this weekend but for now I have just managed to give it a broad-strokes once over. So far everything works really well but I have concerns that this structure in text.js may make future contributions difficult. Text rendering is a difficult task but I wonder if there are ways that this can be changed slightly to make it less intimidating to future contributors. Namely there is a high density of class declarations and utility functions. This may be unavoidable but it feels different than the rest of the library. Also, why the additions to the p5.Vector class? Maybe I am missing something but couldn't the static methods be used to accomplish this same task without adding to that class?

Spongman · 2018-08-11T02:19:43Z

the main reason i added the private fluent methods to p5.Vector was to make the vector math in the cubic approximation code easier to read. i originally had it with the static methods, but after the linter made a mess of things, i switched it to fluent.

for example, there's a statement in splitInflections() which calculates the vector expression c = this.p1 - this.c1 - a - b*2:

        var c = p5.Vector.sub(
          p5.Vector.sub(p5.Vector.sub(this.p1, this.c1), a),
          p5.Vector.mul(b, 2)
        );

and here's the same statement with the fluent interface:

        var c = this.p1
          .minus(this.c1)
          .minus(a)
          .minus(b.times(2));

maybe it's just me, but I find it significantly easier to work out what's actually happening when reading the 2nd version mostly because it reads in my head identically to the math expression it's calculating.

also, having private/undocumented methods in one part of the library that are used by another is not without precedent.

stalgiag · 2018-08-11T22:13:49Z

Hm I see how that probably looks pretty convoluted with the static methods. I still don't really know if it is okay to add these fluent methods to p5.Vector just for this single use.

So I have been trying to think about solutions to the default font issue. This feels like a pretty confusing roadbump for users. I haven't had any epiphanies but I want to keep up a conversation about it because it feels significant. It may be an inescapable aspect of the implementation, in which case we'd need to make sure that the reference clearly explains the need for a loaded font when using WebGL.

stalgiag · 2018-08-11T22:16:22Z

test/manual-test-examples/p5.Font/custom/sketch.js

@@ -1,11 +1,25 @@
+var _webgl = false;


I'm getting what appear to be errors for this in Chrome. I get console warnings about the number of GL contexts and the dead computer icon inside some of the canvases.

yeah, it looks like some browsers limit the number of concurrent webgl contexts. try edge, it doesn't do this.

Spongman · 2018-08-11T23:33:12Z

ok, i don't mind changing it back to the static methods. the math is actually a little simpler than it used to be.

Spongman · 2018-08-19T23:56:18Z

travis stalled again, ugh. i rebased on to master (post webgl-gsoc-2018).

stalgiag · 2018-08-20T20:22:19Z

I agree that there it feels that there should be somewhere for more elaborate documentation. Reference docs focus on clarity and simplicity. Not sure about the solution there, but for now I feel that this PR is ready to merge. @kjhollen have any interest in giving this a final glance before I merge? It is a pretty large addition. Thanks for your patience @Spongman!

AdilRabbani · 2018-08-20T20:52:12Z

WebGL docs would need an update regarding WebGL text in p5.js.

stalgiag · 2018-08-20T20:54:03Z

Good call @AdilRabbani!

stalgiag · 2018-08-24T21:44:34Z

@Spongman just to clarify - I think that we can move forward with a merge. I have spoken with @kjhollen. The only thing that needs to happen before that is that we need to update the [developer_docs] (https://github.com/processing/p5.js/blob/master/developer_docs/webgl_mode_architecture.md) as per @AdilRabbani's suggestion.

stalgiag · 2018-08-29T20:49:55Z

Okay! @Spongman Merging now. Thanks for the final touches, especially the additional comments. It helps demystify quite a lot. This is a large and well-considered contribution!

Can you make an issue that addresses the default font when you get a chance? It seems that you have already done quite a bit of work to consider possible solutions and it would be good to document these for future contributors.

Spongman · 2018-08-30T04:28:09Z

fantastic, thanks for reviewing this!

stalgiag requested a review from AdilRabbani August 3, 2018 15:00

Spongman commented Aug 3, 2018

View reviewed changes

AdilRabbani suggested changes Aug 4, 2018

View reviewed changes

AdilRabbani requested a review from kjhollen August 6, 2018 10:12

Spongman force-pushed the webgl-text branch from 2cd66a4 to 04ce961 Compare August 6, 2018 19:22

Spongman closed this Aug 8, 2018

Spongman reopened this Aug 8, 2018

lmccart added the Area:WebGL label Aug 10, 2018

stalgiag reviewed Aug 11, 2018

View reviewed changes

Spongman force-pushed the webgl-text branch from e13cea3 to 1dad9ab Compare August 11, 2018 23:22

piersh added 16 commits August 18, 2018 22:15

reduce renderer-type dependency in textAlign() and text()

99a39b1

enable creating p5.Textures from ImageData objects

67bf755

add webgl text() support

90b753d

webgl text test

f1a9d39

add torus test-example

4813520

add support for cubic beziers

a0aeb2a

fix antialias scaling on high-dpi screens

f5afdef

add some comments

e502de7

fix vertical alignment default & translation

db05085

add a manual test example displaying chinese characters

5104f28

add a WEBGL text() example

5fa8213

add some comments, optimize antialias calculation

144a176

fix text alignment

88f64b5

fixes for IE11

577906f

uglify vector math

b1b1f22

add text() docs

e14f30a

Spongman force-pushed the webgl-text branch from 1c00c9c to e14f30a Compare August 19, 2018 05:15

Spongman closed this Aug 19, 2018

Spongman reopened this Aug 19, 2018

update architecture docs, more text comments

b16b646

stalgiag merged commit 63ab55d into processing:master Aug 29, 2018

Spongman deleted the webgl-text branch August 29, 2018 20:56

and-who mentioned this pull request Jun 12, 2019

p.text() not displaying P5-wrapper/react#34

Closed

		@@ -0,0 +1,189 @@
		#extension GL_OES_standard_derivatives : enable

Webgl text #3110

Webgl text #3110

Conversation

Spongman commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spongman Aug 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spongman Aug 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdilRabbani commented Aug 4, 2018 • edited

AdilRabbani left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdilRabbani commented Aug 6, 2018 • edited

Spongman commented Aug 6, 2018 • edited

AdilRabbani commented Aug 7, 2018

Spongman commented Aug 7, 2018

AdilRabbani commented Aug 7, 2018 • edited

dhowe commented Aug 8, 2018 • edited

kjhollen commented Aug 8, 2018

stalgiag commented Aug 8, 2018 • edited

Spongman commented Aug 8, 2018

kjhollen commented Aug 8, 2018

Spongman commented Aug 8, 2018

stalgiag commented Aug 11, 2018

Spongman commented Aug 11, 2018 • edited

stalgiag commented Aug 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spongman commented Aug 11, 2018

Spongman commented Aug 19, 2018 • edited

stalgiag commented Aug 20, 2018

AdilRabbani commented Aug 20, 2018

stalgiag commented Aug 20, 2018

stalgiag commented Aug 24, 2018

stalgiag commented Aug 29, 2018

Spongman commented Aug 30, 2018

Spongman Aug 4, 2018 •

edited

Spongman Aug 3, 2018 •

edited

AdilRabbani commented Aug 4, 2018 •

edited

AdilRabbani left a comment •

edited

AdilRabbani commented Aug 6, 2018 •

edited

Spongman commented Aug 6, 2018 •

edited

AdilRabbani commented Aug 7, 2018 •

edited

dhowe commented Aug 8, 2018 •

edited

stalgiag commented Aug 8, 2018 •

edited

Spongman commented Aug 11, 2018 •

edited

Spongman commented Aug 19, 2018 •

edited