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

add getNeighbourPixel for dynamic module #204

Closed
wants to merge 3 commits into from

Conversation

tech4GT
Copy link
Member

@tech4GT tech4GT commented Mar 22, 2018

This is done by defining logic in the dynamic module, @jywarren please have a look
Signed-off-by: tech4GT varun.gupta1798@gmail.com

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Mar 28, 2018

@jywarren is there anything else that should be added to this, we are not using the extra parameter anymore


/* neighbourpixels can be calculated by

this.getNeighbourPixel.fun(x,y)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just one more! could we go with the American English spelling of Neighbor here? OR alias it so we can use both? 👍

And just curious, sorry if it's a silly question but why does getNeighborPixel have to be an object -- can't it just be getNeighborPixel()?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren actually i have made it an object because pixelmanipulation remaps the options.getNeighbourpixel.fun to a new function which internally calls the original getneighbourpixels with the value of pixels it gets by closure. This remapping won't be possible if it's a function since we can't just say

 options.getneighbourspixel = something else

And it feels kind of natural to use the same function defined in the same file
To avoid this we have 2 options either we move all the logic inside pixelmanipulation or use a different name than getneighbourpixels for actual function that will be called

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren i just opened another pull request doing it have getNeighbourPixel as function, #207 you can merge that if that looks more natural to you, thanks😁

Copy link
Member

Choose a reason for hiding this comment

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

OK I left a comment on #207 -- that does look good!

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Mar 30, 2018

Closing this in favour of #207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants