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

Updated some JsDoc definitions #5578

Merged
merged 20 commits into from
Feb 12, 2022
Merged

Updated some JsDoc definitions #5578

merged 20 commits into from
Feb 12, 2022

Conversation

Gaweph
Copy link
Contributor

@Gaweph Gaweph commented Jan 22, 2022

  • Minor fixes to JsDoc comments that are preventing typescript generation
  • Added Method overload for applyMatrix() - added support for 4x4

(JsDoc definition are used in the generation of the typescript files @ https:/p5-types/p5.ts used by https:/DefinitelyTyped/DefinitelyTyped)

@welcome
Copy link

welcome bot commented Jan 22, 2022

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@Gaweph Gaweph closed this Jan 22, 2022

/**
* @method applyMatrix
* @param {Number} a_4x4 numbers which define the 4x4 matrix to be multiplied
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rationale:

p5.RendererGL.applyMAtrix(arguments) is called from this method which then expects either 6 or 16 values

p5.RendererGL.prototype.applyMatrix = function(a, b, c, d, e, f) {
  if (arguments.length === 16) { // 4x4 matrix
    p5.Matrix.prototype.apply.apply(this.uMVMatrix, arguments);
  } 
};

@Gaweph Gaweph changed the title Removed unused parameters and updated JsDoc comments Updated some JsDoc definitions Jan 28, 2022
@Gaweph
Copy link
Contributor Author

Gaweph commented Jan 31, 2022

@limzykenneth Is there anything further needed for this PR?

src/core/transform.js Outdated Show resolved Hide resolved
@limzykenneth
Copy link
Member

It would be great to have a graphic like the one currently used for the 2x3 matrix in the description for the 4x4 matrix as well.

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 1, 2022

It would be great to have a graphic like the one currently used for the 2x3 matrix in the description for the 4x4 matrix as well.

I see what you mean. Are you refering to this page:
https://p5js.org/reference/#/p5/applyMatrix

I shall add a graphic to the 4x4 which will mean that the param descriptions are not needed :)

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 1, 2022

@limzykenneth 4x4 image added. I think this will work fine without any param descriptions, the images highlight what the properties are for :)

Let me know if you think we need anything further on this

@limzykenneth
Copy link
Member

I would still prefer the text description to be present.

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 4, 2022

I would still prefer the text description to be present.

@limzykenneth OK I think it now has the best of all options. There is an example image for both 6 and 16 array sizes. Each overload has clear description and param names aren't reused so there is no confusion regarding what neds to be passed into each method.

Let me know if you think anything further is needed

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 7, 2022

@limzykenneth Do you ned any further changes on this? Once this PR goes in I can generate the new p5 definitelytyped d.ts definitions

src/core/transform.js Outdated Show resolved Hide resolved
@@ -24,13 +24,41 @@ import p5 from './main';
* > <img style="max-width: 150px" src="assets/transformation-matrix.png"
* alt="The transformation matrix used when applyMatrix is called"/>
*
* <img style="max-width: 150px" src="assets/transformation-matrix-4-4.png"
Copy link
Member

Choose a reason for hiding this comment

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

150px width is a bit too small for this matrix. You can preview this using grunt yui:dev.

@limzykenneth
Copy link
Member

Almost there, just one more thing.

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 8, 2022

Almost there, just one more thing.

@limzykenneth
Increased size, you are right, was way too small :)

@Gaweph
Copy link
Contributor Author

Gaweph commented Feb 10, 2022

Almost there, just one more thing.

@limzykenneth Ping. Are we happy with this now sir?

@limzykenneth limzykenneth merged commit f66d2c5 into processing:main Feb 12, 2022
@limzykenneth
Copy link
Member

Looks good. Thanks!

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