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

Rectangle issue #2624

Closed
mramato opened this issue Apr 7, 2015 · 7 comments · Fixed by #2639
Closed

Rectangle issue #2624

mramato opened this issue Apr 7, 2015 · 7 comments · Fixed by #2639

Comments

@mramato
Copy link
Contributor

mramato commented Apr 7, 2015

The below code (from KmlDataSource.js) fails because both west and east ends up being -Math.PI It works without convertLongitudeRange, but I'm using it in this case because I'm reading the data externally and values may extend beyond 180 degrees. Am I doing something wrong or is there a bug in convertLongitudeRange when values are +-PI?

var viewer = new Cesium.Viewer('cesiumContainer');

var west = Cesium.Math.convertLongitudeRange(Cesium.Math.toRadians(-180));
var south = Cesium.Math.negativePiToPi(Cesium.Math.toRadians(-60));
var east = Cesium.Math.convertLongitudeRange(Cesium.Math.toRadians(180));
var north = Cesium.Math.negativePiToPi(Cesium.Math.toRadians(60));

viewer.entities.add({
    rectangle : {
        coordinates : new Cesium.Rectangle(west, south, east, north)
    }
});

It's possible that I need to special case something during KML load, but I'm just trying to confirm convertLongitudeRange behavior is correct.

@WarpDrive
Copy link

UPDATE: wait a minute, +180 is the same angle as -180. I'll check other values and see what happens.

I threw that function into a sandcastle app to play with it. This may not be a factor here, but the floor of -0.5 is -1, though one could easily think that it would be 0. Maybe it should be absolute value then floored?

var viewer = new Cesium.Viewer('cesiumContainer');

var angle = 180/180*Math.PI;
var twoPi = 2*Math.PI;

var simplified = angle - Math.floor(angle / twoPi) * twoPi;
console.log(Math.floor(angle / twoPi));
console.log(simplified);

if (simplified < -Math.PI) {
   console.log(simplified + twoPi);
}
else if (simplified >= Math.PI) {
    console.log(simplified - twoPi);
}
else {console.log(simplified);};

If you change >= to > then -180 is wrong instead of 180 being wrong. When I get time I could figure out a fix, unless someone beats me to it.

@WarpDrive
Copy link

Re-reading what you've wrote, I see what you mean, why does this work and not with the convert function. I should read more carefully!

var viewer = new Cesium.Viewer('cesiumContainer');

var west = -Math.PI;
var south = -60/180*Math.PI;
var east = Math.PI
var north = 60/180*Math.PI;

viewer.entities.add({
    rectangle : {
        coordinates : new Cesium.Rectangle(west, south, east, north)
    }
});

It fails if east and west signs are the same, or if east is negative and west is positive, but works if west is negative and east positive.

@WarpDrive
Copy link

Here's a re-write of convertLongitudeRange that retains the sign of Math.PI inputs.

var viewer = new Cesium.Viewer('cesiumContainer');

var angle = 180/180*Math.PI;
var twoPi = 2*Math.PI;

//extract sign
var sign=1;if(angle<0){sign=-1;}
angle=Math.abs(angle);

//remove extra revolutions
var simplified = angle - Math.floor(angle / twoPi) * twoPi;

//convert to other hemi-circle
if(simplified>Math.PI)
{
    simplified=Math.PI-(simplified-Math.PI);
    sign*=-1;
}

//re-apply sign
simplified*=sign;

//return the value
console.log(simplified/Math.PI*180);

@bagnell
Copy link
Contributor

bagnell commented Apr 15, 2015

Why not use Cesium.Math.negativePiToPi for both east and west? Even though it's the same angle it will preserve the sign.

@mramato
Copy link
Contributor Author

mramato commented Apr 15, 2015

Why not use Cesium.Math.negativePiToPi for both east and west? Even though it's the same angle it will preserve the sign.

Most likely because I have no idea what I'm doing and convertLongitudeRange just sounded like the right function :) So what is the use case for convertLongitudeRange?

mramato added a commit that referenced this issue Apr 16, 2015
Replace `convertLongitudeRange` with `negativePiToPi` as discussed in #2624.

Fixes #2624
@WarpDrive
Copy link

Ya, that function works well, I'm not sure why one would use convertLongitudeRange rather than negativePiToP now that Bagnell mentioned it. Both functions go back to 2012 though.

var viewer = new Cesium.Viewer('cesiumContainer');
console.log(Cesium.Math.negativePiToPi(-Math.PI));
console.log(Cesium.Math.negativePiToPi(Math.PI));
console.log(Cesium.Math.negativePiToPi(-Math.PI-0.1));
console.log(Cesium.Math.negativePiToPi(Math.PI+0.1));

I noticed that ced9b31 also uses negativePitoPi though latitude only spans -Pi/2 to +Pi/2 , but I doubt anyone would plug in a latitude value outside of that range.

@bagnell
Copy link
Contributor

bagnell commented Apr 16, 2015

@mramato I don't know. I looked at the history and it was added when Cesium was first submitted to GitHub. It's not used anywhere though.

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

Successfully merging a pull request may close this issue.

3 participants