-
Notifications
You must be signed in to change notification settings - Fork 209
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
rotate module #1218
rotate module #1218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
- Coverage 32.07% 32.05% -0.02%
==========================================
Files 107 107
Lines 1986 1987 +1
Branches 297 296 -1
==========================================
Hits 637 637
- Misses 1349 1350 +1
|
@jywarren @harshkhandeparkar can you please guide me how to fix the issues given by codeclimate |
I never fixed them till date. They will require a separate dedicated PR.
don't worry about it, they can be ignored for now.
…On Tue, 20 Aug, 2019, 10:14 PM keshav234156, ***@***.***> wrote:
@jywarren <https://github.com/jywarren> @harshkhandeparkar
<https://github.com/HarshKhandeparkar> can you please guide me how to fix
the issues given by codeclimate
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1218?email_source=notifications&email_token=AIJI5H623C5Y3JKBI62BNE3QFQNQRA5CNFSM4INZPRVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4W5RTY#issuecomment-523098319>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIJI5H5M7W3IWL7UIWUTQ2LQFQNQRANCNFSM4INZPRVA>
.
|
Yeah just make sure that the new code you add is refactored.
…On Tue, Aug 20, 2019, 10:14 PM keshav234156 ***@***.***> wrote:
@jywarren <https://github.com/jywarren> @harshkhandeparkar
<https://github.com/HarshKhandeparkar> can you please guide me how to fix
the issues given by codeclimate
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1218?email_source=notifications&email_token=AGKQ7SHUF2Y74BKJQK2C3JTQFQNQRA5CNFSM4INZPRVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4W5RTY#issuecomment-523098319>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGKQ7SG6UOEMG7DLP56ZAR3QFQNQRANCNFSM4INZPRVA>
.
|
@harshkhandeparkar @jywarren can you please review the changes!! |
Nice work! I am a bit busy as of now. I guess I will review it tmrw. I'll
have to test it out locally. Slowly but surely.
…On Tue, 20 Aug, 2019, 10:51 PM keshav234156, ***@***.***> wrote:
@harshkhandeparkar <https://github.com/HarshKhandeparkar> @jywarren
<https://github.com/jywarren> can you please review the changes!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1218?email_source=notifications&email_token=AIJI5H3MWNOPO2MRO5CDGGLQFQR3LA5CNFSM4INZPRVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4XBCTA#issuecomment-523112780>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIJI5H7MNUH3RA37GZTRNQDQFQR3LANCNFSM4INZPRVA>
.
|
@harshkhandeparkar can you please review if you are free |
I am extremely sorry. I have a test on this Wednesday. I will only be able
to review on that day. I will do it as soon as possible.
…On Mon, 26 Aug, 2019, 10:59 PM keshav234156, ***@***.***> wrote:
@harshkhandeparkar <https://github.com/HarshKhandeparkar> can you please
review if you are free
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1218?email_source=notifications&email_token=AIJI5H5FS6LPUHLWLDJQY73QGQHHDA5CNFSM4INZPRVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5FB56A#issuecomment-524951288>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIJI5HYD4MSHLAIDTEE56CDQGQHHDANCNFSM4INZPRVA>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfect. Only one thing. Is it possible to make the code more readable? You could start by splitting the file into two - Module.js and Rotate.js (take inspiration from other modules)
@keshav234156
@harshkhandeparkar Splitting the file inti Module.js and Rotate.js would make the code easy to understand and i think there won't be any issue.So will make change and push it asap. |
@jywarren @harshkhandeparkar have splitted it into 2 files.please have a look |
Then why is it done again in Rotate.js?
…On Mon, 2 Sep, 2019, 11:00 AM keshav234156, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/modules/Rotate/Module.js
<#1218 (comment)>
:
> -
- if (rotate_value % 360 == 0)
- return pixels;
-
- var bitmap = new imagejs.Bitmap({ width: pixels.shape[0], height: pixels.shape[1] });
- bitmap._data.data = pixels.data;
-
- var rotated = bitmap.rotate({
- degrees: rotate_value,
- });
- pixels.data = rotated._data.data;
-
+ var radians = 3.141592653589793 * rotate_value / 180;
+ var width = pixels.shape[0];
+ var height = pixels.shape[1];
+ var cos = Math.cos(radians);
@harshkhandeparkar <https://github.com/HarshKhandeparkar> i think these
would still be required as we are returning pixels2 from the Rotate.js ,so
it must be initialised before passing it as argument in module.js.
What do you think about this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1218?email_source=notifications&email_token=AIJI5HYLTWWPMWVDBWJ7O7DQHSQHNA5CNFSM4INZPRVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDKS32Q#discussion_r319810336>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIJI5HZXIRX4AW7KEDM7CTLQHSQHNANCNFSM4INZPRVA>
.
|
@publiclab/is-reviewers @jywarren can you please review the PR |
The work done is really great. Thanks. Can you document the flow so that it becomes easy to understand. |
@aashna27 have a look.. |
@jywarren Have a look |
Apologies, @keshav234156 i was in a week of meetings, I'm looking now! First, i'll update to sync with the latest main branch. Then will look through the code. Thanks for your patience, we're still recovering a bit from our Summer of Code wrap-up! |
src/modules/Rotate/Rotate.js
Outdated
|
||
if (rotate_value % 360 == 0) | ||
return pixels; | ||
function copyPixel(x1, y1, x2, y2){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, couldn't this get extra pixels1, pixels2
parameters and then we'd have only one function instead of 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, would do the changes to make a 1 function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jywarren any other changes that are required??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jywarren Had done the changes to make a single function instead of 2
var bitmap = new imagejs.Bitmap({ width: pixels1.shape[0], height: pixels1.shape[1] }); | ||
bitmap._data.data = pixels1.data; | ||
|
||
var rotated = bitmap.rotate({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so why do we need to do so much pixel copying and rotation math if we are still using the Imagejs Bitmap function to perform the rotation? Perhaps some comments would help clarify what's going on here? Sorry, it's just a little hard to follow! Thanks for your help, i love the output, just being sure I understand what's happening inside as well :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
earlier if we use bitmap on original image then it cropped some of the image.to solve this i have put the image on a larger frame ie is square so that there is no problem of clipping,rotated the larger frame image with the help of imagejs rotation function and then extra white is cropped according to to final image dimensions after rotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pixel1 is a square frame that contain original image at it's centre.then we rotated the pixel1 with help of imagejs,at the last we copied pixels from pixel1 to pixel2 so that there is no extra whitespace.
@jywarren if there any other changes or method in your mind please let me know!!.would love to solve this issue. |
Looks great. Thanks so much! |
* this PR solves the problem of clipping of the image while rotating * a little clipping was still present .so made the changes * Splitted into two files * ommitted extra declared variables * Update Module.js * Update Rotate.js * Update Rotate.js
Fixes #1137
this PR fixes the clipping of image while using rotate module
image at 30
image at 90
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/is-reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!