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

This patch adds support for custom ARArtifact stores #123

Merged
merged 11 commits into from Jun 10, 2019

Conversation

Projects
None yet
2 participants
@mmocny
Copy link
Collaborator

commented Jun 6, 2019

Developer can now provide a artifactStores: config parameter. Among other things, this will enable support for custom datastores, including cloud services for artifacts. Possibly useful if you have thousands of results to maintain.

If/when there is a popular ARArtifact web service API available, we could offer a client ArtifactStore for it.

This patch fixes Issue #34, adds to #87, and makes Issue #38 even more relevant.

@mmocny mmocny requested a review from paullewis Jun 6, 2019

@paullewis
Copy link
Collaborator

left a comment

LGTM with nits

@@ -126,7 +126,7 @@
evt.preventDefault();
const card = new Elements.Card();
card.src = found[0].content;
card.src = found[0].artifact.arContent;

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

Does this mean we're now exposing the structured data more directly?

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

Only trivially. Previously, NearbyResult would have a content property, but this was always just set to artifact.arContent automatically. The actual schema was all the same. So now the client more directly sees that content comes from the artifact schema -- which I think is a good change.

If we want to help go from returning the raw Schema to returning a more structured result, we should change the return type more dramatically. But I think that in general its good to give the client back the schema exactly as it was given to us.

@@ -1,15 +1,25 @@
# Simple Config Demo

This demo showcases the best way to load multiple artifact sources into the Perception Toolkit. See [`index.html`](./index.html) for details.
This demo showcases how to load multiple ARArtifact definitoons into the Perception Toolkit, all at once.

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

nit: definitions

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

Thank yoou

async getDetectableImages() {
return [];
}
async findRelevantArtifacts(nearbyMarkers, geo, detectedImages) {

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

I feel like this function, maybe the class overall could take a few comments about what it's doing?

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

Ack.

Since this demo isn't testing detectable images, and since I don't want to explain how to create image targets here -- I changed the interface to support these functions as optional.

Then, I can drop "getDetectableImages" as a distraction here. We can update or make a separate demo one day.


export { NearbyResult };

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

You can export directly if that's helpful? This reads a little oddly, and I wonder if it would actually create a dupe under the hood.

export { NearbyResult } from './stores/artifact-store.js';

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

But I need it imported locally, and doing just export doesn't also import it for use in this file.

I assumed that if it's already imported it was better to just export the local -- WDYT? No strong preference here, no idea what the conventions are.

return [
...this.markerStore.findRelevantArtifacts(nearbyMarkers),
...this.imageStore.findRelevantArtifacts(detectedImages),
];
}

getDetectableImages(): DetectableImage[] {
async getDetectableImages(): Promise<DetectableImage[]> {

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

Are we sticking with getDetectableImages() or are we moving to a more generic name?

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

In the refactor work which is coming up, there will be a wrapper for detectableImages, and other things like it (i.e. give me everything I need for next frame).

But the images portion will still look like this. I don't want to make the change in this patch.

assert.lengthOf(results, 1);
});

it('finds multiple', () => {
const results = localArtifactStore.findRelevantArtifacts(
it('finds multiple', async () => {

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 7, 2019

Collaborator

feels like it needs to clarify multiples of what

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 7, 2019

Author Collaborator

Ack.

mmocny added some commits Jun 7, 2019

@mmocny mmocny merged commit c3c11de into master Jun 10, 2019

5 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mmocny mmocny deleted the rest_support branch Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.