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

refactor(cesium): reorganize featureconverter #853

Merged

Conversation

wallw-teal
Copy link
Contributor

@wallw-teal wallw-teal commented Jan 27, 2020

This is largely a reorganization, not a rewrite or a change in logic. FeatureConverter was a spaghetti mess and hard to make sense of for anyone outside of a couple of experts.

The general design for the refactor is shown in sync/iconverter.js and sync/runconverter.js. The individual converter implementations are all pulled in by sync/converter.js.

This is largely an attempt at feature-parity with the previous converter, with the following changes:

  • Several style bugs were fixed by consolidating and reusing style code where possible instead of using the previous one-off implementations
  • All functions being sent a feature, geometry, style, context now have a consistent parameter order everywhere
  • A few minor performance items were added (typically things like using scratch arrays/objects where possible)
  • All instances of @suppress {checkTypes} were removed and fixed
  • Most larger functions have been split into smaller blocks with a couple of exceptions
  • Cesium prefers wide scene trees rather than deep ones. Therefore, instead of having converters return a Cesium.CollectionLike of items (billboards, line primitives, polygon fill/outline primitves, polylines), we now only keep the root collections on the scene and keep track of the N items added per geometry.

I attempted to change anything outside of FeatureConverter the least amount possible (VectorSynchronizer and VectorContext) other than converting them to ES6 class syntax.

FeatureConverter was previously entirely missing tests. WebGL mocks have been added and are fleshed out enough that we can use a real Cesium scene in the tests. Many tests have been added but I still have more to write.

Note that for coverage reports to work on goog module files we need a change to the goog module preprocessor.

Testing

Basic Geometries

  1. Load geomtest.json.txt (point, multipoint, line, multiline, polygon, multipolygon, geometry collection)
    • Verify that the overall file looks the same in 2D and 3D (no features in one but not the other, etc.)
    • Verify that all the items change style properly when hovered/selected
    • Verify that the items are not shown when the layer's visibility is unchecked
    • Verify that all style items apply properly to each geometry (point/icon, color, fill color, line dash, size, opacity, etc.)
    • Verify that labels work on the geometry
  2. Open the Timeline and use the Load range preset button to jump to this layer
    • Verify that it animates properly (especially with labels enabled)

Ellipses and Ellipsoids

  1. Load ellipse-test.txt (point, ellipse, ellipsoid)
    • Verify that the point appears at the origin
  2. Change the style to Ellipse
    • Verify that the ellipse is displayed
    • Verify that all style items apply to the ellipse (size, color, fill color, etc.)
    • Verify that labels work on the ellipse
    • Repeat for other ellipse styles ("Ellipse with Center", etc.)
  3. Check "Show Ground Reference"
    • Verify that the ground reference line is displayed
  4. Check "Show Ellipsoids"
    • Verify that the ellipsoid is shown
    • Verify that the ellipsoid colors are correct for outline and fill (note: size is specifically ignored for ellipsoid outline width on master and I kept that)
    • Verify that labels work on the ellipsoid

Tracks

Line String Tracks
  1. Load whale_shark.kml.txt (point/icon, line)
  2. Uncheck the migration path at the bottom to get just the points
  3. Draw a box and select the points
  4. Right-click the layer and select "Create Track"
  5. Ditch (or just hide) this layer to just view the track
    • Verify that the track displays properly with the line, endpoint, and label
    • Verify that all style items apply properly
  6. Open the Timeline and use the Load preset button to jump to "Saved Places"
    • Animate over the track and verify that it displays as expected
    • Verify that all style items apply properly to each geometry (point/icon, color, fill color, line dash, size, opacity, etc.)
Multi Line String Tracks
  1. Make two new points, one on either side of the antimeridian. Give each one a time instant (maybe back one up from now by a couple of hours for a good animation range).
  2. Select both points
  3. Right-click "Saved Places" and select "Create Track"
    • Verify that the track displays properly with the line, endpoint, and label
    • Verify that all style items apply properly
  4. Open the Timeline and use the Load preset button to jump to "Saved Places"
    • Animate over the track and verify that it displays as expected
    • Verify that all style items apply properly

Altitude Mode

  1. Enable Terrain
  2. Load AltitudeMode.kml.txt
  3. Right-click the layer and select "Go To"
  4. Points
    • Verify that "Mt. Everest Peak" is on the peak (clampToGround)
    • Verify that "Absolute Above Mt. Everest Peak" is 5km above the peak
    • Verify that "Relative to Mt. Everest Peak" is 2500m above the ground (above the peak; relativeToGround)
  5. Lines
    • Verify that "Everest Line" follows the terrain, roughly through the peak (clampToGround)
    • Verify that "Absolute Everest Line" is at the same height as "Absolute Above Mt. Everest Peak"
    • Verify that "Relative Everest Line" does not work properly because Cesium does not support relativeToGround for lines
  6. Polygons
    • Verify that "Everest Circle" follows the terrain, roughly centered on the peak (clampToGround)
    • Verify that "Absolute Everest Circle" is the same circle, but all at the altitude of "Absolute Above Mt. Everest Peak"
    • Verify that "Relative to Everest Circle" does not work properly because Cesium does not support relativeToGround for lines/polygons.

Extrusions

  1. Load KML_Samples.kml.txt (note: this is slightly modified from the original due to some stuff I noted in Potential KML issues #907)
  2. In KML Samples > Polygons > Extruded Polygon, right click the polygon and Go To it
    • Verify that it displays and highlights correctly
  3. Turn on KML Samples > Paths and Go To that folder
    • Verify that each item displays and highlights correctly with the caveat that Cesium does not support relativeToGround for anything but points/icons (at least as far as I can tell) so anything marked "Relative" is really "Absolute" with the given altitude values.
    • Note: You will need to enable terrain to see the difference between the "Tessellated" and "Untessellated" lines

MultiPoint Volume

  1. Load multipoint_volume.json.txt
    • Verify that the file performs well when loaded (the original complaint was ~2500 features with ~5500 total points was running at <10 fps on a high-quality machine)

This is largely a reorganization, not a rewrite or a change in logic.
FeatureConverter was a spaghetti mess and hard to make sense of for
anyone outside of a couple of experts.

The general design for the refactor is shown in sync/convertertypes.js
and sync/runconverter.js. The individual converter implementations are
all pulled in by sync/converter.js.

This is largely an attempt at feature-parity with the previous
converter, with the following changes:

  - Several style bugs were fixed by consolidating and reusing
    style code where possible instead of using the previous one-off
    implementations
  - All functions being sent a feature, geometry, style, and context now
    have a consistent parameter order everywhere
  - A few minor performance items were added (typically things like
    using scratch arrays objects where possible)
  - All instances of @Suppress {checkTypes} were removed and fixed
  - Most larger functions have been split into smaller blocks with a
    couple of exceptions.

I attempted to change anything outside of FeatureConverter the least
amount possible (VectorSynchronizer and VectorContext) other than
converting them to ES6 class syntax.

FeatureConverter was previously entirely missing tests. WebGL mocks
have been added and are fleshed out engouh that we can use a real
Cesium scene in the tests. Many tests have been added but I still have
more to write.

Note that the tests currently require this PR:
karma-runner/karma-googmodule-preprocessor#23
@wallw-teal
Copy link
Contributor Author

The test errors are coming from the Cesium 1.65 merge. I'll look into those.

@wallw-teal
Copy link
Contributor Author

That test is failing intermittently for me locally too. I'll look at it.

@schmidtk
Copy link
Contributor

schmidtk commented Feb 3, 2020

  • When changing size or border on the layer style, polygons are not updated. Toggling to 2D and back to 3D will sync the style.
  • When changing the icon on a layer style (ie, default to airplane), the initial icon size is larger than expected. Toggling to 2D and back to 3D will correct the size. This appears to only happen the first time an icon is selected for a particular layer. Subsequent uses of that icon init with the correct size.
    Large Icons
  • After the base icon size is fixed, the hover/selected icon size is still incorrect. It seems any color change to the icon uses the incorrect size.
    Large Hover Icon
  • In the debug build, drawing a polygon/circle/line (not box) triggers an assertion error:
Uncaught goog.asserts.AssertionError {message: "Failure: unhandled baseline undefined"}
goog.asserts.fail @ asserts.js:243
updateVerticalOrigin  @ labelconverter.js:239
  • When creating a multi-track across the AM, I'm getting an error. I created four points alternating across the AM, starting on the left (+179) side. I can also reproduce by loading MultiTrack.kml.txt and hovering the line. The file contains a gx:MultiTrack comprised of two gx:Track elements.
multidynamiclinestringconverter.js:54 Uncaught TypeError: primitives.add is not a function
    at createOrUpdateDynamicMultiLineString (multidynamiclinestringconverter.js:54)
    at Object.update (multidynamiclinestringconverter.js:78)
    at runConverter (runconverter.js:20)
    at convertGeometry (converter.js:35)
    at convert (featureconverter.js:26)
    at VectorSynchronizer.updateFeature_ (vectorsynchronizer.js:458)
    at VectorSynchronizer.updateHighlightedItems_ (vectorsynchronizer.js:636)
    at VectorSynchronizer.onSourcePropertyChange_ (vectorsynchronizer.js:403)
    at plugin.file.kml.KMLSource.boundListener (events.js:17)
    at plugin.file.kml.KMLSource.ol.events.EventTarget.dispatchEvent (eventtarget.js:88)

@wallw-teal
Copy link
Contributor Author

Most of those are fixed. I'm not totally happy with the icon stuff (there's a bit of a flicker to a larger size and back on rollover) and I'm continuing to try to improve that.

stopping work to handle the forward-referenced type issue from the compiler
This differs more from the original implementation but it helps work
around a forward-referenced type bug in the compiler when declaring the
@type of a function as a @typedef.
@wallw-teal wallw-teal force-pushed the refactor-cesium-feature-converter branch from 0a9b3ba to 7527875 Compare February 26, 2020 22:58
Also general cleanup in some other places as test coverage was added
Had to adjust our Cesium mixin for creatImage to work with
fetch options rather than a simple string. Additionally, this new
mixin is more targeted, allowing more of Cesium's image loading
logic to run.
Also gate adding fills on fill alpha > 0. When removing fills, simply do
not update the dirty flag on the item and let the update succeed with a
noop.
In order to fix (many) DeveloperErrors in Cesium, lines no longer always
use the same appearance, and rather only use the
PolylineMaterialAppearance when a dash is used in the style. This allows
for better performance in general, but now we need the
isDashPatternChanging() method to check that the dashPattern is not
defined (the correct value matching the else case of no
PolylineMaterialAppearance).
Dunno what I was thinking before. This is way more readable. The idea is
that the isFillBeingAdded check only runs once on the original
primitive, but isFillBeingRemoved needs to run on each primitive as the
method recurses.
Cesium prefers wide scene trees to deep ones, but there are several
collection implementations that do not implement the 'show' property,
preventing us from setting the visibility at the collection level
(representing layer visibility). Therefore, these collections are added
under a root PrimitiveCollection so that it's 'show' property can be
used to properly control their visibility.
Using the underlying OL style to load the icon is problematic because
ImageIcon applies color to the image. That's all well and good, but it
requires setting the imageId in Cesium to 'src + color' rather than just
'src', which blows up the texture atlas. The solution is just to load
the image directly and let Cesium color it. The downside is that custom
extensions of those icon classes will not work properly (we do not
currently have any).
Points using RegularShape styles now (potentially) use less memory by
keeping a single instance of the shape with a white fill, using the
'color' property of the billboard to color it. I say "potentially"
because the full memory usage is still occuring on the OL/StyleManager
side when the styles are created/updated. We could potentially add a
mixin loaded by the Cesium plugin which could disable that since the
billboards no longer need the original image style off of those.
Also added a test for the default color case.
MultiPolygonConverter was not properly detecting added fills, and falling
through to PolygonConverter, while not passing it enough information to
properly determine whether a fill is missing.

Instead, do PolygonConverter's check for that before looping over the
items and otherwise don't worry about it during the child loop (return
false override).
After Cesium "loads" the image, whether that is an actual load over the
network or retrieving the image from the texture atlas, we need to
update the size on the style and have it reset/recalculate the
normalizedAnchor and normalizedScale.
@jsalankey jsalankey merged commit a5cc0fe into ngageoint:master Jun 26, 2020
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