Skip to content

docs: update documentation on transform function #124

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

Merged
merged 16 commits into from
Jul 8, 2025

Conversation

EscapedGibbon
Copy link
Contributor

No description provided.

@EscapedGibbon EscapedGibbon linked an issue Jul 1, 2025 that may be closed by this pull request
Copy link

cloudflare-workers-and-pages bot commented Jul 1, 2025

Deploying image-js-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2712e93
Status: ✅  Deploy successful!
Preview URL: https://81a0eeeb.image-js-docs.pages.dev
Branch Preview URL: https://123-update-documentation-on.image-js-docs.pages.dev

View logs

@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from 0cf1e5e to 942a235 Compare July 3, 2025 08:35
@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from d41ed2a to 5f0df19 Compare July 3, 2025 08:52
@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from dac82a4 to e23f98b Compare July 3, 2025 09:38
@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from ca032fd to 86dc5b4 Compare July 3, 2025 10:35
@EscapedGibbon EscapedGibbon marked this pull request as ready for review July 3, 2025 10:45
@EscapedGibbon
Copy link
Contributor Author

I have some doubts about some parts of the tutorial that i want to mention @stropitek :

  1. I find the current tutorial practical but it doesn't really explain what image transformation really is in too much detail. Is there a need perhaps to add some explanation of how matrix multiplication in image transformation work?
  2. We haven't really discussed it, but the transform() demo in the features folder remains unchanged. I think the image-js release is needed and perhaps i should add the "getPerspectiveWarp" function afterwards so it can be accessed in the demo?
  3. The image at the beginning of the tutorial shows well how different matrices change coordinates, but it uses a 3x2 matrix, when our function accepts 2x3. Is this too misleading? Should I remove the image or maybe put a note explaining the difference?
  4. I propose to add another page in Useful Tips dedicated to parameters in "transform()" function in a separate commit. I think it is worth explaining more different interpolation types and border handling. It can also explain "fullImage" and "inverse" parameters because I don't think that explanation on "transform()" feature page is enough.
  5. I made a small change in package-json and added exports field so that test-package workflow passes. I am not sure if I did it correctly.

@stropitek
Copy link
Contributor

stropitek commented Jul 3, 2025

Good work on the tutorial!

  1. I find the current tutorial practical but it doesn't really explain what image transformation really is in too much detail. Is there a need perhaps to add some explanation of how matrix multiplication in image transformation work?

You can add a reference if you have a good one, but let's not go into too many mathematical details in those tutorials. They should remain simple.

In the example which multiplies matrices, you can also show how to do the calculation with ml-matrix, and add a link to the library.

2. We haven't really discussed it, but the transform() demo in the features folder remains unchanged. I think the image-js release is needed and perhaps i should add the "getPerspectiveWarp" function afterwards so it can be accessed in the demo?

Yes you can add a new page to "features/geometry". I think an explanation / API description should be enough, it's not easy to do a useful demo for getPerspectiveWarp.

3. The image at the beginning of the tutorial shows well how different matrices change coordinates, but it uses a 3x2 matrix, when our function accepts 2x3. Is this too misleading? Should I remove the image or maybe put a note explaining the difference?

Do you mean the gif at the beginning? If you have done it yourself you can correct it if it's not too much work. If you found it, it is fine you can leave it like that without further explanations.

4. I propose to add another page in Useful Tips dedicated to parameters in "transform()" function in a separate commit. I think it is worth explaining more different interpolation types and border handling. It can also explain "fullImage" and "inverse" parameters because I don't think that explanation on "transform()" feature page is enough.

Yes why not, you can create an issue about this. Would be nice to have a (not necessarily exhaustive) list of methods which have those options. Would be an opportunity to check that they all share the same API as well.

5. I made a small change in package-json and added exports field so that test-package workflow passes. I am not sure if I did it correctly.

This is not a published library so you can revert that change and disable the test-package workflow. Here is workflow's input to configure that: https://github.com/zakodium/workflows/blob/e95b919cfd1754f0e2ca4b98f13445a330a1f71a/.github/workflows/nodejs.yml#L38

@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from 322451d to 8da604b Compare July 4, 2025 12:40
@EscapedGibbon EscapedGibbon force-pushed the 123-update-documentation-on-transform-function branch from 248e989 to 4cadbab Compare July 4, 2025 13:13
@EscapedGibbon
Copy link
Contributor Author

  • I have added the explanation for matrix multiplication in image transformations, but i feel like the part about "ml-matrix" is out of place here. Maybe i misunderstood what you meant.

@stropitek
Copy link
Contributor

@EscapedGibbon as it is really difficult to have a working state for the build when updating any dependency, I reverted to the package-lock of the main branch.

We don't need to update image-js since the new features are only documented, not used in the demos.

Let's handle deps update in a future PR.

@EscapedGibbon
Copy link
Contributor Author

@EscapedGibbon as it is really difficult to have a working state for the build when updating any dependency, I reverted to the package-lock of the main branch.

We don't need to update image-js since the new features are only documented, not used in the demos.

Let's handle deps update in a future PR.

Then I only modify the part about ml-matrix as was discussed. I made it as a dropdown link, to not to overcrowd the documentation.

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

I thought from our discussions that you would update this example to show how the matrix could also be computed using mljs by multipling a translation and a rotation together.

Otherwise LGTM.

@EscapedGibbon
Copy link
Contributor Author

I thought from our discussions that you would update this example to show how the matrix could also be computed using mljs by multipling a translation and a rotation together.

Otherwise LGTM.

Yeah, you're right, I think it can be combined into one section.

@EscapedGibbon EscapedGibbon merged commit ac88bd0 into main Jul 8, 2025
8 checks passed
@EscapedGibbon EscapedGibbon deleted the 123-update-documentation-on-transform-function branch July 8, 2025 09:51
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.

add tutorial on transform() function
2 participants