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

feat(cesium): support altitude modes via ground primitives #493

Merged
merged 30 commits into from
Aug 14, 2019

Conversation

wallw-teal
Copy link
Contributor

@wallw-teal wallw-teal commented Apr 22, 2019

This addresses lines/polygons either drawn by the user or from data sources. In addition, getCoordinateFromPixel was improved with Cesium's recommendation for handling the terrain vs. ellipsoid cases.

Tests:
General Features

  • Test drawing box/circle/polygon in 2D, 3D (no terrain), and 3D (terrain), and all in multiple projections.
  • Test loading lines/polys from a file and changing the altitude mode
  • Test the case from Places have interesting altitudes #381

Places

  • Verify that adding a new place allows you to choose an Altitude Mode and that the preview feature is correctly updated on each change
  • Verify that editing the place populates the Altitude Mode dropdown correctly
  • Verify that reloading the application displays the geometry correctly
  • Verify that exporting the place results in the appropriate <altitudeMode> tag being added to the KML
  • Verify that importing that KML results in the correct altitude mode
  • Add a line and polygon with altitude to places and repeat the above steps

Feature Requests:

  • Can places be assigned an altitude if terrain is on in 3D when created via Layers widget (pasting coordinates or clicking on the map)?
  • Can non-point places support terrain like polygons? Currently they are flattened.

Discussions:

  • Cesium upgrade and new items like Scene.highDynamicRange
  • Interpolation granularity

This addresses lines/polygons either drawn by the user or from data
sources. In addition, getCoordinateFromPixel was improved with
Cesium's recommendation.
@wallw-teal
Copy link
Contributor Author

Note that this does not address Measure Lines. The calculation for those is following the surface of the ellipsoid, and therefore that is where they will be drawn.

We certainly have the capability to do that calculation over terrain should anyone request it.

Copy link
Contributor

@schmidtk schmidtk left a comment

Choose a reason for hiding this comment

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

  • Creating buffers (on point/line/polygon) throws a lot of errors and doesn't create the buffer.
  • Polygon/line drawing gets clipped by the globe.

Screen Shot 2019-04-23 at 8 54 47 AM

src/plugin/cesium/cesiumrenderer.js Outdated Show resolved Hide resolved
src/os/mapcontainer.js Outdated Show resolved Hide resolved
src/plugin/cesium/sync/featureconverter.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@justin-bits
Copy link
Collaborator

justin-bits commented Apr 23, 2019

  1. Places have a negative altitude when created from the context menu with terrain off in 3D
    negative altitude

  2. Can places be assigned an altitude if terrain is on in 3D when created via Layers widget (pasting coordinates or clicking on the map)?

  3. Console error thrown on attempting to draw box, 3D. Map locks up (won’t pan).
    console error on drawing box 3d

  4. Line for polygon disappears depending on orientation
    polygon line disappears

@schmidtk
Copy link
Contributor

@justin-bits

Console error thrown on attempting two draw box, 3D. Map locks up (won’t pan).

Did you do a yarn upgrade? Cesium was updated in this PR, and it looks like you're on the previous version. That would cause the error you mentioned.

@justin-bits
Copy link
Collaborator

Altitude update (1) after Will's fix:

Altitude is non zero with terrain off after mode change. On 3d terrain off>2d>3d save place it shows altitude on saving the place (slightly positive to slightly negative). Not 100% consistent. 4.65 meters comes up frequently.

image

@justin-bits
Copy link
Collaborator

Doing yarn upgrade, will update.

@justin-bits
Copy link
Collaborator

justin-bits commented Apr 23, 2019

  1. Still valid (initially negative altitude, now 4.65)
  2. Outstanding question/request.
  3. Resolved by yarn upgrade.
  4. Still valid (line clipping).
  5. (new) The globe looks terrible, already mentioned by Kevin.

@wallw-teal
Copy link
Contributor Author

wallw-teal commented Apr 23, 2019

  1. I'm concerned that Cesium might have a bug there. Both the forums and the docs indicate that the value returned by Camera.prototype.pickEllipsoid() should have a height of zero.
  2. Sure, but I'm going to avoid adding more features until the current bugs are cleared.
  3. w00t
  4. This is due to our interpolation granularity defaulting to 100,000m while Cesium's internal granularity (which we have turned off) defaults to 9,999m. If you change the value in Settings > Interpolation from 100km to 10km that fixes it. However, that's 10x the number of vertices per area.

@justin-bits
Copy link
Collaborator

justin-bits commented Apr 23, 2019

  1. Console errors thrown on buffering a box/circle/polygon/line and place (3d, terrain on/off). Probably same thing Kevin mentioned.
Uncaught TypeError: Cannot read property 'x' of undefined
    at v.setBatchedAttribute (Cesium.js:471)
    at Y (Cesium.js:471)
    at H.update (Cesium.js:472)
    at M.update (Cesium.js:474)
    at a.update (Cesium.js:483)
    at a.update (Cesium.js:483)
    at a.update (Cesium.js:483)
    at at (Cesium.js:522)
    at it (Cesium.js:522)
    at $e (Cesium.js:522)
  1. Can non-point places support terrain like polygons? Currently they are flattened.

@wallw-teal
Copy link
Contributor Author

That is the same thing that Kevin mentioned. It has to do with polygon fills and I'm looking into it.

@wallw-teal wallw-teal changed the title feat(cesium): support altitude modes via ground primitives WIP: feat(cesium): support altitude modes via ground primitives Apr 24, 2019
@wallw-teal
Copy link
Contributor Author

wallw-teal commented Apr 24, 2019

For the interpolation granularity (affecting clipping through the earth), we could simply enable Cesium's interpolation in addition to ours. In the event that ours is at the default 100km or greater, the polygon on our side will keep the same number of vertices and only the Cesium primitive will have more at their default of 9.999km. This does appear to be a little chunky (slow) in my tests though (largely because drawn items are rendered with async: false and are updated quickly).

Edit: I'm gonna push this and see how it looks for you

@wallw-teal
Copy link
Contributor Author

Just don't forget to put Settings > Interpolation > Granularity back to 100km.

@justin-bits
Copy link
Collaborator

  • Console error on buffering resolved
  • Save places altitude working correctly
  • Still getting clipping for lines/polygons, though it appears to be significantly better

@wallw-teal
Copy link
Contributor Author

wallw-teal commented May 2, 2019

Setting these items appears to fix the globe rendering issues.

scene.highDynamicRange = false;
scene.globe.showGroundAtmosphere = false;

Most of it is solved by the second one, so we may want to leave highDynamicRange on.

@wallw-teal
Copy link
Contributor Author

I think this is ready to go other than deciding whether or not scene.highDynamicRange should be on or off.

@wallw-teal wallw-teal changed the title WIP: feat(cesium): support altitude modes via ground primitives feat(cesium): support altitude modes via ground primitives Jul 9, 2019
Copy link
Contributor

@schmidtk schmidtk left a comment

Choose a reason for hiding this comment

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

  • The create/edit place controls don't seem to update the feature/label consistently. Using Relative to Ground, changing the altitude updates both the feature/label. With Absolute, changing the altitude only updates the feature. Turning on Clamp to Ground updates the label but not the feature.
  • When terrain is enabled, Relative to Ground positions the label correctly but the feature still uses absolute positioning.
  • Changing Altitude Mode for lines/polygons doesn't update the preview feature. It does update the feature when you finish editing.
  • Altitude Mode on lines/polygons isn't saved correctly on application refresh. It seems to use absolute positioning on the feature, but editing the place always populates Clamp to Ground.
  • My personal preference would be disabling highDynamicRange. Visually I think it's too dark/muted and reduces contrast between tiles/features, making it harder to distinguish what you're looking at.

@wallw-teal
Copy link
Contributor Author

wallw-teal commented Jul 25, 2019

I think all of those issues are fixed. I'll note that Cesium does not actually support relativeToGround for lines or polygons. It is picking either GroundPrimitive (clampToGround) or Primitive for polygons, and GroundPolylinePrimitive/Geometry or Primitive/PolylineGeometry for lines. There was no option similar to heightReference for BillBoards. absolute and relativeToGround are displayed the same in those cases.

See CesiumGS/cesium#6976

schmidtk
schmidtk previously approved these changes Jul 30, 2019
clampToGround is implemented on Polyline and Primitive (polygon) not as a
heightReference value but as separate classes (GroundPolylinePrimitive and
GroundPrimitive). Therefore, if the heightReference changes, we cannot
update those primitives and must instead recreate them.
src/os/ui/featureedit.js Outdated Show resolved Hide resolved
schmidtk
schmidtk previously approved these changes Aug 12, 2019
@wallw-teal wallw-teal merged commit 5275f4f into ngageoint:master Aug 14, 2019
@wallw-teal wallw-teal deleted the fix-draw-clamped-to-ground branch August 14, 2019 13:23
@justin-bits
Copy link
Collaborator

@wallw-bits

  1. Error on creating places (with defaults) under the projections 3031 and 3413. These places shift around the globe on changing projections. On removal via the layers dialog, they also remain on the map.

  2. Box/circle/polygons shift around on the map when changing projections. Even when returning to the projection in which they were created, they are in dramatically different positions at different sizes.

  3. Units on Altitude field do not respect user settings, it defaults to meters. Also, precision is unreasonably high.

  4. On opening the Add Place dialog, the place is not visible. It only becomes visible on clicking OK.

@wallw-teal
Copy link
Contributor Author

  1. That is unrelated to this ticket
  2. I'm not able to reproduce that
  3. That is unrelated to this ticket
  4. I'll fix that

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.

3 participants