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

createOutlines() + pathToPoints() #352

Merged
merged 11 commits into from
May 21, 2020
Merged

createOutlines() + pathToPoints() #352

merged 11 commits into from
May 21, 2020

Conversation

ffd8
Copy link
Member

@ffd8 ffd8 commented Mar 22, 2020

  • added createOutlines(), handles textFrames, textPaths, multi-line text, multi-box text.
  • added pathToPoints(), handles polygons from createOutlines() or most other pageItems (oval, rect, polygon, etc), also accepts groups and arrays.
  • added examples per function in documentation
  • added tests for both functions
  • updated changelog
  • added nice error message for textSize(), common beginner mistake giving size less than zero.
    (totally new approaching, replacing basiljs gets points! #268 )

- added `createOutlines()` function, handles textFrames, textPaths, multi-line text, multi-box text.
- added `pathToPoints()` function, handles polygons from `createOutlines()` or most other pageItems (oval, rect, polygon, etc), also accepts groups and arrays.
- added tests for both functions
- updated changelog
- added nice error message for `textSize()`, common beginner mistake giving size less than zero.
@ffd8 ffd8 requested a review from trych March 22, 2020 00:37
@ffd8 ffd8 mentioned this pull request Mar 22, 2020
@ffd8
Copy link
Member Author

ffd8 commented Mar 22, 2020

PS - anyone using Sublime + docblockr (for jsdocs) know of a trick for adding the * before a pasted in block of code for an example??

ie. when adding an example of code to an @example, I get the following:

 * @example <caption>Beziers from text using createOutlines()</caption>
 * textSize(400);
var myText = text("H", width/4, height/4, 500, 500);
var outlines = createOutlines(myText);
var pts = pathToPoints(outlines);
property(outlines, "fillColor", "None");

(where only the first line where pasted into is commented... all the following need to be manually given the * in front:

 * @example <caption>Beziers from text using createOutlines()</caption>
 * textSize(400);
 * var myText = text("H", width/4, height/4, 500, 500);
 * var outlines = createOutlines(myText);
 * var pts = pathToPoints(outlines);
 * property(outlines, "fillColor", "None");

What's the trick for pasting in and getting those *'s.. or post-paste, getting them without re-formatting the whole jsdoc block (which doesn't add proper *'s to code block either).... Maybe there's a better Sublime plugin? Would like to eventually go through and add lots of tiny examples to functions – but need to figure out this workflow.

Copy link
Contributor

@trych trych left a comment

Choose a reason for hiding this comment

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

Hi @ffd8, great stuff!

Here is some observations and suggestions:

pathsToPoint()

  • Docs:

    • I think the description is really confusing. I did not understand at all what to expect until I started examining the examples in detail with help of the inspect function.

      Suggestion:
      Returns an object containing an array of all points, an array of all beziers (points + their anchor points) and an array of all paths of a given path item in InDesign. Together with createOutlines() this can be used on text items to retrieve points, beziers and paths of text items. Accepts both single paths or a collection/group of paths.
      When using this on a multi path object (e.g. text with separate paths), the paths property can be used to loop over every path separately, whereas the properties points and beziers contain arrays for all paths combined.
      An optional second parameter allows to add and return additional points between existing points, which is helpful for subdividing existing paths.

    • Skip the "Ignore if using beziers."

    • Return value I would describe like this:
      Returns object with the following arrays points, beziers, paths

  • Examples
    The examples you give are more suited for our examples repo as they go into detail quite a bit.
    I think the examples that should be given in the inline docs should mostly show the syntax and the different options how to use the function with its different options.

    Generally the examples are good, however, I think they could use some improvements in some areas:

    • Example 1
      I think I would rather use some sort of s-shaped path here instead of a circle. With the circle it's not quite obvious at first glance that the five extra points between the original points are necessarily created via the pointsToPath() function (alternativly they might be placed there via matrix rotation). I think an actual path in an s-shape makes that more clear.

    • Examples 2
      I would set the debug bezier paths to a different color (or more transparent) to make them distinct from the actual letter paths. Also, you should use a different character than "H", because if the users have set Arial or so as their default font they won't see any beziers. Maybe an "f" or "G"

    • Example 3
      I would assign a random color to each separate path to make it more obvious that the script results in separated paths.

    • Example 1 & Example 2
      It seems you created the script with points as default unit. When I run it with MM, the bezier points are huge and don't really look like path points anymore. We should just add units(PT) at the beginning of the script to make the result look consistent.

  • Function:
    If I pass the second parameter (add extra points) on open curves, it adds points as if the shape were closed (see image). I would expect it to only return points of actual existing paths though.

20200322-152213_Screenshot_InDesign

createOutlines

  • Docs
    I would skip "Intended for use with pathToPoints() ..." as the function has lots of other useful applications.

  • Examples
    Same as above, I find the examples to extensive for the inline examples. I would move them to the examples repo and instead only provide short examples here that show the syntax and how to use the different options

    • Example 1
      Instead of setting fill color to none, I would set it to some distinct color.
      Either uncomment the inspect line or take it out.

    • Example 2
      Instead of setting fill color to none, I would set it to some distinct color.
      I find the mapping of the point size more confusing than helpful. Maybe that's also a unit problem?

    • Example 3
      Love it! :)

    Example 1, Example 2
    Same comment about the units as the pathToPoints() examples above, set to PT

  • Function

    • Return value: Would it maybe be better to always return an array, even if there is just one polygon? Otherwise the user would always have to test first, if they have a polygon or an array? Not sure. If you feel otherwise, ignore this.

    • if converting a story or a word with overset text, InDesign will throw an error. What should we do about this?

    • if converting linked text frames this behaves really unpredictably. It deletes all text that comes after the give text frame. It converts all text frames to groups of polygons, if there are text frames before the given text frames. So the return value becomes even more unpredictable. I think we should try to return predictable results. For example if I use this command on a text frame that is linked with other text frames, I think all these text frames should be converted to outlines and then all the polygons should be returned in an array.

    • From the docs:

    Warning, InDesign requires that text have a fill color in order to createOutlines.

    I think it would be nice if the function would take care of not letting that happen (i.e. give text temporarily a fill color so it can be converted to outlines)

  • All in all the function seems quite slow. I wonder if it can be optimized in any way or if we just hit the limits of InDesign here?

Other:

You changed the checkNull option to

var checkNull = function (obj) {
  if(obj === null || typeof obj === undefined) error("Received null " + obj);
};

What's the purpose of that? This prints "Received null null" when an actual null is passed to this function.

@ffd8
Copy link
Member Author

ffd8 commented Mar 22, 2020

Thanks for the thorough review- good points in there.. will walk through them and apply as necessary.

A couple replies to some of the feedback:

pathsToPoint()

  • I think the description is really confusing. I did not understand at all what to expect until I started examining the examples in detail with help of the inspect function.

Good suggestion above (haha probably wrote too late/fast in first draft).

  • Function:
    If I pass the second parameter (add extra points) on open curves, it adds points as if the shape were closed (see image). I would expect it to only return points of actual existing paths though.

Ahhh damn.. found a weird case I hadn't tested.. guess I'll have to check if line or closed shape – since in all closed shapes, need to finish interpolation back to starting point.

createOutlines

  • Docs
    I would skip "Intended for use with pathToPoints() ..." as the function has lots of other useful applications.

No prob to remove intended, but should still reference the other function since both are necessary when wanting to play with type points.

  • Return value: Would it maybe be better to always return an array, even if there is just one polygon? Otherwise the user would always have to test first, if they have a polygon or an array? Not sure. If you feel otherwise, ignore this.

Good question and idea.. yeah, it can be random/surprising when InDesign says it's a single polygon or an array = probably safest to just always suggest iterating over an array.

  • if converting a story or a word with overset text, InDesign will throw an error. What should we do about this?

Hmm ok gotta test this (and see what natively happens in ID using GUI option to createOutlines – some of this weird behavior would happen here too).

  • if converting linked text frames this behaves really unpredictably. It deletes all text that comes after the give text frame. It converts all text frames to groups of polygons, if there are text frames before the given text frames. So the return value becomes even more unpredictable. I think we should try to return predictable results. For example if I use this command on a text frame that is linked with other text frames, I think all these text frames should be converted to outlines and then all the polygons should be returned in an array.

Strange.. only tested with two linked textFrames (on same page.. no prob, on split pages, naturally only the one on active page converted) - but will test more.

  • From the docs:

Warning, InDesign requires that text have a fill color in order to createOutlines.
I think it would be nice if the function would take care of not letting that happen (i.e. give text temporarily a fill color so it can be converted to outlines)

This is tricky territor, as within InDesign GUI of createOutlines - you get the same warning if trying to createOutlines on non-filled text = who are we to change the color of that type? and to what color? Of course it helps if one had said noFill() prior to making a text() they want to outline.. but again, how should it default to a color? I can try to give it a fill.. then remove that fill from the outlined polygons...

  • All in all the function seems quite slow. I wonder if it can be optimized in any way or if we just hit the limits of InDesign here?

True – it definitely is slower than just using the GUI menu.. but maybe we hit some slog in how many things we have to check that they don't?

Other:

You changed the checkNull option to
What's the purpose of that? This prints "Received null null" when an actual null is passed to this function.

That was while trying to finally debug (and quiely exit) a super common bug when doing something like words() with a callback function.. changing the pointSize and running items off the page that can no longer be edited or plenty of other similar situations.. in that example, it was more helpful to atleast return the type of object in the error message - rather than just saying generically 'received null object' (since that doesn't help student figure out which one). There's probably a far better way to handle this.. so I can remove from this PR and we look at it separately.

@ffd8
Copy link
Member Author

ffd8 commented Mar 22, 2020

PS @trych - while I redo/simplify the examples.. do you know the trick in Sublime + Docblockr for pasting in code after @example and automatically getting the doc *'s on each line?

@ffd8
Copy link
Member Author

ffd8 commented Mar 22, 2020

Function:
If I pass the second parameter (add extra points) on open curves, it adds points as if the shape were closed (see image). I would expect it to only return points of actual existing paths though.

Wow- this is driving me nuts.. anyone know how to tell if a polygon has an open path or not?? Testing out a handful of shapes to try and isolate a drawn or pen-tooled open curve (to avoid adding extra points between first and last point, needed on all closed forms). Can't find a key that reports if a pageItem has a closed path... Compared files and only the ID and contenType was unique as 'unassigned' but the same is true for a rectangle..??

Running inspect() on the following forms:
Screen Shot 2020-03-22 at 19 39 42

Issue selected - when running pathToPoints(obj, 5) (adding 5 points):
Screen Shot 2020-03-22 at 19 45 20

Code:

function draw(){
	units(PT);
	// inspect(selection());
	var pts = pathToPoints(selections(), 5);

	fill(255, 0, 0);
	for(var i=0; i<pts.points.length; i++){
		var pt = pts.points[i];
		ellipse(pt.x, pt.y, 5, 5);
	}
}

- returned core.js back to normal
- modified shape description
@trych
Copy link
Contributor

trych commented Mar 22, 2020

PS @trych - while I redo/simplify the examples.. do you know the trick in Sublime + Docblockr for pasting in code after @example and automatically getting the doc *'s on each line?

@ffd8 Ok, first this, before I address the other points. If I understand you right you just need to place several * at the beginning of a few lines? You can hold alt key, while click dragging down at the front of some lines to place several cursors there and then you just need to type one *. Is that what you mean?

@trych
Copy link
Contributor

trych commented Mar 22, 2020

About the other points:

No prob to remove intended, but should still reference the other function since both are necessary when wanting to play with type points.

For me it would be enough to have this in an example script, but if you want to briefly mention it in the docs, fine with me.

probably safest to just always suggest iterating over an array.

👍

Strange.. only tested with two linked textFrames (on same page.. no prob, on split pages, naturally only the one on active page converted) - but will test more.

Wonder if this is maybe a CS6 issue? I had linked text frames on the same text frames. Maybe let's set up an example and compare on our systems, so we can see if this behaves differently in CS6 and latest CC.

I can try to give it a fill.. then remove that fill from the outlined polygons...

Yeah, that's what I meant. Does it also not work on text with no fill color but with a stroke color? Because that would be strange if that failed to convert it to outlines if the user can even see the type.

True – it definitely is slower than just using the GUI menu.. but maybe we hit some slog in how many things we have to check that they don't?

Will have a closer look at the function itself later, maybe there is some space for optimization, I just checked the functionality itself, had not really a closer look at the code yet.

There's probably a far better way to handle this.. so I can remove from this PR and we look at it separately.

Yes, I think that would be good. I'm not a big fan of the checknull function to be honest, it is a pretty lazy checking and gives a pretty poor feedback to users without letting them know which function causes the error. So, yes, let's address this separately.

@trych
Copy link
Contributor

trych commented Mar 22, 2020

anyone know how to tell if a polygon has an open path or not

Info is stored in the path object:

anyone know how to tell if a polygon has an open path or not
if(pageItem.paths[0].pathType === PathType.OPEN_PATH) {
  // open path
}

if(pageItem.paths[0].pathType === PathType.CLOSED_PATH) {
  // closed path
}

See: https://www.indesignjs.de/extendscriptAPI/indesign8/#Path.html

@ffd8
Copy link
Member Author

ffd8 commented Mar 22, 2020

Yeah, that's what I meant. Does it also not work on text with no fill color but with a stroke color? Because that would be strange if that failed to convert it to outlines if the user can even see the type.

Works fine with text w/ stroke.. but if no stroke and no fill = nope.
Screen Shot 2020-03-22 at 23 56 21

To avoid a weird situation while coding w/o fill() on text, will just temp give a color then remove it...


Will have a closer look at the function itself later, maybe there is some space for optimization, I just checked the functionality itself, had not really a closer look at the code yet.

Seems to be working just as fast now -making changes as I type.. will push updates before calling it a night.. still trying to isolate text on a path from it's form...?! ie. placing textPath on an ellipse.. with GUI, you get just the text and the ellipse disapears.. doing createOutlines() by code, you get the polygons, but they are still attached to the ellipse..?? Trying to remove the ellipse, will also remove the polygons (wondering if they do a duplication first in the background).


Re: checknull() ✅- not sure I know what to do.. but would definiely help debug scripts if it was more precise.


if(pageItem.paths[0].pathType === PathType.OPEN_PATH)

= 🤩🍻

- updated descriptions
- reduced examples to minimum

#### pathToPoints()
- fixed open path items w/ `addPts`

#### createOutlines()
- fixed no fill/stroke text with
- fixed issue with linked textFrames
- always returns array of polygons
@ffd8
Copy link
Member Author

ffd8 commented Mar 23, 2020

Just pushed new code reacting to most things mentioned above.
Only two issues remain for createOutlines():

textPaths (on pageItems, oval/rect/polygon/line) – trying to remove that item after converting to polygons.. in GUI, it goes poof.. by code, it sticks around. Any ideas? See code here

I just noticed a super slight shift of the type to the left when doing createOutlines by code vs GUI.... not sure if it's an issue with this function.. but could be annoying for really precise type treatment. Maybe it's just my setup.. or maybe InDesign is doing quite some fancy things when doing createOutlines() from GUI. Wanted to show a difference with a border on the textFrame, but just realized they are removing that textFrame (maybe doing a textFit first?) – but also speaks to same issue as above regarding textFrame/holder not going away. Also noticed a speed lag when doing createOutlines() to 6 linked textFrames (w/ placeholder size 25 fontsize) – GUI was instant, by code took 4 seconds...

.. left on while testing..
@trych
Copy link
Contributor

trych commented Mar 23, 2020

I just noticed a super slight shift of the type to the left when doing createOutlines by code vs GUI...

I also get a shift when I use the GUI. I think it's usually because it turns of kerning. Or do you mean an extra shift on top when using createOutlines()?

Also noticed a speed lag when doing createOutlines() to 6 linked textFrames (w/ placeholder size 25 fontsize) – GUI was instant, by code took 4 seconds...

Could you post a code sample here so I could give this some testing?

Will review the stuff tomorrow if I find time.

@trych
Copy link
Contributor

trych commented Mar 23, 2020

Actually to test this, could you maybe share your test scripts and (if required) test docs in .idml format? This is a quite complex feature, it would help me, if I would not have to build everything from scratch as I hae quite limited time right now.

@ffd8
Copy link
Member Author

ffd8 commented Mar 23, 2020

And of course, while preparing the same document for sharing/testing – the lag went poof and the slight offset too??? wtf.. haha now it's the same speed ~100ms and no offset. 🤔

Anyways here's the basic code + IDML.
To test, I suggest selecting a textFrame from one row and createOutlines via GUI, then do select one and run the following code:

function draw() {
  var outlines = createOutlines(selection()); // manually select one of the 3x boxes
}

textFrames_testing.idml.zip

clean up of wording/formatting for docs + made simpler minimal examples. For `pathToPoints()` I think it's still necessary to give small `for` loop examples for a beginner to know just how to process the object.
@ffd8
Copy link
Member Author

ffd8 commented Mar 23, 2020

Just pushed a latest update. I think it qualifies all review points now. The only detail that bugs me is this textPath object not being removed. But that's a tiny detail that will likely bother only me. Can re-approach this later, but would be nice to merge and start testing with students asap.

added small update to `textSize()` to changelog
@ffd8
Copy link
Member Author

ffd8 commented Mar 29, 2020

Been playing with this all week and it's been working really well – no speed issues anymore - outlines instantly. Drawing the bez/points as shapes (beginShape()...endShape()) happens super faast! ellipse(), line()..etc - just take a long time to render (with default visible mode()). Found changing colors between such shapes also slows is down.. but this applies to ANY looping situation. I guess it's so many objects for InDesign to keep track of.

@trych any issues, or can we merge it (and eventually improve if there's any found issues).

@trych
Copy link
Contributor

trych commented Mar 29, 2020

@ffd8 Sorry, I have really limited time due to this lockdown situation and having my kids at home 24/7. Will try to review this tonight.

If there is an urgent need to have your students use this, I would suggest they could download the branch's basil.js for now.

@ffd8
Copy link
Member Author

ffd8 commented Mar 29, 2020

@trych noo problem and totally understand. Great when you have a chance, but no rush.

Indeed – was interested to introduce this to my students next week, and if it's not merged by next Thursday, I'll simply deliver a custom dev basil.js w/ it, since function names/obj etc will likely stay the same.

@trych
Copy link
Contributor

trych commented Mar 29, 2020

Ok, here we go. (sorry, quite a few points again 🙈 )

pathToPoints()

Regarding the examples:

I think it's fine to have some loops, as it helps to demonstrate the functions. But I would suggest these improvements:

  1. have a more descriptive caption for each example
  2. make each example self contained, so users can copy and paste it (so no magic obj variable that might leave some users clueless)
  3. get rid of all unnecessary info, in this case I would skip the println() statements. If you are keen on keeping those in though, feel free to leave them in place.

In example 2, 3 & 4, if you want to keep the println statements, please insert a proper comment before "# of {something}", else there will be a syntax error when copy pasting the examples.

So something like

/*
 * @example <caption>Points</caption>
 * var pts = pathToPoints(obj);
 * println(pts.points.length); // # of points
 *
 * for (var i = 0; i < pts.points.length; i++) {
 *   var pt = pts.points[i];
 *   point(pt.x, pt.y);
 * }
 */

i would change to

/*
 * @example <caption>Draw all points of a vector path</caption>
 * var myCircle = ellipse(50, 50, 20, 20);
 * var pts = pathToPoints(myCircle);
 *
 * for (var i = 0; i < pts.points.length; i++) {
 *   var pt = pts.points[i];
 *   ellipse(pt.x, pt.y, 3, 3);
 * }
 */

(I replaced the point function with the ellipse function here as it would be easier to see the resulting circles)

Same notes go for the other examples.

Example 3:
I find it very hard to grasp what's even happening, as the original shape is basically redrawn. Maybe it would be a better showcase to draw some bezier handles of an arch or something?

Example 4:
To make this self contained this should set up a multi path object somehow in the first line, because when I copy past this and try this on a circle it is again not very clear, what this example demonstrates.

And one tiny extra note (that's probably something about you being an native English speaker and me not), I am not very familiar with the "w/" as a short form for "with", so for out international audience I would simply stay with "with". :)

Sorry, this all might seem pretty nit-picky, but if even I am struggling to get your examples to work easily, then a new user will surely have a difficult time.

Function:

  • Functionality-wise it seems to work fine from the limited testing I could do on it
  • obj instanceof Array should be isArray(obj) (had some unexpected behaviour with instanceof Array in the past)
  • There is not input validation at the moment. All functions should come with some input validation and give some helpful error message if incorrect arguments are given. Currently this is happening:
var tf = text(LOREM, 20, 20, 50, 50);

pathToPoints();       // -> Typfehler: undefined ist kein Objekt
pathToPoints("Test"); // -> Typfehler: undefined ist kein Objekt
pathToPoints([2, 3]); // -> Typfehler: undefined ist kein Objekt
pathToPoints(tf.words.firstItem()); // -> Fehler: Objekt unterstützt Eigenschaft oder Methode paths nicht
  • If an array is passed it would be probably best to check for each array element if it has a paths property and if not to just skip it. This way also the selection() function could be easily used as the input

  • Would it be possible for you to install a linter to make use of the .eslintrc files that we set up? There is quite a bit of incosistent spacing in your code (inconsistent with the rest of the basil code I mean) so to keep this clean I think it would be good, if the code could be linted.


createOutlines()

Docs:

  • To the description I would add a sentence "If used on a text frame, all linked text frames will be converted to outlines as well."

Example 1:

  • would add a caption "Convert text to outlines"

Example 2:

  • would add caption "Run a callback function on each resulting shape"
  • I would do something useful in the callback, maybe just print the number of points?

Function:

  • the debugC0 var and its print statements should be removed (leave the comments in though, those will be helpful)
  • error messages should always start with the function name so it becomes clear for the users which function cause the issue. I would go with something like
    error("createOutlines(), incorrect first parameter. Use: Story | TextFrame ...");
  • no need for return false; after the error, as an error interrupts the entire script anyways
  • outlines.length != undefined I would change to the more descriptive outlines.hasOwnProperty('length')

ffd8 added 3 commits April 2, 2020 23:45
- ran into strange issue with current dev branch
- forgot some inline comment slashes
- untested nested loop was using same var
- cleaned up examples (captions, code, self-contained)
- linted code
- more typechecking and clearer error messages
- removed debuging code
@ffd8 ffd8 merged commit cfd7fb9 into develop May 21, 2020
@ffd8
Copy link
Member Author

ffd8 commented May 21, 2020

@trych thanks for the thorough review. While it pushed the PR to the backburner, all are valid points that I've incorportated and built upon. Just merged, since it's working (and tested with 25 students over the past month). There are minor things to try and improve in the future (createOutlines() by code works differently than built in GUI method of InDesign.. they have a chain of functions taking place to isolate type from textFrame) – but that can be tweaked as needed after 2.0 release.

@ffd8 ffd8 deleted the dev-type branch May 21, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants