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

Incorporate getters and setters for points and add docs #6

Closed
wants to merge 8 commits into from

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Jul 20, 2019

Addresses #3 #4 and (in a bit) #2

cc @jywarren

@rexagod
Copy link
Member Author

rexagod commented Jul 20, 2019

Also, @jywarren I've compared this branch with the core-experimental one which I'll merge into main once this PR is cleared. I did it so you can easily review the diffs.

@jywarren
Copy link
Member

That's awesome @rexagod I'm so pleased!!!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Super easy to review, thank you so much!!! Congrats, this is really great.

README.md Show resolved Hide resolved
README.md Outdated
{"x":84,"y":331}, ...]
```

`matching` is done with the `findMatchedPoints` function, passes a `matchesArray` to the global `utils` object's `getMatchedPoints` object property with the following format:
Copy link
Member

Choose a reason for hiding this comment

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

This is a little hard to follow, could you add a code example showing the function in use? Thanks!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool! I've added code links to those functions since they were kind of lengthy. But let me know if you want me to copy them into the docs. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Just an example of findMatchedPoints being run and the parameters for it would be great.

@rexagod
Copy link
Member Author

rexagod commented Jul 20, 2019

@jywarren Up for your review. 😄

@jywarren
Copy link
Member

Just one request then this should be good to go!

@jywarren
Copy link
Member

Hi, are there any updates on this? Thank you!

@rexagod
Copy link
Member Author

rexagod commented Jul 26, 2019

@jywarren Have a look!

README.md Outdated
```
It runs slower than the point `finding` step due to the added computational overhead of comparing both of the images for matches.

The [`findMatchedPoints`](/src/orb.core.js#L241) function depends upon the values served back into its lexical scope by the [`findPoints`](/src/orb.core.js#) function, which in turn depends upon the `params` argument (see below) supplied by the user, and is solely responsible for the generation of the [`cornersArray`](/src/orb.core.js#233), which is used to instantiate the [`matchesArray`](/src/orb.core.js#L269). The [`findMatchesPoints`](/src/orb.core.js#L241), is [called here](/src/orb.core.js#L302) and the appropriate values are set [in the cache](/src/orb.core.js#L316).
Copy link
Member

Choose a reason for hiding this comment

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

Oops maybe a typo on findmatchespoints here?

Also I really do think a full example here would be ideal in addition to the linked code. It just helps explain each parameter. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

if (findPoints()) {

@jywarren The findMatchedPoints function doesn't take any values explicity, those are served to it owing to the findPoints method. The function, however, depends specifically on args.browser that's initialized all the way at the top. Do you want me to highlight this?

args.browser = window !== undefined ? true : false;

Also, every parameter has been explained in the codeflow section, along with their line addresses as well.

https://github.com/publiclab/matcher-core/blob/c3a5b8a6fccefc5fe7ad521eb40e25a8d9059551/README.md#codeflow

So do you want me to highlight the fact and maybe link a few lines about the working of findMatchedPoints (other than the text above) or modify something else? Sorry for taking your time on this PR, just wanted to be sure we are on the same page here!

Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

The findMatchedPoints function doesn't take any values explicity, those are served to it owing to the findPoints method.

Hm, why doesn't it accept two points parameters, between which it finds matches? Wouldn't that make more sense to a user? Like, it would take two points collections as inputs, and output those which are matched between? If this would take extra changes, let's do that in a follow-up PR. Can you open an issue and copy these notes in? Thanks!

The function, however, depends specifically on args.browser that's initialized all the way at the top. Do you want me to highlight this?

Yes, that doesn't seem so clear to me so it may need explaining.

So, Below is a summary of each component of the orbify instance doesn't clearly spell out that these are parameters that can be submitted in the constructor. I think it's very important to literally show verbatim how to use each of the constructors, as in this example:

image

That'll make it much easier for people to start using this library, or to use it in unexpected ways!

Thank you, @rexagod -- much appreciated!

@rexagod
Copy link
Member Author

rexagod commented Aug 1, 2019

@jywarren I did a sweep of the branches avoid confusion and am shifting this PR's mergability from core-experimental (deleted branch) to main. Will continue on #10 after that. 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.

None yet

2 participants