Add requestViewportScale/recommendedViewportScale #1132

klausw · 2020-09-23T18:44:58Z

klausw · 2020-09-23T18:52:53Z

I hope this captures the consensus from the previous calls. I'd appreciate a sanity check for the added internal state and update steps to make sure it accurately captures the desired behavior.

FYI, I added a fairly detailed "obtain a scaled viewport" algorithm to match the existing update the viewports which is quite specific. By contrast, construct webgl layer has a much shorter step:

The user-agent MAY choose to clamp or round |scaleFactor| as it sees fit here, for example if it wishes to fit the buffer dimensions into a power of two for performance reasons.

Please let me know if you think it would be preferable to simplify the new algorithm along those lines instead.

toji

This is looking really good! Thank you! A few comments:

toji · 2020-09-23T22:06:58Z

index.bs

@@ -1376,6 +1388,8 @@ The <dfn attribute for="XRView">projectionMatrix</dfn> attribute is the [=view/p

 The <dfn attribute for="XRView">transform</dfn> attribute is the {{XRRigidTransform}} of the viewpoint. It represents the position and orientation of the viewpoint in the {{XRReferenceSpace}} provided in {{XRFrame/getViewerPose()}}.

+The optional <dfn attribute for="XRView">recommendedViewportScale</dfn> attribute contains a UA-recommended viewport scale value that the application can use for a {{XRView/requestViewportScale()}} call to configure dynamic viewport scaling. It is `null` if the system does not implement a heuristic or method for determining a recommended scale. If not null, the value MUST be a numeric value between the [=view/minimum viewport scale=] and 1.0.


Is there a reason why we shouldn't report 1.0 instead of null on systems without any heuristics? Seems like a safer fall back, especially if developers start their development on a system that always provides a value and never test on one that doesn't.

I think that would be unhelpful. Always reporting 1.0 would make it impossible to distinguish "this system doesn't provide a heuristic" from "the heuristic currently recommends scale 1.0". That would block applications from doing their own viewport scale calculations on systems that don't provide the heuristic.

Going back a step, if we want requestViewportScale(view.recommendedViewportScale) to work without errors on all systems, I could change requestViewportScale() to silently ignore non-numeric values such as null or undefined. Would that be preferable?

That would be better, yes. Or have it treat non-numerics as an implicit "revert to default (1.0)". Either way prevents crashes of apps that haven't tested across a wide hardware range.

toji · 2020-09-23T22:09:42Z

index.bs

+
+A [=view=] has an internal <dfn for="view">minimum viewport scale</dfn> value that represents the smallest supported dynamic viewport scale for this view. On a system that does not support dynamic viewport scaling, it equals 1.0. It must be a value greater than zero and less than or equal to 1.0, and does not change for the duration of a session. The minimum value MUST be large enough to ensure that the resulting viewport has nonzero width and height after scaling.
+
+Note: Dynamic viewport scaling allows applications to render to a subset of the full-sized viewport using a scale factor that can be changed every animation frame. For correct rendering, it's essential that the XR system and application agree on the active viewport. An application can call {{XRView/requestViewportScale()}} for an {{XRView}} multiple times within a single animation frame, but the requested scale does not take effect until the application calls {{XRWebGLLayer/getViewport()}} for that view. The `getViewport` call applies the change to the current anmimation frame and locks in the view's new scaled viewport for the remainder of this animation frame, and any additional calls to `requestViewportScale` in the same animation frame have no effect. The scaled viewport continues to be used for this view for future animation frames, but can be modified by calling `requestViewportScale` followed by `getViewport` on a future frame. Optionally, the system can provide a suggested value through the {{XRView/recommendedViewportScale}} attribute based on internal performance heuristics and target framerates.


any additional calls to requestViewportScale in the same animation frame have no effect.

Is there value to keeping the requested changes and just not allowing them to apply till a subsequent frame? I can see it being mildly surprising to some developers if they're trying to update their scale for the next frame based on some measurement they took in the current one only to find that it was always getting discarded.

I've changed the logic to separately track state for "requested viewport scale" and "current viewport scale", that makes it more consistent and avoids the issue of values getting discarded.

toji · 2020-09-23T22:22:17Z

index.bs

+  1. If |scale| is smaller than |view|'s [=view/minimum viewport scale=], set it to the [=view/minimum viewport scale=].
+  1. Set the |view|'s [=view/viewport scale=] value to |scale|.
+
+</div>


I appreciate the idea that some non-primary views may want to scale separately from the primary views, and though it's a little weird to me that the primary views could scale their viewports separately it's something that appears to be well supported in the various backends.

Let's theorize about a backend that must have some/all of it's views using the same viewport size for whatever reason, though. The only compatibility path I see for that scenario is to have it force the minimum viewport scale to 1.0 across the board, thus effectively disabling this feature. (Though frankly if your backend is limited like that maybe it couldn't have supported this feature anyway?)

Does anyone think that's not sufficient? Any we might want a non-normative note that says something about how that case can be handled.

I don't have a strong opinion on this one, but IIRC there was a desire to support per-viewport scaling. I think the suggested workaround of disabling viewport scaling in the unlikely case that it's not supportable on a platform seems acceptable, but please speak up if this is concerning.

index.bs

toji · 2020-09-23T22:30:05Z

index.bs

@@ -2032,6 +2075,8 @@ The <dfn method for="XRWebGLLayer">getViewport(|view|)</dfn> method, when invoke
  1. If |session| is not equal to |layer|'s [=XRWebGLLayer/session=], throw an {{InvalidStateError}} and abort these steps.
  1. If |frame|'s [=XRFrame/active=] boolean is `false`, throw an {{InvalidStateError}} and abort these steps.
  1. If |view|'s [=XRView/frame time=] is not equal to |frame|'s [=XRFrame/time=], throw an {{InvalidStateError}} and abort these steps.
+  1. If |view|'s [=view/viewport scale=] was changed, [=update the viewports=] for |session|.


This step is a little awkward, as it may allow for running the update the viewports algorithm (which updates ALL viewports) up to once per viewport if you change and then fetch each viewport in the course of a frame.

Also, the language "was changed" is a little loose. I'd kind of prefer an explicit dirty flag or similar.

I've updated it to do a direct replacement in the list of viewport objects, and changed the "obtain a scaled viewport" method to return an XRViewport instead of glViewport so that it can be shared more easily with the "update the viewports" method.

The "was changed" is now a direct comparison of "current viewport scale" and "requested viewport scale", and I've moved the "viewport modifiable" check and update here.

Separately track "requested viewport scale" and "current viewport scale" so that changes after getViewport don't get ignored. Do an in-place viewport replacement instead of running the full "update the viewports" algorithm. Allow null arguments to requestViewportScale to avoid traps on systems that don't supply a recommended viewport scale. Rephrase notes to be (hopefully) clearer. Fix height/width cut&paste error.

klausw · 2020-09-24T01:23:03Z

@toji, I think commit baae48 should address the comments, apart from the open question about systems that can't support independently-scaled views. I've left the conversations unresolved until we have consensus that this addresses the issues.

toji

Thanks! Update are perfect, LGTM!

I am interested to hear if anyone else wants to chime in on the edge case of how to handle systems that don't allow independent scaling. (To be clear, I'm not aware of any right now. Even Cardboard supports it!) Given that it's a theoretical, though, and that there's at least a plausible support path in the event that such an API does emerge, I don't think it's worth holding up landing this for. It's something that can reasonably be handled as a follow up if anyone thinks it's worth discussing.

Manishearth · 2020-09-24T22:22:50Z

index.bs

@@ -1367,6 +1378,9 @@ enum XREye {
  readonly attribute XREye eye;
  readonly attribute Float32Array projectionMatrix;
  [SameObject] readonly attribute XRRigidTransform transform;
+  readonly attribute double? recommendedViewportScale;
+
+  undefined requestViewportScale(double? scale);


Suggested change

undefined requestViewportScale(double? scale);

void requestViewportScale(double? scale);

Done. I was following https://heycam.github.io/webidl/#idl-undefined which uses undefined in examples and has this note:

Note: This value was previously spelled void, and more limited in how it was allowed to be used.

No, the use of undefined here is correct @Manishearth. We recently merged that change for the rest of the spec.

Manishearth · 2020-09-24T22:25:58Z

index.bs

+
+The <dfn method for="XRView">requestViewportScale(|scale|)</dfn> method requests that the user agent should set the [=view/requested viewport scale=] for this viewport to the requested value.
+
+When this method is invoked on a [=view=] |view|, the user agent MUST run the following steps:


The method cannot be invoked on a [=view=] (an abstract concept that is per-session), rather it is invoked on an {{XRView}}, which contains a reference to a [=view=]

Done, thank you for the clarification.

Manishearth

overall seems good

Manishearth · 2020-09-24T22:39:19Z

index.bs

@@ -1406,7 +1406,7 @@ Note: The {{XRView/transform}} can be used to position camera objects in many re

 The <dfn method for="XRView">requestViewportScale(|scale|)</dfn> method requests that the user agent should set the [=view/requested viewport scale=] for this viewport to the requested value.

-When this method is invoked on a [=view=] |view|, the user agent MUST run the following steps:
+When this method is invoked on an {{XRView}} |view|, the user agent MUST run the following steps:


you still need to talk about |view|'s [=XRView/view=] below, since the min/requested viewport scale are properties of the [=view=]

Updated, I had missed that distinction. I'm now using separate |xrview| and |view| variables to differentiate them.

Add requestViewportScale/recommendedViewportScale

97e0bec

See immersive-web#1091

toji reviewed Sep 23, 2020

View reviewed changes

toji approved these changes Sep 24, 2020

View reviewed changes

Manishearth approved these changes Sep 24, 2020

View reviewed changes

Address manishearth's requests

1c3a3de

Manishearth reviewed Sep 24, 2020

View reviewed changes

klausw added 2 commits September 24, 2020 15:49

Fix view/XRView mixup

27ee847

Change "void" back to "undefined"

634e8cf

Manishearth added this to the Pre-CR milestone Sep 25, 2020

toji merged commit 132fa32 into immersive-web:master Sep 25, 2020

klausw mentioned this pull request Sep 25, 2020

Bring back requestViewportScaling? #1091

Closed

takahirox mentioned this pull request Oct 2, 2020

Support requestViewportScale/recommendedViewportScale MozillaReality/WebXR-emulator-extension#257

Closed

klausw mentioned this pull request Jan 12, 2021

WebXR dynamic viewport scaling mozilla/standards-positions#473

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add requestViewportScale/recommendedViewportScale #1132

Add requestViewportScale/recommendedViewportScale #1132

klausw commented Sep 23, 2020 •

edited by pr-preview bot

Loading

klausw commented Sep 23, 2020

toji left a comment

toji Sep 23, 2020

klausw Sep 24, 2020

toji Sep 24, 2020

toji Sep 23, 2020

klausw Sep 24, 2020

toji Sep 23, 2020

klausw Sep 24, 2020

toji Sep 23, 2020

klausw Sep 24, 2020

klausw commented Sep 24, 2020

toji left a comment

Manishearth Sep 24, 2020

klausw Sep 24, 2020

toji Sep 24, 2020

Manishearth Sep 24, 2020

klausw Sep 24, 2020

Manishearth left a comment

Manishearth Sep 24, 2020

klausw Sep 24, 2020

		@@ -1376,6 +1388,8 @@ The <dfn attribute for="XRView">projectionMatrix</dfn> attribute is the [=view/p

		The <dfn attribute for="XRView">transform</dfn> attribute is the {{XRRigidTransform}} of the viewpoint. It represents the position and orientation of the viewpoint in the {{XRReferenceSpace}} provided in {{XRFrame/getViewerPose()}}.

		The optional <dfn attribute for="XRView">recommendedViewportScale</dfn> attribute contains a UA-recommended viewport scale value that the application can use for a {{XRView/requestViewportScale()}} call to configure dynamic viewport scaling. It is `null` if the system does not implement a heuristic or method for determining a recommended scale. If not null, the value MUST be a numeric value between the [=view/minimum viewport scale=] and 1.0.


		A [=view=] has an internal <dfn for="view">minimum viewport scale</dfn> value that represents the smallest supported dynamic viewport scale for this view. On a system that does not support dynamic viewport scaling, it equals 1.0. It must be a value greater than zero and less than or equal to 1.0, and does not change for the duration of a session. The minimum value MUST be large enough to ensure that the resulting viewport has nonzero width and height after scaling.

		Note: Dynamic viewport scaling allows applications to render to a subset of the full-sized viewport using a scale factor that can be changed every animation frame. For correct rendering, it's essential that the XR system and application agree on the active viewport. An application can call {{XRView/requestViewportScale()}} for an {{XRView}} multiple times within a single animation frame, but the requested scale does not take effect until the application calls {{XRWebGLLayer/getViewport()}} for that view. The `getViewport` call applies the change to the current anmimation frame and locks in the view's new scaled viewport for the remainder of this animation frame, and any additional calls to `requestViewportScale` in the same animation frame have no effect. The scaled viewport continues to be used for this view for future animation frames, but can be modified by calling `requestViewportScale` followed by `getViewport` on a future frame. Optionally, the system can provide a suggested value through the {{XRView/recommendedViewportScale}} attribute based on internal performance heuristics and target framerates.

	undefined requestViewportScale(double? scale);
	void requestViewportScale(double? scale);


		The <dfn method for="XRView">requestViewportScale(\|scale\|)</dfn> method requests that the user agent should set the [=view/requested viewport scale=] for this viewport to the requested value.

		When this method is invoked on a [=view=] \|view\|, the user agent MUST run the following steps:

Add requestViewportScale/recommendedViewportScale #1132

Add requestViewportScale/recommendedViewportScale #1132

Conversation

klausw commented Sep 23, 2020 • edited by pr-preview bot Loading

klausw commented Sep 23, 2020

toji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klausw commented Sep 24, 2020

toji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klausw commented Sep 23, 2020 •

edited by pr-preview bot

Loading