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

Rename PerspectiveFrustum.fov to PerspectiveFrustum.fieldOfView #5650

Open
pjcozzi opened this issue Jul 19, 2017 · 8 comments
Open

Rename PerspectiveFrustum.fov to PerspectiveFrustum.fieldOfView #5650

pjcozzi opened this issue Jul 19, 2017 · 8 comments
Labels

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

See https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/PerspectiveFrustum.js#L42

Also search to see if fov is used anywhere else in the public Cesium API. The abbreviation is fine in the private interfaces and implementations.

@emackey
Copy link
Contributor

emackey commented Jul 19, 2017

If we're breaking the FOV API, we could consider changes to behavior as well. Originally, this was specified as fovy, a vertical-only FOV. This was problematic in that it was possible to resize the window such that the implied horizontal FOV exceeded 180 degrees and broke. Some years ago I submitted a fix, by changing to fov that would apply to whichever dimension is larger, width or height, which also aligned with what STK and some other tools do.

But recently, I've had complaints that this behavior is not as friendly to wide-screen displays. Some displays and desktop arrangements typically fill the full height consistently, as the height is relatively smaller, and the width is more variable, with side UI panels coming and going affecting the width. Having the FOV based on this larger variable-width dimension is bad because the effective vertical FOV ends up changing in a jarring way when side panels come and go. It would be more pleasing to the eye for these side panels to simply crop or un-crop the display, changing the horizontal FOV without changing the vertical FOV.

How to implement this without re-introducing the original FOV problem is up for debate. One idea I had would be to go back to a VerticalFieldOfView like fovy, but add some logic that caps the horizontal FOV and will tweak the user's selected vertical FOV to avoid the problem. This needs some more thought and experimentation to see what's the best path forward.

@emackey
Copy link
Contributor

emackey commented Oct 13, 2017

Here's a more recent idea: We change fov to fieldOfView, and add a new property, maximumFieldOfView. The fieldOfView controls the vertical FOV, but only up to the point where the horizontal FOV would cross the maximumFieldOfView, then it is capped there (for both safety and sanity).

The existing read-only fovy that reports the "true" vertical FOV should remain, although could be renamed to verticalFieldOfView, and there should be a corresponding read-only horizontalFieldOfView that reports the true horizontal FOV.

I haven't tried any of this to see what it "feels" like to a user. Obviously a good bit of hands-on usability testing would be needed before any one strategy was accepted.

@emackey
Copy link
Contributor

emackey commented Oct 13, 2017

I created a branch field-of-view to experiment with the behavior described above. It actually works surprisingly well. The old behavior already had an FOV "kink" when resizing the window, right where the window becomes square, which always felt like an odd place for it. This change essentially moves that same kink out to more of a landscape/widescreen aspect ratio, which is a more natural location for it on most modern monitors and projectors. Mobile phones in portrait mode will have a slightly smaller FOV, but that's good for small screens, and the home view automatically adapts to that. I think the only downside would be for people attempting to project this on a large vertical wall, like the side of a building, much taller than wide, then it could be odd.

I didn't attempt to do the full rename described above, as that's actually a larger change and I wanted to get other peoples' opinions on this behavior first. The rename will be useful though for making this a true "breaking" change with a new API. The old behavior can be replicated by setting fov and maxFov to the same value, so the old API can just do that with a warning. Currently, there's no sanity check that maxFov is larger than fov, that will of course need to be enforced, but for now just be careful not to test putting in bad values there.

Here's a Sandcastle demo with an FOV HUD that works in both master and the field-of-view branch in its current state:

var viewer = new Cesium.Viewer('cesiumContainer', {
    animation: false,
    timeline: false
});
var hud = document.getElementById('toolbar');
hud.style.font = '1.4rem monospace';
var frustum = viewer.camera.frustum;

viewer.clock.onTick.addEventListener(function() {
    var fovy = Cesium.Math.toDegrees(frustum.fovy);
    var fovx = Cesium.Math.toDegrees(
        Math.atan(Math.tan(frustum.fovy * 0.5) * frustum.aspectRatio) * 2.0);
    hud.innerHTML = 'FOV X: ' + fovx.toFixed(2) +
        '<br/>FOV Y: ' + fovy.toFixed(2);
});
@emackey
Copy link
Contributor

emackey commented Oct 13, 2017

Latest stable Cesium: Sandcastle demo

This branch: Sandcastle demo

Try resizing the inset Cesium window horizontally and vertically. In master, FOV X and FOV Y both max out at 60 degrees. On this branch, FOV X maxes out at 89.95, and FOV Y maxes out at only 45.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Oct 13, 2017

At quick glance, it all sounds reasonable.

@bagnell or @lilleyse any thoughts?

@lilleyse
Copy link
Contributor

I'm all for the new behavior. I don't have a widescreen monitor to test with, but I tried both demos with a squashed view and like the new way.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Dec 7, 2017

@emackey do you plan to make a pull request for this?

@emackey
Copy link
Contributor

emackey commented Dec 7, 2017

I do. I'm just swamped with other stuff right now. I have a field-of-view branch with the new mechanic working, it just needs the actual rename and the DeprecationWarning stuff added.

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