pragmatically and precisely compute ellipse rectangle on demand #6382
Conversation
|
@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Source/Core/EllipseGeometry.js
Outdated
| @@ -805,7 +798,7 @@ define([ | |||
| VertexFormat.pack(value._vertexFormat, array, startingIndex); | |||
| startingIndex += VertexFormat.packedLength; | |||
|
|
|||
| Rectangle.pack(value._rectangle, array, startingIndex); | |||
| Rectangle.pack(value.rectangle, array, startingIndex); | |||
There was a problem hiding this comment.
Not sure we need to pack the rectangle. It looks like we're not unpacking it
| @@ -980,6 +975,9 @@ define([ | |||
| */ | |||
| rectangle : { | |||
| get : function() { | |||
| if (!defined(this._rectangle)) { | |||
| this._rectangle = computeRectangle(this); | |||
There was a problem hiding this comment.
It can be in a separate PR, but we should also update polygon, corridor and rectangle to work this way
|
Looks good @likangning93! Those are my only comments |
|
@hpinkos this is up-to-date and passing for me locally, but I can't see CI for some reason? [EDIT] looks like Travis might just be backed up |
|
Yeah I've seen travis do that before where it just totally misses running on a commit |
Source/Core/CircleGeometry.js
Outdated
| @@ -15,6 +16,7 @@ define([ | |||
| defineProperties, | |||
| EllipseGeometry, | |||
| Ellipsoid, | |||
| Rectangle, | |||
|
I think this warrants an entry in |
geometry rectangles are currently private in the API though? |
Oh right, I forgot about that. Nevermind =) |
|
@hpinkos Travis is done, this is ready! |
|
Awesome job @likangning93! Gold star for the sandcastle example, that made testing this really easy. |
|
Update CHANGES.md? |
|
@pjcozzi no because it's a change to part of the private API |
|
OK, I thought it fixed a bug. |
In place of #6376.
Fixes #6347.
This does the thing where ellipses crossing the poles get circles as well.
Sandcastle for comparison on cesiumjs.