Ability to cap the canvas size#3641
Conversation
|
Nice. I wanted this because, yeah, fullscreen on Macbook is a drain. You don't have to build the dist for PRs, you can remove that. Perhaps |
This reverts commit 9f5ffd7.
– Can now set both max height and max width independently
|
Great! I've implemented the two separate attributes, and added them to the renderer schema to stop the warning. Though the default doesn't do anything. Size caps are ignored in VR mode. I'll update the documentation also, if that's all okay? EDIT: Will figure out what's going wrong with the test failing |
ngokevin
left a comment
There was a problem hiding this comment.
Added some nit comments. Thanks!
src/core/scene/a-scene.js
Outdated
| } | ||
|
|
||
| function getMaxSize (max, isVR) { | ||
| var size = { |
There was a problem hiding this comment.
define vars-on-top and initialize where needed.
var aspectRatio;
var size;
src/core/scene/a-scene.js
Outdated
| }; | ||
| } | ||
|
|
||
| function getMaxSize (max, isVR) { |
There was a problem hiding this comment.
I would call all these variables like definedMaxSize or maxSize.
src/core/scene/a-scene.js
Outdated
| }; | ||
| } | ||
|
|
||
| function getMaxSize (max, isVR) { |
There was a problem hiding this comment.
Can you document this function and what isVR will do?
src/core/scene/a-scene.js
Outdated
| * @param {boolean} embedded - Is the scene embedded? | ||
| */ | ||
| function getCanvasSize (canvasEl, embedded) { | ||
| function getCanvasSize (canvasEl, embedded, max, isVR) { |
There was a problem hiding this comment.
I think better order is canvasEl, maxSize, isVR, isEmbedded of importance
src/core/scene/a-scene.js
Outdated
| rendererConfig.antialias = rendererAttr.antialias === 'true'; | ||
| } | ||
|
|
||
| this.maxSize = { |
| physicallyCorrectLights: true;"></a-scene> | ||
| physicallyCorrectLights: true; | ||
| maxCanvasWidth: 1920; | ||
| maxCanvasHeight: 1920;"></a-scene> |
There was a problem hiding this comment.
Can do – but I suppose in most instances you'd be best having them the same value thereby capping for both landscape and portrait aspect ratios
src/components/scene/renderer.js
Outdated
| module.exports.Component = register('renderer', { | ||
| schema: { | ||
| antialias: {default: 'auto', oneOf: ['true', 'false', 'auto']}, | ||
| maxCanvasWidth: {default: -1}, |
There was a problem hiding this comment.
1920 x 1080 might be a reasonable default?
There was a problem hiding this comment.
Shouldn't the default be uncapped?
There was a problem hiding this comment.
I think it might be good to limit as default to 1920x1080 because higher resolutions are overkill. It would be nice if all A-Frame apps are capped because so many people have Retina screens. People can configure it uncapped if they want. For example, ShaderToy caps to a very small resolution.
VR mode is the more important part, I think it's okay to cap what is essentially the preview. What do you think?
There was a problem hiding this comment.
I think you're right, it's probably the sanest default – as anything but the simplest scene starts dropping frames on my iMac at full screen.
Though if we set to 1920x1080 – we're automatically constraining portrait devices to a smaller size, so perhaps 1920 x 1920 is better.
If people want to implement different constraints for mobile/tablets, they can do it manually through a custom component.
There was a problem hiding this comment.
So I added the defaults here, since it avoids the warning for an undefined attribute – but the default value doesn't have an effect, hence this line:
width: rendererAttr.maxCanvasWidth ? parseInt(rendererAttr.maxCanvasWidth) : -1
Is that a problem?
src/core/scene/a-scene.js
Outdated
| height: window.innerHeight | ||
| }; | ||
|
|
||
| if (!max || isVR || (max.width === -1 && max.height === -1)) return size; |
docs/components/renderer.md
Outdated
| > **NOTE:** When glTF models contain lights, use the physically-correct lighting mode to match | ||
| > the results in the original modeling tool. | ||
|
|
||
| ### maxCanvasWidth / maxCanvasHeight |
There was a problem hiding this comment.
I think can be explained in the attributes table
|
I played around. I am not sure setSize helps with Retina. The size is already reported as 1440x900 and pixel ratio is 2. Setting pixel ratio to 1 seems like what you'd want to do for Retina? |
|
Ahh, of course. Let's say your screen was 1920x1080 at pixel ratio of 3 – and the cap is set to 2560 x 2560. If the pixel ratio was then set to 1 but a size larger than the screen, would it cause any problems? |
|
Don't know what that means, size larger than the screen. It varies between screens. My MacBook has a pixel ratio of 2, and it's OS-configured too, my Windows is at 1.25. Rather than maxSize, we should allow configuration of |
– Rounding the short side to avoid a decimal size – Taking into account pixel ratio in sizing and comparison
|
Agreed that would be much simpler – but maybe there's value in the max size, since it means you can make the most of the pixel ratio at smaller window sizes where it wouldn't be a frame rate issue? I've just pushed a change that should hopefully fix the sizing on retina screens. |
|
Thanks! Let's see how it goes. |
Description:
Adding an attribute to the scene component which allows you to set a maximum height/width.
Currently, you get some big frame drops on high-density screens in a full width window – especially on iMacs & Macbooks with retina screens and slightly less powerful GPUs.
Was just a first stab.
Would it be more useful with a separate width and height attribute?
Tried this rather than a pixel ratio attribute, as you can make the most of the high-density screen at smaller sizes.