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

Emit events when a marker is found and lost, when using aframe's <a-marker> #303

Merged
merged 9 commits into from Sep 24, 2018

Conversation

Projects
None yet
9 participants
@nikolaymihaylov
Copy link
Contributor

nikolaymihaylov commented Jan 31, 2018

What kind of change does this PR introduce?

New feature

Can it be referenced to an Issue? If so what is the issue # ?

#217

How can we test it?

Check the aframe/examples/marker-events.html that I created.

Summary

When an <a-marker> is found or lost, there is currently no event emitted (as far as I know), to "the outside world". In other words, currently an application that is using AR.js cannot react to the marker becoming visible or invisible.

With my addition, markerFound and markerLost  events will be emitted when the marker changes its visible state. The events are only emitted for <a-marker> elements which have emitevents='true' set. emitevents defaults to false, so there will be no additional events emitted for any of the existing users of the AR.js library.

Does this PR introduce a breaking change?

No.

Other information

I also created a new build, and updated some README and CHANGELOG files. If these should be excluded from the pull request, let me know and I could make the adjustments.
EDIT: The build files are no longer included in this pull request.

@peterlunglum

This comment has been minimized.

Copy link

peterlunglum commented Apr 25, 2018

@nikolaymihaylov this is great! How can I download this branch to use in my project?

@nikolaymihaylov

This comment has been minimized.

Copy link
Contributor

nikolaymihaylov commented Apr 25, 2018

Hey @peterlunglum,

You can download my forked version git@github.com:nikolaymihaylov/AR.js.git and then do a git checkout tags/1.5.2 for the changes that are currently present in this pull request.

Keep in mind that this pull request has already diverged a lot with the original repository, so by choosing to use my forked version, you won't be getting other improvements or bugfixes for AR.js.

The proper solution would be to get this pull request merged to the main repository. I do not know what its maintainers think about my changes though.

@AndiDomi

This comment has been minimized.

Copy link

AndiDomi commented May 1, 2018

@nikolaymihaylov A really great job! I just tried it and it works really well with Hiro marker and Kanji, but I cannot make it work with a custom marker for example like

<a-marker preset="custom" type="pattern" url="./332A.patt" id="332-A" registerevents>
</a-marker>

Am I doing something wrong or is just that custom markers are not supported on your branch?

Update: when you apply this commit to your branch it works perfectly :)

@nikolaymihaylov

This comment has been minimized.

Copy link
Contributor

nikolaymihaylov commented Jun 19, 2018

Hey, there is an ongoing effort to sync this branch with the latest dev at: nikolaymihaylov#1 Thanks @MaesterChestnut for the effort!

We have an issue with it though. If someone has time to take a look, your help is appreciated!

@leeprobert

This comment has been minimized.

Copy link

leeprobert commented Jun 27, 2018

+1 on this :-)

@nicolocarpignoli

This comment has been minimized.

Copy link
Collaborator

nicolocarpignoli commented Jul 4, 2018

@nikolaymihaylov can you please resolve conflicts first?

@MaesterChestnut

This comment has been minimized.

Copy link

MaesterChestnut commented Jul 4, 2018

@nikolaymihaylov the PR I have to your branch (which does not have these merge conflicts) now works as expected. If you merge my PR to your branch, this PR should be ready for merging

@nicolocarpignoli

This comment has been minimized.

Copy link
Collaborator

nicolocarpignoli commented Jul 9, 2018

@MaesterChestnut and @nikolaymihaylov this is a big feature in my opinion. What needs to be done to make this ready to merge?

@MaesterChestnut

This comment has been minimized.

Copy link

MaesterChestnut commented Jul 10, 2018

@nikolaymihaylov please my PR into yours or else I'll need to open a separate PR for this feature

@nikolaymihaylov

This comment has been minimized.

Copy link
Contributor

nikolaymihaylov commented Jul 17, 2018

The branch in this pull request is now brought up-to-date. Thanks again @MaesterChestnut for your effort in the pull request nikolaymihaylov#1

I have now excluded the build files from the pull request, which will hopefully make this easier to merge. That means that for anyone using my latest dev branch, you'll need to run the make minify as described in https://github.com/jeromeetienne/AR.js#how-to-release- .

If anyone has based their applications on this pull request, you can still use the tag 1.5.2 that I had created, as described in #303 (comment), which has the old build files. That is, of course, not recommended, since the code is outdated by now.

@nicolocarpignoli

This comment has been minimized.

Copy link
Collaborator

nicolocarpignoli commented Jul 23, 2018

Can someone tests this? This is a great feature that solves several issues

@derzu

This comment has been minimized.

Copy link

derzu commented Sep 4, 2018

I hope this branch be merged. But while it is not, there is a short solution is:

At your HTML:

<a-marker preset='hiro' id="amarker" ref="amarker">

Create a method to verify the marker state:

verifyMarker() {
    //var amarker = this.$refs.amarker; // vuejs
    var amarker = document.querySelector("#amarker")
    if(amarker.object3D.visible == true) {
        console.log('marker is visible');
    }
    else {
        console.log('marker is lost');
    }
}

At some point start your verification:

setInterval(this.verifyMarker, 1000); // 1000 means 1 second

@LaurenceNairne

This comment has been minimized.

Copy link

LaurenceNairne commented Sep 20, 2018

Any plans to test and merge this pull request? It seems pretty robust and it'll do wonders for making AR.js more useful.

@nicolocarpignoli

This comment has been minimized.

Copy link
Collaborator

nicolocarpignoli commented Sep 21, 2018

HI @nikolaymihaylov, I will test it myself. I only ask you if you can give me a builded version of your AR.js so I can directly import it on my example and test it. Something like: <script src="https://jeromeetienne.github.io/AR.js/aframe/build/aframe-ar.js"></script> will be great :) Thanks a lot

@jeromeetienne
Copy link
Owner

jeromeetienne left a comment

thanks a lot for your contributions @nikolaymihaylov, im making comments here but you dont have to do it. i can do the modification myself after the merge

@@ -34,6 +34,12 @@ AFRAME.registerComponent('arjs-anchor', {
type: 'number',
default: 0.6,
},

This comment has been minimized.

@jeromeetienne

jeromeetienne Sep 24, 2018

Owner

(thanks a lot for your contributions @nikolaymihaylov, im making comments here but you dont have to do it. i can do the modification myself after the merge)

this seems harmless to emit the event. it is a rare event so no performance impact. we can remove this options for the sake of simplicity

}else if( !_this.el.object3D.visible && wasVisible ){
_this.el.emit('markerLost')
}
}

This comment has been minimized.

@jeromeetienne

jeromeetienne Sep 24, 2018

Owner

Nice standalone trick!

what about the 'cameraTransformMatrix' case tho ?

@@ -212,6 +227,7 @@ AFRAME.registerPrimitive('a-marker', AFRAME.utils.extendDeep({}, AFRAME.primitiv
'preset': 'arjs-anchor.preset',
'minConfidence': 'arjs-anchor.minConfidence',
'markerhelpers': 'arjs-anchor.markerhelpers',
'emitevents': 'arjs-anchor.emitevents',

This comment has been minimized.

@jeromeetienne

jeromeetienne Sep 24, 2018

Owner

to be removed, as seen above

@jeromeetienne jeromeetienne merged commit 3f39bfb into jeromeetienne:dev Sep 24, 2018

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