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 geographic limit rectangle #6987

Merged
merged 6 commits into from
Sep 10, 2018
Merged

Add geographic limit rectangle #6987

merged 6 commits into from
Sep 10, 2018

Conversation

likangning93
Copy link
Contributor

@likangning93 likangning93 commented Aug 31, 2018

Needed for #6986, because some projections really really don't work well unless you clip out everything but the local area (EPSG 2039, for example).

This is distinct from clipping planes on the globe in that this respects curvy projections, which become possible in #6986. Keeping this separate also avoids adding extra complication to the clipping planes implementation.

Pretty pictures:
limiter3d
Coffee belt in 3D

limitercv
Coffee belt in Columbus View

@cesium-concierge
Copy link

cesium-concierge commented Aug 31, 2018

Thanks for the pull request @likangning93!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@likangning93 likangning93 mentioned this pull request Aug 31, 2018
4 tasks
@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

Nice! Please update CHANGES. People have been asking for this feature for a while (even without projections).

Does it work with both infinite scrolling and 2D map rotate modes?

We should probably wait until after the release to merge this, even if we think it's ready.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 31, 2018

How hard would it be to get rid of the white seam at the IDL when east == west?
Did you check to make sure this works if east < west (ie the rectangle crosses the idl)

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

I assumed the outline was for the screenshots, is there a need to have it at all in production?

@likangning93
Copy link
Contributor Author

Whoops, sorry for the slow response.

I assumed the outline was for the screenshots, is there a need to have it at all in production?

Yes, outline was for screenshots and demos. I thought it would be helpful since we don't draw the backsides of terrain.

Did you check to make sure this works if east < west (ie the rectangle crosses the idl)

Not working, I'll try getting that fixed soon.

Does it work with both infinite scrolling and 2D map rotate modes?

I think so, but using a limiter that that covers less than -180 to 180 longitude will be discontinuous:

limiterscroll2d

@@ -84,6 +84,13 @@ define([
vertexLogDepthDefine = 'DISABLE_GL_POSITION_LOG_DEPTH';
}

var geographicLimitRectangleFlag = 0;
var geographicLimitRectangleDefine = '';
if (clippedByBoundaries) {//} && frameState.mode !== SceneMode.SCENE3D) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented out code.


/**
* A property specifying a {@link Rectangle} used to selectively prevent tiles outside a region from loading.
* For limiting terrain in scenes that use custom projections or Proj4JS projections that cause overlapping tiles.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the line about custom projections since this would likely get merged before support is added for them and that is not the only use case.

Prevents tiles from loading and also clips tiles on the boundary?

@bagnell
Copy link
Contributor

bagnell commented Sep 6, 2018

It's strange that the performance drops in 2D only when showing the rectangles. I only get ~20 FPS.

@bagnell
Copy link
Contributor

bagnell commented Sep 6, 2018

How hard would it be to change the center of projection? Columbus view might be useless if there is a limiting rectangle crossing the anti-meridian:
image

@likangning93
Copy link
Contributor Author

likangning93 commented Sep 7, 2018

It's strange that the performance drops in 2D only when showing the rectangles. I only get ~20 FPS.

I'm seeing this in master as well, although not in the 1.49 release. Interesting thing, it doesn't happen if the scene starts in 2D. I'll write up an issue.

[EDIT] open here: #7018

@likangning93
Copy link
Contributor Author

likangning93 commented Sep 7, 2018

How hard would it be to change the center of projection? Columbus view might be useless if there is a limiting rectangle crossing the anti-meridian:

Should be pretty easy in #6986, here's something that should work. Users might have to be careful with small polygons crossing the prime meridian though:

anitimeridian

[EDIT] that is a strange looking morph

// Tropics of Cancer and Capricorn
var coffeeBeltRectangle = Cesium.Rectangle.fromDegrees(-180, -23.43687, 180, 23.43687);

viewer.scene.globe.geographicLimitRectangle = coffeeBeltRectangle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called "cartographicLimitRectangle" given Cesium's Cartographic type? Same question for this filename and throughout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just limitRectangle, cullRectangle, or whatever language is consistent with the rest of Cesium?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's go with cartographicLimitRectangle.

});

// Tropics of Cancer and Capricorn
var coffeeBeltRectangle = Cesium.Rectangle.fromDegrees(-180, -23.43687, 180, 23.43687);
Copy link
Contributor

Choose a reason for hiding this comment

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

Append .0 when the intention is floating point.

Throughout.

@@ -155,6 +159,15 @@ vec4 computeWaterColor(vec3 positionEyeCoordinates, vec2 textureCoordinates, mat

void main()
{

#ifdef TILE_LIMIT_RECTANGLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to add a fast reject case to the vertex shader so that if the entire tile is outside the region, the vertex is moved beyond the camera like we do for billboards, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be necessary, the limiter is tied into tileset visibility computation. Tiles completely outside bounds won't get drawn and tiles completely inside bounds won't receive shader modification.

@likangning93
Copy link
Contributor Author

@bagnell @pjcozzi this is up-to-date

@bagnell bagnell merged commit d6ad77b into master Sep 10, 2018
@bagnell bagnell deleted the globeLimiterRectangle branch September 10, 2018 19:54
@thw0rted
Copy link
Contributor

Hi all, you forgot to update the documentation for the Globe class. The new member in the public API isn't mentioned there. It looks like it's a Rectangle that defaults to undefined? Also, should it be possible to specify this at construction time, maybe by passing it to the constructor of the owning Scene?

@kenghuo
Copy link

kenghuo commented Nov 27, 2018

Can i use other shape ?
Only 'rectangle'?

@hpinkos
Copy link
Contributor

hpinkos commented Nov 27, 2018

@kenghuo only rectangle is supported at this time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants