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

Refactor MM: `updatePerceptionState` once per-frame #134

Merged
merged 18 commits into from Jun 21, 2019

Conversation

Projects
None yet
2 participants
@mmocny
Copy link
Collaborator

commented Jun 14, 2019

I'm still doing some manual tests for this refactor, but the simple demo cases work for me, and the console logs seem to make sense.

Since this ended up being more beefy than expected, I wrote up a big picture summary (below). If need be, I can break this up into cleaner parts, since I now know which parts where necessary for what.

Significant Changes:

  • Bugfix: PT previously never fired events for "lost" content. (purgeOldMarkers cleaned up but didn't fire)
  • Refactor: MM replaces all Found/Lost calls with an updatePerceptionState call.
  • Feature: MM now returns all ProbableTargets and does so every frame, instead of supplying just detectableImages and just on startup.
    • (ProbableTargets still just has images for now, but we can add any params needed for detectors.)
    • However, PT only calls prepareForNextFrame once on startup. I didn't want to do per-frame until I ensure efficient image loading per-frame. Future patch.
  • Feature: MM now tracks URLs it indexed, so it won't index them again. This also fixed a bug w/ dynamic URL markers leading to duplicate artifacts found (if that artifact was already indexed on startup via tests/config).
  • Refactor: Moved "timeout" tracking from PT to MM. That moved "new target" tracking to MM. Events still fire from PT for now.
  • Refactor: ArtifactDealer removed all state tracking since (it gets all context at once now), and so I moved "diff" code up to MM (which does track state, for timeouts).
  • Refactor: DetectedImages are no longer shoehorned into Markers (e.g. in PT, in planar image detector, and in MM).
    • this simplified a few code paths, and complicated others (e.g. now have similar/duplicate code in a few places). Filed a bug for implementing a common base-class for these things, which is clean that up.
  • Tests: Updated lots due to changes, but nothing significant.
  • (Sorta?) Breaking Change: If multiple new markers are found in one frame, but we only partially get found results, we have at one unknown marker -- but don't know which one. Today, we already don't put up a card for unknown marker if there is at least 1 content result, anyway, so this change is not visible. But we should resolve the ambiguity (ideas below)
  • Feature: create multiple unknown result cards if maxCards > 1

Questions:

  • Dealing with lastSeenTimeBuffer in tests -- for now I just override to 0ms. WDYT? Should we fake the clock, mock out performance.now?

Future changes?

  • call prepareForNextFrame every frame.
  • Artdealer::getPerceptionResults to include the perception state used to lead to a specific result.
  • Maybe change the way we track "new targets" by instead having MM return all: along found: and lost:. Seems useful anyway, and this way we can infer whats new (once PerceptionResult includes original target alongside)

mmocny added some commits Jun 10, 2019

WIP: ArtifactDealer now idempotent; no state diffs
Moves the "diff" that generates found/lost into MM, in preparation for
time-based found/lost, rather than just doing direct state diffs.

Also fixes issues #48, since tests were failing on the same dynamic
URL's being loaded multiple times causing artifacts to be loaded
multiple times.

Fixes a bunch of tests.

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

@mmocny mmocny self-assigned this Jun 14, 2019

@mmocny

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

Ran a few more manual tests (all good) and fixed demo/ to work with changes.

Show resolved Hide resolved src/artifacts/artifact-dealer.ts Outdated
Show resolved Hide resolved src/artifacts/artifact-dealer_test.ts Outdated
Show resolved Hide resolved src/artifacts/stores/local-image-store.ts Outdated
Show resolved Hide resolved src/elements/meaning-maker/meaning-maker.ts Outdated
Show resolved Hide resolved src/elements/meaning-maker/meaning-maker.ts
Show resolved Hide resolved src/elements/meaning-maker/meaning-maker_test.ts
Show resolved Hide resolved src/elements/perception-toolkit/perception-toolkit.ts Outdated
@mmocny
Copy link
Collaborator Author

left a comment

Alright, changes made. Tests pass. At this point I think it's ready to land, please take a look.

Will do a few more last-minute manual tests on all the demo/ pages.

@paullewis
Copy link
Collaborator

left a comment

It looks good to me. Before committing can you try the demos? I know we're due to add tests here, but at least taking a manual pass prior would boost confidence.

private prevPerceptionResults = new Set<PerceptionResult>();
private artifactsForUrl = new Map<string, ARArtifact[]>();
private lastSeenMarkers = new Map<string, { marker: Marker, timestamp: number }>();
private lastSeenImages = new Map<string, { image: DetectedImage, timestamp: number }>();

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 21, 2019

Collaborator

Just pondering how this expands should we have other detectors.... would a lastSeenItem map make sense?

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 21, 2019

Author Collaborator

Discussed offline, but:
Yes, this would be my preferred way to do it. I could even do that now.

The problem is that later on I would have to split Items out based on type, and there is no clean way to do this without assuming types based on existence of specific props. Since there are only two types for now, I just left it distinct all the way through.

There is issue #115 which addresses the question of merging these back into one using a single Target/Item wrapper.

}
return [];
const artifacts = await this.artloader.fromUrl(url);

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 21, 2019

Collaborator

nit: blank line

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 21, 2019

Author Collaborator

Add a blank line?

this.lastSeenMarkers.delete(markerId);
}
}
for (const [imageId, { timestamp }] of this.lastSeenImages.entries()) {

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 21, 2019

Collaborator

Again, I wonder how this goes for n detectors.

private readonly onVisibilityChangeBound = this.onVisibilityChange.bind(this);
private readonly onMarkerFoundBound = this.onMarkerFound.bind(this);
private readonly onCaptureFrameBound = this.onCaptureFrame.bind(this);
private readonly onCloseBound = this.onClose.bind(this);
private readonly startupDetections: Array<Promise<Marker[]>> = [];
private readonly startupDetections: Array<Promise<object[]>> = [];

This comment has been minimized.

Copy link
@paullewis

paullewis Jun 21, 2019

Collaborator

Can this not be

const startupDetections: Array<Promise<Array<{}>>> = [];

or

const startupDetections: Promise<Array<{}>>[] = [];

This comment has been minimized.

Copy link
@mmocny

mmocny Jun 21, 2019

Author Collaborator

I get Array type using 'T[]' is forbidden for non-simple types. Use 'Array<T>' instead. (array-type) warning when I replace top-level Array<>

But I switched to Array<{}> inside

Show resolved Hide resolved src/elements/perception-toolkit/perception-toolkit.ts Outdated

@mmocny mmocny merged commit 18378ca into master Jun 21, 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
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.