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

F2F Followup: Layers #186

Closed
toji opened this issue Feb 3, 2017 · 15 comments
Closed

F2F Followup: Layers #186

toji opened this issue Feb 3, 2017 · 15 comments
Labels
feature request Request for a new feature
Milestone

Comments

@toji
Copy link
Member

toji commented Feb 3, 2017

During the face to face meeting in Kirkland I presented a proposal for converting VRLayers from dictionaries to constructable interfaces. (Pull Request #169, which this replaces) There was some really good feedback around the proposal, especially regarding how the layer list should be handled in a more "webby" way.

Issues being addressed:

Overview:

Following Elliot's suggesions at the F2F, I propose adding a VRLayerList interface that acts a bit like a single level deep DOM node tree. This lives on the VRSession (See #179). Updates to the list happen "live", which means that the state of the list at the end of the current callback is what will be used when presenting the next frame to the VRDisplay. The layer list handles a little bit of capability advertising, such as the maximum number of layers it can contain, and when the number of layer types grows in the future we'd probably also but a mechanism for testing layer type support on the VRLayerList as well.

The layers themselves are now proper interfaces, but unlike the proposal in #169 are now live as well and so we no longer have the need to maintain an awkward pair of Layer/LayerReadOnly types. Also incorporates some of the desired fixes for the bounds setting.

Rough IDL:

interface VRLayer {};

typedef (HTMLCanvasElement or
         OffscreenCanvas) VRCanvasLayerSource;

[Constructor(optional VRCanvasLayerSource source)]
interface VRCanvasLayer : VRLayer {
  VRCanvasSource source;

  void setLeftBounds(float left, float bottom, float right, float top);
  sequence<float> getLeftBounds();

  void setRightBounds(float left, float bottom, float right, float top);
  sequence<float> getRightBounds();
};

interface VRLayerList {
  // Throws if you attempt to append more than maxLayers or if the layer type
  // isn't supported.
  [Throws] void appendLayer(VRLayer layer);
  // Throws if the layer isn't in the list.
  [Throws] void removeLayer(VRLayer layer);
  VRLayer item(unsigned long index);

  readonly attribute unsigned long length;
  readonly attribute unsigned long maxLayers;
};

partial interface VRSession {
  VRLayerList layers;
}

Example usage:

// Create a VRCanvasLayer.
let canvasLayer = new VRCanvasLayer(webglCanvas);

// Let's just assume this succeeds.
let vrSession = await vrDisplay.requestSession();

// Set the layer
vrSession.layers.appendLayer(canvasLayer);

// This takes effect because the objects are live.
canvasLayer.setLeftBounds(0.0, 0.0, 1.0, 1.0);

// And now you do your render loop. This concludes our very exciting example. :D

Oustanding questions:

  • If the layer list is live and follows DOM node tree conventions, then updating the tree will take effect at the end of the current JS callback. Assuming we want changes to the layer list to propagate to the VRDisplay on the current frame and not the next one, this means we can't begin pushing frames to the display until the current callback (almost certainly the rAF) ends. This is probably not great for latency, and limits how much additional work apps would want to do after drawing.
    • You could get around that with an explicit session.submitFrame(), but I'd like to separately look at if that's a pattern we want to carry forward.
  • You could smoosh ALL SORTS of functionality into the layer list. clear(), insertLayer(layer, index), removeLayer(index), isEmpty(), etc. Happy to hear reasons why some missing bit of functionality is necessary, but for the moment I'm trying to keep it slim. Keep in mind that in the first pass this will only support a single layer at a time.
@toji toji added enhancement feature request Request for a new feature labels Feb 3, 2017
@toji toji added this to the 1.2 milestone Feb 3, 2017
@AlbertoElias
Copy link

I like the idea but I don't understand what the maximum number of layers depends on

@toji
Copy link
Member Author

toji commented Feb 6, 2017

Maximum layer count is likely to be an artificially enforced limit (For example, I would fully expect initial WebVR implementations to only support 1 until we can flesh out compositing logic in the spec) or to be communicated from the backend implementation, which may have a limit on how many layers it can composite.

@domenic
Copy link

domenic commented Feb 6, 2017

It sounds like maybe there was this feedback at the F2F, but the VRLayerList is not a great idea. We are trying to get rid of these custom "fake arrays" from the web platform (with item() and length). For example web developers are generally confused and surprised that .map, .filter, .forEach, etc. don't work, and that you have to use .appendLayer instead of .push. Using a real array is much better if at all possible.

@toji
Copy link
Member Author

toji commented Feb 6, 2017

That may have been a misunderstanding on my part. Would be good to get @esprehn's input here, since he was the primary motivator behind moving to a live Layer list at the F2F.

@RafaelCintron
Copy link

At the F2F, we talked about layer attributes and a "computed" layer attributes. An example for the canvas layer type is the size you're supposed to render. At layer definition time, the web developer requests a renderScale, which the user agent uses in conjunction with other criteria to return render size the web developer is meant to draw with. We touched on this as part of #169 and there was debate whether the canvas itself is resized automatically by the user agent or the web developer resizes it themselves.

@toji , how are computed vs. non-computed layer attributes factored into your proposal?

@toji
Copy link
Member Author

toji commented Feb 7, 2017

This proposal avoids tackling that issue, though it's certainly a concept that may be valuable in the future.

For the specific question of canvas size, my intent with this proposal (in conjunction with #179) is that the developer would just query the canvas size from the session (with a scale factor) and resize their canvas accordingly. This is probably not the right solution for everything, but for this specific case it seemed appropriate.

@RafaelCintron
Copy link

Gotcha! The canvas size is the only "computed" property we have at the moment. Will follow up the discussion in #179.

@toji
Copy link
Member Author

toji commented Feb 8, 2017

I'd still like @esprehn to weigh in on this. Thinking about it some more, though, how would a real array be used here? Especially in light of the feedback that we've received that sequences must not be used as attributes (Issue #166). I guess the intent would be that you would set/update the whole array at once?

let layers = [ layer1, layer2 ];
vrSession.setLayers(layers);

// later
let layers = vrSession.getLayers();
layers.append(layer3);
vrSession.setLayers(layers);

That's actually not too far off from what we had previously, just using requestPresent() as the set mechanism. I thought that was what Elliot was requesting we get away from at the F2F, though.

@domenic
Copy link

domenic commented Feb 8, 2017

Yes, an API similar to that would work, or you could have vrSession.appendLayer and removeLayer (instead of setLayers) if you wanted to restrict the API to only small changes instead of giving it entirely new arrays and requiring diffing them against the old values. You can also use FrozenArray<VRLayer> so that the getting syntax is a bit nicer: vrSession.layers instead of vrSession.getLayers(). It would start returning a new array every time the underlying layer list was modified.

I am indeed curious to hear Elliott's perspective and whether he was really suggesting a fake-array approach.

@esprehn
Copy link

esprehn commented Feb 10, 2017

The discussion was that VRDisplay and VRLayer are like the DOM of VR, in the same way that AudioContext and AudioNode are the DOM of WebAudio, and that we should evolve the object model into something that's about creating retained objects with APIs in the same manner as the inputs (DOM + CSS) and computed values of style.

Given the idl above it seems like VRSession should have appendLayer and VRLayer could have remove(). The session can then have a getter that returns a FrozenArray of layers. I do agree we should avoid adding anything else with a .item() method.

Using a mutable array for the layers means we need to observe it with a Proxy, having something like .setLayers(...) be the only way to mutate it requires us to do diffing. I'd like to avoid that if possible and just make this look like DOM. Instead we should have mutator methods, and a getter for a read only view, perhaps even make layers into a linked list.

@toji
Copy link
Member Author

toji commented Feb 10, 2017

Thanks for the clarification! A few follow up questions:

At the moment it wouldn't be a concern, but down the line if we want to re-arrange layers it seems like only having an append and remove method would frequently necessitate removing and re-attaching the entire layer list? I guess you could get around that with an optional index in the appendLayer method, but I don't know if that's what you had in mind. (And again, that's an issue that doesn't affect the immediate spec needs.)

What would the benefit of making the layers into a linked list be in this case? Not sure I follow.

@toji
Copy link
Member Author

toji commented Feb 10, 2017

Ah, wait. I think I see where you were going with the linked list thing: If each layer points to the next one in the list, then the session object only needs a single setLayer/getLayer. In fact, in can just be an attribute at that point. So it becomes:

interface VRLayer {
  attribute VRLayer nextLayer;
};

// Various layer types...

partial interface VRSession {
  attribute VRLayer layer;
}

That actually works really nicely too with our goals of keeping the first pass of the API simple and extending over time, since we can easily restrict it to one layer for the time being by simply not adding the nextLayer attribute.

I think I like this idea! :D

@kearwood
Copy link
Contributor

If we implement a "nextLayer" attribute like a linked-list, could we have methods such as "remove()", insertAfter, and insertBefore? I like the structure of the linked-list, but wouldn't want to require iterating through all layers to add another layer at a point relative to another.

Perhaps as more complex relationships will grow with future types of layers (ie. portal layers), there would be more of a tree-like structure formed with children layers. In this case, we could always add a "firstChildLayer" attribute or such later on while keeping our initial spec simple.

@toji
Copy link
Member Author

toji commented Feb 17, 2017

After some more offline conversations about this I think it would actually be more productive to start with a firstLayer or baseLayer on the session, something of an analog to firstChild on a DOM node. (I've changed the explainer to use baseLayer) Then we have some flexibility moving forward: We could still go with a linked list if we chose, or we could turn it into an array of nodes that functions like the DOM node children. The important part is that it wouldn't require us to make the decision right now. :)

@toji
Copy link
Member Author

toji commented Mar 8, 2017

Feels like we've got general consensus here, and the current state of this conversation is reflected in the 2.0 explainer. Closing this issue as a result. If you have specific issues with the proposed functionality please file a new issue for it.

@toji toji closed this as completed Mar 8, 2017
@cwilso cwilso modified the milestones: Spec-Complete for 1.0, 1.0 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

7 participants