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 three scenes for link traversal #1575

Merged
merged 1 commit into from
Jun 30, 2017
Merged

add three scenes for link traversal #1575

merged 1 commit into from
Jun 30, 2017

Conversation

dmarcos
Copy link
Member

@dmarcos dmarcos commented Jun 23, 2016

Description:
Creates a showcase to illustrate link traversal, characteristic of the WebVR 1.0 spec. A hover and click effects for the links are still pending.

@cvan I know you have opinions on how pages should handle link traversal while remaining in VR. Let's discus and incorporate those ideas into this PR.

This is a link to the demo if you want to quickly try it out:
http://swimminglessonsformodernlife.com/aframe/examples/showcase/link-traversal/

<a-entity id="sky"
geometry="primitive: sphere; radius: 65;"
material="shader: skyGradient; colorTop: #353449; colorBottom: #BC483E; side: back"></a-entity>
<a-entity ground='url: https://media.aframe.io/link-traversal/models/groundSunrise.json'></a-entity>
Copy link
Contributor

Choose a reason for hiding this comment

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

why single ticket marks?

Copy link
Member Author

Choose a reason for hiding this comment

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

a problem of having different conventions for different contexts. JS vs HTML

@ngokevin ngokevin added this to the 0.3.0 milestone Jul 28, 2016
@dmarcos dmarcos force-pushed the link branch 2 times, most recently from 4bcf46b to 599a838 Compare August 9, 2016 01:22
@ngokevin ngokevin modified the milestones: 0.4.0, 0.3.0 Aug 11, 2016
@ngokevin ngokevin removed this from the 0.4.0 milestone Oct 24, 2016

module.exports.Component = registerComponent('link', {
schema: {
url: { default: '' },
Copy link
Member

Choose a reason for hiding this comment

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

Maybe href to be consistent with the primitive that is abstracting it as with as with <a>?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, please

Copy link
Contributor

@cvan cvan Nov 15, 2016

Choose a reason for hiding this comment

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

or, rather can we call the component href? I know it conflicts with the aframe-href-component. another approach I thought about, in keeping with the behaviour of 2D/classic anchors: perhaps we could actually wrap the entities in <a> elements. any emitted click would bubble up to the <a> and handle the page navigation (i.e., modifying window.location from JS). thoughts?

Copy link
Member

@ngokevin ngokevin Nov 15, 2016

Choose a reason for hiding this comment

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

Wrapping in <a> isn't great. It doesn't fit with the framework, is syntactically more complicated (both in HTML, and you can't just synchronously do setAttribute('link', ..>) or use mixins/aframe-mss/inspector), and will cause tons of complications in introducing a non-A-Frame element into the scene graph.

No strong preference for href/link, but href might be confusing because people will be passing a URL string to it, when it needs both an href and an event name (href="href: blah.com; on: grab).

Copy link
Contributor

@cvan cvan Nov 15, 2016

Choose a reason for hiding this comment

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

I know A-Frame is entering (or reentering, heh) unchartered territory. FWIW, per the WHATWG's HTML Living Standard, the href attribute is defined as such:

This implied hyperlink has no special meaning (it has no link type) beyond linking the element's document to the resource given by the element's href attribute.

Given that, perhaps it makes sense to leave the href as just a pointer (i.e., URL) to a document (or target [i.e., hash] in the active document).

Also this:

A hyperlink can have one or more hyperlink annotations that modify the processing semantics of that hyperlink.

Given that, perhaps it makes sense to allow people to declaratively define event triggers using the rel attribute (or custom logic using JS). A default behaviour might be to listen for the click listener (or some default even of A-Frame's choosing).

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

link="href: foo.bar; on: hit" is fine. Separate attribute/component isn't consistent with the framework, prescribing an event is inflexible to different use cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, what I'm suggesting is to separate the attributes/components. Perhaps this is something worth asking developers in Slack, for example, of what feels/looks more intuitive/expected.

Copy link
Member

Choose a reason for hiding this comment

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

I can say it's not intuitive nor consistent with A-Frame's design (i..e, separate attributes for related properties of a component).

At the primitive level, it can be done, which is what @dmarcos has already done. <a-link href="foo.bar" on="click">.

@cvan
Copy link
Contributor

cvan commented Nov 15, 2016

can you rebase?

@micahstubbs
Copy link

as a developer, I like the idea of link, as it feels a bit more intuitive

@dmarcos
Copy link
Member Author

dmarcos commented Jun 24, 2017

Tests pending

@dmarcos dmarcos force-pushed the link branch 3 times, most recently from ddb9260 to e0d06aa Compare June 24, 2017 12:14
* Tracked controls system.
* It maintains a list with the available tracked controllers
*/
module.exports.System = registerSystem('link', {
Copy link
Member

@ngokevin ngokevin Jun 24, 2017

Choose a reason for hiding this comment

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

I don't think this needs to be a system, it doesn't provide any services to the link component. It can be part of the scene module, which is where all the other WebVR-related handlers live (e.g., onvrdisplaypresentchange as well as enterVR/exitVR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does. I should probably initialize the variable but the currently peeked link is referenced on the system

Copy link
Member

Choose a reason for hiding this comment

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

That's fine to put any variables you need here, but the activate/deactivate event handlers belong on the scene module vs. links IMO, they're not link-specific.

@@ -0,0 +1,372 @@
/*eslint-disable */
Copy link
Member

Choose a reason for hiding this comment

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

We can pull the 3rd party components in from unpkg.com

Copy link
Member

@ngokevin ngokevin left a comment

Choose a reason for hiding this comment

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

I just did a parse initial pass, more nits than not. I'm trying to test it on Vive/Nightly, but not entering VR at the moment for me.

*/
module.exports.Component = registerComponent('link', {
schema: {
href: {default: ''},
Copy link
Member

Choose a reason for hiding this comment

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

can sort these

Copy link

Choose a reason for hiding this comment

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

Hi

color: {default: 'white', type: 'color'},
highlighted: {default: false},
highlightedColor: {default: '#24CAFF', type: 'color'},
visualAspectEnabled: {default: true},
Copy link
Member

Choose a reason for hiding this comment

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

maybe portalMode?

init: function () {
this.redirect = this.redirect.bind(this);
this.previousQuaternion = undefined;
this.hiddenEls = [];
Copy link
Member

Choose a reason for hiding this comment

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

Can comment what this does

if (data.on !== oldData.on) { this.updateEventListener(); }
if (data.peekMode !== oldData.peekMode) { this.updatePeekMode(); }
if (!data.src || oldData.src === data.src) { return; }
el.setAttribute('material', 'pano', data.src.src);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if an inline URL is passed?

href: {default: ''},
title: {default: ''},
on: {default: 'click'},
src: {type: 'asset'},
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this like image to not confuse with href and start moving away from the src name
(holdover from the 2D web)

this.cameraPosition.setFromMatrixPosition(camera.matrixWorld);
},

tick: function (time, delta) {
Copy link
Member

Choose a reason for hiding this comment

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

prefer to see the handlers first (top level) and then the helper methods

el.removeEventListener('trackpadtouchend', this.stopPeeking);
break;
default:
console.warn('Uknown controller ' + this.controller + '. Cannot remove link event listeners.');
Copy link
Member

Choose a reason for hiding this comment

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

typo

thetaStart: 0,
thetaLength: 360
});

Copy link
Member

Choose a reason for hiding this comment

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

can remove a couple of these blank lines to group the entity modifications together


/**
* The tick handles:
* 1. It swaps the plane the represents the portal with a sphere with a hole when the camera is close
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to put It behind anywhere since it's ambiguous and we can infer from context

cameraPortalOrientation = this.calculateCameraPortalOrientation();
// If the user gets very close to the portal it is replaced
// by a holed sphere where see can peek inside
if (distance < 1.0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do closer than 1 meter?

@ngokevin
Copy link
Member

Alright, Nightly enters for me now. Camera had a low height for a bit until I restarted. But I experienced that the enter VR wasn't kicking in after navigation, and the portals are too large/far away (I can't walk up to them, and or poke my head thru because they look huge)

@stevel4857
Copy link

stevel4857 commented Jun 26, 2017 via email

@ngokevin
Copy link
Member

ngokevin commented Jun 28, 2017

In link-controls.js

if (previousSelectedLinkEl || selectedLinkEl.components.link === undefined) { return; }

This threw an error for me selectedLinkEl is not yet set.

link-controls.js:302 Uncaught TypeError: Cannot read property 'components' of undefined
    at i.onMouseEnter (http://localhost:9005/js/link-controls.js:302:49)
    at Object.module.exports.fireEvent (http://localhost:9005/js/aframe-master.min.js:450:2078)
    at http://localhost:9005/js/aframe-master.min.js:293:2801
    at Array.map (native)
    at HTMLElement.value (http://localhost:9005/js/aframe-master.min.js:293:2772)
    at i.twoWayEmit (http://localhost:9005/js/aframe-master.min.js:202:3787)
    at i.onIntersection (http://localhost:9005/js/aframe-master.min.js:202:3035)
    at HTMLElement.<anonymous> (http://localhost:9005/js/aframe-master.min.js:432:132)
    at Object.module.exports.fireEvent (http://localhost:9005/js/aframe-master.min.js:450:2078)

@ngokevin
Copy link
Member

ngokevin commented Jun 28, 2017

Still looking into this, but creating the links programmatically with

var LINKS = [
  {href: '?env=arches', image: 'img/arches.png', title: 'Arches'},
  {href: '?env=checkboard', image: 'img/checkerboard.png', title: 'Checkerboard'},
  {href: '?env=egypt', image: 'img/egypt.png', title: 'Egypt'},
  {href: '?env=forest', image: 'img/forest.png', title: 'Forest'},
  {href: '?env=goaland', image: 'img/goaland.png', title: 'Goaland'},
  {href: '?env=goldmine', image: 'img/goldmine.png', title: 'Goldmine'},
  {href: '?env=japan', image: 'img/japan.png', title: 'Japan'},
  {href: '?env=poison', image: 'img/poison.png', title: 'Poison'},
  {href: '?env=threetowers', image: 'img/threetowers.png', title: 'Three Towers'},
  {href: '?env=tron', image: 'img/tron.png', title: 'Tron'},
  {href: '?env=yavapai', image: 'img/yavapai.png', title: 'Yavapai'},
];

AFRAME.registerComponent('generate-links', {
  init: function () {
    LINKS.forEach(link => {
      var linkEl;

      // Current page.
      if (window.location.search.indexOf(link.href.substring(1)) !== -1) { return; }

      linkEl = document.createElement('a-entity');
      linkEl.setAttribute('link', {
        href: link.href,
        src: link.image,
        title: link.title
      });
      this.el.appendChild(linkEl);
    });
  }
});

Gives errors like:

core:schema:warn Unknown property `0` for component/system `material`. 
aframe-master.min.js:21 core:schema:warn Unknown property `1` for component/system `material`. 
aframe-master.min.js:21 core:schema:warn Unknown property `2` for component/system `material`. 
aframe-master.min.js:21 core:schema:warn Unknown property `3` for component/system `material`. 

aframe-master.min.js:297 Uncaught (in promise) TypeError: Cannot use 'in' operator to search for 'strokeColor' in pano
    at i.updateCachedAttrValue (aframe-master.min.js:297)
    at i.updateProperties (aframe-master.min.js:297)
    at HTMLElement.value (aframe-master.min.js:289)
    at HTMLElement.value (aframe-master.min.js:289)
    at i.update (aframe-master.min.js:222)
    at i.updateProperties (aframe-master.min.js:297)
    at HTMLElement.value (aframe-master.min.js:289)
    at e (aframe-master.min.js:289)
    at Array.forEach (<anonymous>)
    at HTMLElement.value (aframe-master.min.js:289)

I debugged and found that attrValue at some point became a string value of pano rather than an object like I'd expect. material component was calling updateCachedAttrValue('pano')

@ngokevin
Copy link
Member

ngokevin commented Jun 28, 2017

I think an issue is el.setAttribute('material', 'pano', data.src.src); in link.js. If I pass in a raw URL vs <img> from <a-assets>, then it sort of breaks. This has been an awkward part of the API, we should just have the link component check if it's an <img> or string, or hope it just works as a string.

Changed to el.setAttribute('material', 'pano', typeof data.src === 'string' ? data.src : data.src.src);

@ngokevin
Copy link
Member

It doesn't look like the portals are doing look-at on the camera

@ngokevin
Copy link
Member

Should <a-entity link scale="0.5 0.5 1"> work?

@ngokevin
Copy link
Member

I think the title text above the portals need double-sided materials in case you go to the other side

@ngokevin
Copy link
Member

r+

@ngokevin ngokevin mentioned this pull request Jun 30, 2017
@philipardeljan
Copy link

philipardeljan commented Jun 30, 2017

Am I doing something wrong or maybe I don't get it, should we be able to "click" on them with mouse/trackpad?

edit: similar to this BUT instead of hovering/waiting to load you can Tap/Click on them instead? https://aframe-gallery.glitch.me/

@ngokevin
Copy link
Member

This is for VR to VR scene traversal.

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.

7 participants