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

b-less transform #251

Merged
merged 35 commits into from
Mar 22, 2018
Merged

b-less transform #251

merged 35 commits into from
Mar 22, 2018

Conversation

trych
Copy link
Contributor

@trych trych commented Mar 6, 2018

❗️ This merges against the b-less branch

Ok, so finally here's the result of basil.js Coding Sprint day 1 and 2. This branch includes the changes to basil.js' transform behavior as we discussed in our Skype calls. The changes are:

Processing style translate(), rotate() and scale()

All credits for this go to @bohnacker, I retrieved them from his pull request. Using these three functions basil.js behaves like Processing and p5.js now. I gave this quite a bit of testing, it just seems to work.

transform()

A new transform() function was added to allow for matrix independent transforms of page items, as discussed here.

Example of transforms with all possible anchor points:

transform

There was a test script added to easily generate these: test/visual/transform.jsx. Make sure to use this, to familiarize yourself with the transform function and quickly see results.

To take a shortcut, here's a copy-paste of the function's documentation:

/**
 * @description Transforms a given page item. The type of transformation is determinded with the second parameter. The third parameter is the transformation value, either a number or an array of x and y values. The transformation's reference point (top left, bottom center etc.) can be set beforehand by using the <code>referencePoint()</code> function. If the third parameter is ommited, the function can be used to measure the value of the page item.
 * There are 10 different transformation types:
 * – <code>"translation"</code>: Translates the page item by the given <code>[x, y]</code> values. Returns the coordinates of the page item's anchor point as an array.
 * – <code>"rotation"</code>: Rotates the page item to the given degree value. Returns the page item's rotation value in degrees.
 * – <code>"scaling"</code>: Scales the page item to the given <code>[x, y]</code> scale factor values. Alternatively, a single scale factor value can be used to scale the page item uniformely. Returns the scale factor values of the page item's current scale as an array.
 * – <code>"shearing"</code>: Shears the page item to the given degree value. Returns the page item's shear value in degrees.
 * – <code>"size"</code>: Sets the page item's size to the given <code>[x, y]</code> dimensions. Returns the size of the page item as an array.
 * – <code>"width"</code>: Sets the page item's width to the given value. Returns the width of the page item.
 * – <code>"height"</code>: Sets the page item's height to the given value. Returns the height of the page item.
 * - <code>"position"</code>: Sets the position of the page item's anchor point to the given <code>[x, y]</code> coordinates. Returns the coordinates of the page item's anchor point as an array.
 * – <code>"x"</code>: Sets the x-position of the page item's anchor point to the given value. Returns the x-coordinate of the page item's anchor point.
 * – <code>"y"</code>: Sets the y-position of the page item's anchor point to the given value. Returns the y-coordinate of the page item's anchor point.
 *
 * @cat Document
 * @subcat Transformation
 * @method transform
 * @param {PageItem} pItem The page item to transform.
 * @param {String} type The type of transformation.
 * @param {Number|Array} [value] The value(s) of the transformation.
 * @returns {Number|Array} The current value(s) of the specified transformation.
 *
 * @example <caption>Rotating a rectangle by 25 degrees</caption>
 * var r = rect(20, 40, 200, 100);
 * transform(r, "rotation", 25);
 *
 * @example <caption>Measure the width of a rectangle</caption>
 * var r = rect(20, 40, random(100, 300), 100);
 * var w = transform(r, "width");
 * println(w); // prints the rectangle's random width between 100 and 300
 *
 * @example <caption>Position a rectangle's lower right corner at a certain position</caption>
 * var r = rect(20, 40, random(100, 300), random(50, 150));
 * referencePoint(BOTTOM_RIGHT);
 * transform(r, "position", [40, 40]);
 */

Examples:

// Rotating a rectangle by 25 degrees
var r = rect(20, 40, 200, 100);
transform(r, "rotation", 25);
// Measure the width of a rectangle
var r = rect(20, 40, random(100, 300), 100);
var w = transform(r, "width");
println(w); // prints the rectangle's random width between 100 and 300
// Position a rectangle's lower right corner at a certain position
var r = rect(20, 40, random(100, 300), random(50, 150));
referencePoint(BOTTOM_RIGHT);
transform(r, "position", [40, 40]);

The function acknowledges the user's units as well as the current canvas mode.

referencePoint()

A function was added to allow transform() to work around a specific reference point (aka anchor point), as we discussed here.

Constants were added to go along with this function (TOP_LEFT, TOP_CENTER etc.). Alternatively the function can take InDesign anchor point enumerators or digits from 1 to 9 as arranged on a numpad (which refers to InDesign's infamous reference point keyboard shortcut and is a huge time saver during testing).

Removal of legacy functions

All "item functions" (itemX(), itemWidth()etc.) as well as thetransformImage()function were removed, as they are replaced by the newtransform()` function.

Other stuff

The test suit script was updated to work with transform(), changelog.txt was updated. The changes don't break any tests of the testing suite.


@basiljs Could you all give me feedback if this goes along with our plans of how the functions are suppose to work. Preferably asap, so I can make good use of the sprint. Thanks! 😎

@ffd8 In the context of our coding sprint, could you maybe checkout the branch and give this some testing? As I mentioned, there is is the script test/visual/transform.jsx to create pages like the ones you see above. You could just test, if the transform() function works as intended. Also, you could do some testing, if the Processing style translate(), rotate() and scale() really do work like in Processing.

@fabianmoronzirfas Could you give me a quick feedback on how I should format a list in jsdocs? As you can see, I added a list here. Is there a more appropriate way of doing this?

@ffd8
Copy link
Member

ffd8 commented Mar 7, 2018

Bam! fantastic – will give it a test drive tomorrow.
Along the lines of this merge, should we consider adding apush() + pop() that simply mirrors our existing pushMatrix() and popMatrix()? P5js, switched to that style which I'm not a fan of, but would make the new ability of easily porting code from P5js thanks to the b-less upgrade that much smoother.

@trych trych mentioned this pull request Mar 7, 2018
@trych
Copy link
Contributor Author

trych commented Mar 7, 2018

Bam! fantastic – will give it a test drive tomorrow.

Awesome, thanks!

should we consider adding apush() + pop() that simply mirrors our existing pushMatrix() and popMatrix()

Interesting. I opened a new issue about this here with a suggestion of how to implement this. Let's continue the push-pop discussion in this issue.

@trych
Copy link
Contributor Author

trych commented Mar 7, 2018

Added two commits, now transform() should work reliably with all canvas modes and it considers the geometric bounds when measuring.

@ffd8
Copy link
Member

ffd8 commented Mar 8, 2018

Loops w/ push/popMatrix() + translate()/scale()/rotate() all worked exactly as one would expect in Processing.

transform():
I found some of the wording confusing, not sure if the 'ing' and 'tion' are necessary:

  • "shear" vs "shearing"
  • "scale" vs "scaling"
  • "rotate" vs "rotation"?
    = I'd suggest, just simple verb: shear/scale/rotate/translate/

I think transform() should by default grab the current referencePoint as seen in the InDesign GUI ( app.activeWindow.transformReferencePoint ) and the function referencePoint() would set it. It's odd if one uses the GUI/mouse and set it in one position, but the code didn't recognize that until explicitly using the referencePoint() function.. which acts as expected, but didn't update the GUI. Otherwise, it works exactly as expected = great! A tad confusing that all anchors require x_y descriptions except center.. center_center is kind of dumb, but helps anyone following the pattern to guess how the center anchor point works.

@trych
Copy link
Contributor Author

trych commented Mar 8, 2018

Thanks for testing!

I'd suggest, just simple verb: shear/scale/rotate/translate/

Hm, my reason to do it this way was, because we have position and size which I considered nouns, but now that I think about it, they are verbs as well. :) So, I would make the verbs the default behaviour in the docs, but I would like to leave scaling, rotation etc. working n the background. Ok for you?

I think transform() should by default grab the current referencePoint

At first I was against that, because I thought the outcome of the script will be unpredictable, but now I think we do the same with the units and the user's page size. So, yes I agree, I will change that.

center_center is kind of dumb, but helps anyone following the pattern to guess how the center anchor point works

I really would leave this at CENTER. If the user made it that far to start using referencePoint() for their transform(), I am sure they will figure out that it's CENTER or be able to look it up.

@ffd8
Copy link
Member

ffd8 commented Mar 8, 2018

Sure - no problem for me if it has additional ways of saying it.

Re: transform using set referencePoint() – I think this is a different case than units..? Nevertheless, it would be very helpful if we disabled as many style/settings presets on our side that override a typical InDesign user. Especially when someone is adding code to an existing project that has many specific settings.. for a different thread, but I'd propose we try to use as many user settings for things like units, colormode, stroke, fill, .... – and have a defaults() or similar function that we can use in workshops to make sure everyone's document is setup ready for Processing style start (fill = 255, stroke = 0, colorMode = RGB, etc.).

re: CENTER, perhaps we just have an extra CONST for CENTER_CENTER, aliases CENTER? Similar user-friendly logic as verb names above? It's not necessary, but could be helpful.

@trych
Copy link
Contributor Author

trych commented Mar 8, 2018

As I said I would consider the ref point that was set by the user.

Other things as colormode, stroke, fill I would prefer to leave it as it is now. I think that worked well and is in the spirit of Processing and p5 that do the same. If someone uses basil on a existing project, he has to make sure to set stroke and fill to whatever he needs. No need to make everyone else use defaults() all the time just to get consistent behavior. (Also, the existence of a defaults() function would be a bit paradoxical: Why do I need a function to set defaults, if they are defaults? ;) )

@trych
Copy link
Contributor Author

trych commented Mar 8, 2018

@ffd8 Maybe we could have a quick Skype talk? (see my email)

@ff6347
Copy link
Member

ff6347 commented Mar 8, 2018

@fabianmoronzirfas Could you give me a quick feedback on how I should format a list in jsdocs? As you can see, I added a list here. Is there a more appropriate way of doing this?

I guess using html is the way to go for jsdoc

It should be something like this:

/**
* <ul>
* <li> <code>"translation"</code>: Translates the page item by the given <code>[x, y]</code> values. Returns the coordinates of the page item's anchor point as an array.</li>
* <li> <code>"rotation"</code>: Rotates the page item to the given degree value. Returns the page item's rotation value in degrees.</li>
* </ul>
*/

I take a look tomorrow.

ff6347
ff6347 previously requested changes Mar 10, 2018
Copy link
Member

@ff6347 ff6347 left a comment

Choose a reason for hiding this comment

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

I deleted the inline comment so this shows in the conversation history
The list in the docs is working there is just one <li> element missing.

@trych
Copy link
Contributor Author

trych commented Mar 10, 2018

Good spot. Thanks, fixed.
Edit: Ok now after fixing, the "Changes requested" thing is still showing. How do I get rid of this now? Do I need to click "Dismiss review"?

@trych trych dismissed ff6347’s stale review March 22, 2018 11:40

Resolved.

@trych
Copy link
Contributor Author

trych commented Mar 22, 2018

Ok, as discussed via Skype recently, I made the matrix functionalities private and kept only the ones that are needed for the basil methods.

This passes all tests.

This branch is good to go, I will merge it.

@trych trych merged commit 3737546 into b-less Mar 22, 2018
@trych trych deleted the b-less-transform branch March 22, 2018 11:50
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.

3 participants